Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(zcoin): implement zcoin/pirate transport layer for WASM #1996

Merged
merged 73 commits into from
Dec 22, 2023

Conversation

borngraced
Copy link
Member

@borngraced borngraced commented Oct 20, 2023

I've implemented a Zcoin transport layer that adheres to a common interface shared with the native transport. Additionally, I introduced helper functions within the FetchRequest and performed some refactoring in the mm2_net module

ZCoin activation on the web

Screenshot 2023-12-05 at 12 57 01

Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
@borngraced borngraced added the in progress Changes will be made from the author label Oct 20, 2023
@borngraced borngraced self-assigned this Oct 20, 2023
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
@borngraced borngraced changed the title feat(zcoin): implement zcoin/pirate transport layer feat(zcoin): implement zcoin/pirate transport layer for WASM Oct 20, 2023
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
# Conflicts:
#	Cargo.lock
#	mm2src/coins/Cargo.toml
#	mm2src/coins/z_coin/storage/walletdb/wasm/storage.rs
#	mm2src/coins/z_coin/z_rpc.rs
#	mm2src/mm2_main/src/wasm_tests.rs
#	mm2src/mm2_net/Cargo.toml
#	mm2src/mm2_net/src/lib.rs
#	mm2src/mm2_test_helpers/src/for_tests.rs
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, next iteration. some notes + one question

mm2src/coins/z_coin/z_params/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
/// Download, validate and return z_params from given `DOWNLOAD_URL`
async fn fetch_params(name: &str, expected_hash: &str) -> MmResult<Vec<u8>, ZcashParamsBytesError> {
let (status, file) = FetchRequest::get(&format!("{DOWNLOAD_URL}/{name}"))
.cors()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont use cors() for FetchRequest::get. Did you test it without cors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't work too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests fail without cors call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cors() doesn't fix cors issues with the server until the server was updated to allow all origin..so there's no reason to use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove cors() then, if it doesnt change the situation.
Plus, I also used cors() in get req long time ago for nft, and GUI got some error on their side, after cors deletion error has gone.
As far as I remember there is no need to add cors in get req, browser does it itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borngraced I have a question, did you check fetch_params process with react-komodefi-wasm project? Everything was fine in logs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't test with [react-komodefi-wasm](https://github.com/KomodoPlatform/react-komodefi-wasm) yet. But I'll do that today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be working as supposed

Screenshot 2023-12-05 at 12 02 09

Copy link
Member

@laruh laruh Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if no cors issues in console, then fine

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more notes

mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Show resolved Hide resolved
mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <[email protected]>
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, one more note

mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <[email protected]>
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

Signed-off-by: borngraced <[email protected]>
@shamardy shamardy requested review from shamardy and removed request for shamardy December 5, 2023 18:20
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work as always! Only 2 comments for now, will do one last review after this probably!

mm2src/mm2_net/Cargo.toml Outdated Show resolved Hide resolved
mm2src/mm2_main/src/wasm_tests.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! Please fix wasm warnings of unused imports and fix conflicts!
Will probably do one final review round after this one!

mm2src/mm2_main/tests/mm2_tests/mod.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/tests/mm2_tests/z_coin_tests.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/wasm/http.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <[email protected]>
…ort-layer

# Conflicts:
#	mm2src/coins/z_coin.rs
#	mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs
Signed-off-by: borngraced <[email protected]>
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next Iteration :)

mm2src/coins/z_coin/z_params/indexeddb.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/wasm/body_stream.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/wasm/body_stream.rs Show resolved Hide resolved
mm2src/mm2_net/src/wasm/body_stream.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/wasm/body_stream.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/wasm/body_stream.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <[email protected]>
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@shamardy shamardy merged commit 3d80b3a into zcoin_storage Dec 22, 2023
23 of 30 checks passed
@shamardy shamardy deleted the zcoin-transport-layer branch December 22, 2023 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants