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): allow ARRR/ZCOIN compilation in wasm #1805

Merged
merged 32 commits into from
Jun 9, 2023
Merged

feat(zcoin): allow ARRR/ZCOIN compilation in wasm #1805

merged 32 commits into from
Jun 9, 2023

Conversation

borngraced
Copy link
Member

This PR allows ARRR to be compiled in WASM.
After this PR, I will start implementing the empty/todo fn and storages

Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced borngraced added the in progress Changes will be made from the author label May 5, 2023
@borngraced borngraced requested a review from shamardy May 5, 2023 04:20
@borngraced borngraced self-assigned this May 5, 2023
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@shamardy shamardy requested a review from laruh May 5, 2023 12:32
@borngraced borngraced added under review and removed in progress Changes will be made from the author labels May 8, 2023
@shamardy
Copy link
Collaborator

shamardy commented May 8, 2023

@borngraced can you please fix the clippy warnings related to wasm?

Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
# Conflicts:
#	mm2src/coins/z_coin.rs
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.

Thank you for starting this important feature! First review iteration.

mm2src/coins/lp_coins.rs Show resolved Hide resolved
Comment on lines 298 to 303
#[cfg(not(target_arch = "wasm32"))]
pub struct BlockDb(Connection);
#[cfg(target_arch = "wasm32")]
pub struct BlockDb(String);

#[cfg(not(target_arch = "wasm32"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

please follow BlockHeaderStorageOps example https://github.com/KomodoPlatform/atomicDEX-API/blob/d01dd41e91ccf7e353a4cdb4e2135bdeb8dfc3de/mm2src/mm2_bitcoin/spv_validation/src/storage.rs#L87
by creating a common trait that should be implemented for both struct BlockDb(Connection) and pub struct BlockDb(String);. Shouldn't pub struct BlockDb(String); use IndexedDb and not a String ? You can change these structs as you see fit.

Copy link
Member Author

@borngraced borngraced May 11, 2023

Choose a reason for hiding this comment

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

To start with, I use obvious string just to make WASM build pass as I wouldn't want to implement the storages. structure in this pr to not look messy or too complex to review.

BlockDb can't be implemented like the way BlockHeaderStorageOps is done because BlockDb additionally needs to implement a trait with a generic function.
https://github.com/KomodoPlatform/atomicDEX-API/blob/46535e882896e10aa16f3d6364007ba2a8c0cc3d/mm2src/coins/z_coin/z_rpc.rs#L363-L372

which makes doing these impossible with the current rust

https://github.com/KomodoPlatform/atomicDEX-API/blob/46535e882896e10aa16f3d6364007ba2a8c0cc3d/mm2src/coins/utxo/utxo_block_header_storage/mod.rs#L19-L21

Meanwhile, I already completed the implementation structure in another branch, so I got it covered 💯
https://github.com/KomodoPlatform/atomicDEX-API/blob/daa1b650b0ae319c79923ee0f651f889a7a438cc/mm2src/coins/z_coin/storage/blockdb/mod.rs#L30-L38

Copy link
Member

Choose a reason for hiding this comment

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

Meanwhile, I already completed the implementation structure in another branch, so I got it covered

May be it worth adding this implementation in this PR with todos if you have it? So we can see the whole picture.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cool, after Omar's comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

BlockDb can't be implemented like the way BlockHeaderStorageOps is done because BlockDb additionally needs to implement a trait with a generic function.

BlockDb can implement multiple traits, I don't see the problem. It doesn't have to be exactly like BlockHeaderStorageOps/BlockHeaderStorage but I meant to take it as an example. Another example is the LightningDB trait which I plan to implement for IndexedDB later and have a common interface. https://github.com/KomodoPlatform/atomicDEX-API/blob/a1fc8f7c92819f68c0dcdf7b1c650d86cdf945ad/mm2src/coins/lightning/ln_db.rs#L200

May be it worth adding this implementation in this PR with todos if you have it? So we can see the whole picture.

As discussed with @borngraced on DM, he will add them to this PR, as for the above suggestion about following BlockHeaderStorageOps/LightningDB/etc.. you can try that in the next PRs since it will be tricky dealing with generics.

mm2src/coins/z_coin.rs Show resolved Hide resolved
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced
Copy link
Member Author

@shamardy @laruh PR is ready for review again.

I implemented mock and structure for blockdb and only structure for walletdb in wasm

Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
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 review iteration!

mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/blockdb/mod.rs Outdated Show resolved Hide resolved
async fn get_spendable_notes(&self) -> Result<Vec<SpendableNote>, MmError<ZcashClientError>> {
let db = self.z_fields.light_wallet_db.clone();
#[cfg(target_arch = "wasm32")]
async fn my_balance_sat(&self) -> Result<u64, MmError<ZCoinBalanceError>> { todo!() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

my_balance_sat wasm implementation shouldn't return a different error from the native implementation. I understand that ZcashClientError is SqliteClientError, please consider implementing a common error type for both wasm and native implementation in the next PR, you can then implement From this new error type for SqliteClientError, etc..
I guess having a common interface as discussed here #1805 (comment) helps in this.

Copy link
Member Author

@borngraced borngraced May 24, 2023

Choose a reason for hiding this comment

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

yes I understand 💯 that was always the plan

mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced borngraced added under review and removed in progress Changes will be made from the author labels May 25, 2023
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 review iteration!

mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_coin_errors.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
shamardy
shamardy previously approved these changes May 31, 2023
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.

LGTM at this stage! Only some minor non-blocker comments!
@borngraced please don't forget to add some unit tests for IndexedDb implementations in the next PRs.

@laruh please do a final review of this when you can.

mm2src/coins/z_coin/storage/blockdb/mod.rs Show resolved Hide resolved
mm2src/coins/z_coin/storage/blockdb/mod.rs Outdated Show resolved Hide resolved
pub enum WalletDbError {
ZcoinClientInitError(ZcoinClientInitError),
ZCoinBuildError(String),
IndexedDBError(String),
Copy link
Collaborator

Choose a reason for hiding this comment

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

IndexedDBError should probably be for wasm target only too

@shamardy
Copy link
Collaborator

@borngraced please fix the PR title to make it inline with conventional commits :)

@borngraced borngraced changed the title allow ARRR compilation in wasm feat(zcoin): allow ARRR/ZCOIN compilation in wasm May 31, 2023
laruh
laruh previously approved these changes Jun 1, 2023
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.

LGTM! Thanks for your work!
I have some non blocker notes and suggestions for next PR.

In next implementations may be there is a need in trait Ops for BlockDbImpl, to impl common fns like get_latest_block, insert_block, rewind_to_height etc for two targets.
And perhaps it is worth separating the database implementations for different targets into different modules (I mean not to keep both of them in mod.rs). Lets see.

mm2src/coins/z_coin/storage/walletdb/wallet_idb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/wallet_idb.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/wallet_idb.rs Outdated Show resolved Hide resolved
@borngraced
Copy link
Member Author

LGTM! Thanks for your work! I have some non blocker notes and suggestions for next PR.

In next implementations may be there is a need in trait Ops for BlockDbImpl, to impl common fns like get_latest_block, insert_block, rewind_to_height etc for two targets. And perhaps it is worth separating the database implementations for different targets into different modules (I mean not to keep both of them in mod.rs). Lets see.

yeah, you're right... my comment to Omar here is related to this too #1805 (comment)

Signed-off-by: borngraced <samuelonoja970@gmail.com>
@borngraced borngraced dismissed stale reviews from laruh and shamardy via 48596c8 June 1, 2023 07:57
@shamardy
Copy link
Collaborator

shamardy commented Jun 8, 2023

@borngraced can you please resolve the conflicts introduced due to merging this PR #1853

# Conflicts:
#	mm2src/coins/Cargo.toml
#	mm2src/coins/z_coin.rs
#	mm2src/coins/z_coin/z_rpc.rs
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
Signed-off-by: borngraced <samuelonoja970@gmail.com>
@shamardy shamardy merged commit e876ee7 into dev Jun 9, 2023
@shamardy shamardy deleted the arrr-wasm branch June 9, 2023 01:50
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.

3 participants