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

[r2r] Zcoin native mode support integration & multi lightwalletd servers support #1438

Merged
merged 66 commits into from
Sep 12, 2022

Conversation

laruh
Copy link
Member

@laruh laruh commented Aug 8, 2022

related to #927

  • zcoin native mode support
  • multi lightwalletd servers support [will be implemented in the next iteration]
    lightwalletd ports grpcs/http:
    443/8000
    1443/8002
    2443/8003

@laruh laruh changed the title [wip] Zcoin native mode support integration [wip] Zcoin native mode support integration & multi lightwalletd servers support Sep 2, 2022
@laruh laruh changed the title [wip] Zcoin native mode support integration & multi lightwalletd servers support [r2r] Zcoin native mode support integration & multi lightwalletd servers support Sep 2, 2022
@laruh laruh changed the title [r2r] Zcoin native mode support integration & multi lightwalletd servers support [wip] Zcoin native mode support integration & multi lightwalletd servers support Sep 5, 2022
@laruh laruh changed the title [wip] Zcoin native mode support integration & multi lightwalletd servers support [r2r] Zcoin native mode support integration & multi lightwalletd servers support Sep 5, 2022
Copy link
Member

@artemii235 artemii235 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/utxo/rpc_clients.rs 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/z_coin_errors.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Awesome progress!
Please consider my suggestions

mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
let compact_tx = TonicCompactTx {
index: tx_id as u64,
hash: hash_tx_vec,
fee: 0,

Choose a reason for hiding this comment

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

Is the fee 0 always?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is note for fee in CompactTx The transaction fee: present if server can provide. In the case of a stateless server and a transaction with transparent inputs, this will be unset because the calculation requires reference to prior transactions.

The light mode all the time puts fee: 0 for compact transactions.
So, yes, it will be 0.

Comment on lines +195 to +197
drop_mutability!(hash);
drop_mutability!(prev_hash);
drop_mutability!(compact_txs);

Choose a reason for hiding this comment

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

I think there is no need to drop the mutability of the variables that will be moved immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the one hand, I agree with you, because that is how rust works.
On the other hand, using that macros we can avoid future bugs or ub (reference to this comment). We can just be sure, that everything will be ok.
In addition drop_mutability! is zero cost, so its only about readability issue.

We could just have the project rule "if you don't need the mutable variable - drop the mutability".

mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
Comment on lines 537 to 553
let db =
WalletDb::for_path(wallet_db_path, consensus_params).map_to_mm(ZcoinClientInitError::WalletDbInitFailure)?;
run_optimization_pragmas(db.sql_conn()).map_to_mm(ZcoinClientInitError::WalletDbInitFailure)?;
init_wallet_db(&db).map_to_mm(ZcoinClientInitError::WalletDbInitFailure)?;
if db.get_extended_full_viewing_keys()?.is_empty() {
init_accounts_table(&db, &[evk])?;
if let Some(check_point) = check_point_block {
init_blocks_table(
&db,
BlockHeight::from_u32(check_point.height),
BlockHash(check_point.hash.0),
check_point.time,
&check_point.sapling_tree.0,
)?;
}
}
Ok(db)

Choose a reason for hiding this comment

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

As I can see, this function body has been moved from init_light_client, where it was wrapped into async_blocking({}).
Do we need to use async_blocking({}) in this function as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I missed it during refactoring. Yeap, it should be in create_wallet_db function, because async_blocking uses spawn_blocking from tokio.

Done.

mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
@laruh laruh changed the title [r2r] Zcoin native mode support integration & multi lightwalletd servers support [wip] Zcoin native mode support integration & multi lightwalletd servers support Sep 6, 2022
@laruh laruh changed the title [wip] Zcoin native mode support integration & multi lightwalletd servers support [r2r] Zcoin native mode support integration & multi lightwalletd servers support Sep 8, 2022
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Few more notes.

mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

🔥

@artemii235 artemii235 merged commit ee81219 into dev Sep 12, 2022
@artemii235 artemii235 deleted the zcoin_native_mode_support branch September 12, 2022 06:42
@laruh laruh mentioned this pull request Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants