Implement requesting balances of HD wallet addresses #1190

sergeyboyko0791 · 2022-01-09T11:40:50Z

Add account_balance, scan_for_new_addresses, get_new_address, init_create_new_account, init_create_new_account_status, init_create_new_account_user_action RPC calls
Refactor init_utxo according to a unified init_standalone_coin protocol
Initialize UTXO coin with a pubkey requested from a Trezor device
Move UtxoCoinBuilder, UtxoArcBuilder from utxo.rs to a separate module
Add the check_utxo_maturity flag to electrum/enable RPC call mature_confirmations not used #1181

…otocol * Move `PrivKeyBuildPolicy` to `lp_coins.rs` * Refactor `grpc_web_multi_url_request` * Add `MmRpcResult::map_err`, `RpcTaskStatus::map_err`

* Move `UtxoCoinBuilder`, `UtxoArcBuilder` from `utxo.rs` to a separate module * Add `extended_pubkey`, `derivation_path` to `HDWalletInfo`

* Move `extended_pubkey` from `UtxoHDWallet` to `UtxoHDAccount` * Register `UtxoStandardCoin` at `InitStandaloneCoinActivationOps::init_standalone_coin` * Add `RpcDerivationPath` * Push missed `coin_balance.rs` * Increase `WAIT_DOCKER_READY_TIMEOUT_MS` to 60s * Bump the `ring` crate version to 0.16.15

* Add ZCoinBuilder

* Optimize UTXO coin builders not to clone `UtxoActivationParams` * Refactor `NativeClient::list_transactions_by_address` to `NativeClient::is_address_list_transactions_empty`

* Add `init_qtum`, `init_qtum_status`, `init_qtum_user_action` RPC calls * Add `derivation_path` to `HDAccountBalance`

artemii235

Few questions on the first review iteration :)

mm2src/coins/utxo/bchd_grpc.rs

artemii235 · 2022-01-11T11:12:58Z

mm2src/coins/utxo/utxo_common.rs

+            derivation_path,
+        } = coin.derive_address(hd_account, address_id, change)?;
+
+        let balance = coin.known_address_balance(&address).await?;


Requesting balances sequentially can become a problem if users will have many addresses after a long time. It's preferable to start using batch requests here.

I splitted the wallet_balance into hd_account_balance and check_hd_account_balance, added pagging options to hd_account_balance. Could you please re-review?

I meant using JSON RPC batch requests https://www.jsonrpc.org/specification#batch. E.g. request all balances of known addresses from Electrum in a single batch.
Please check if using batches will be more effective - compare the time required to perform e.g. 10 balance requests sequentially or in the batch. If the difference is significant, consider refactoring to batch requests in the next iteration.

mm2src/coins/utxo/utxo_common.rs

shamardy

Only 2 minor comments :)

mm2src/coins/coin_balance.rs

mm2src/coins/lp_coins.rs

* Return the previous implementation of `grpc_web_multi_url_request` * Split `wallet_balance` into `hd_account_balance` and `check_hd_account_balance` RPC calls

* Add `check_utxo_maturity` to electrum/enable RPC call * Add `QtumCoinBuilder` and `QtumCoinWithIguanaPrivKeyBuilder` that override `check_utxo_maturity` * Add `UtxoCommonOps::list_all_unspent_ordered` that behaves as `UtxoCommonOps::list_unspent_ordered` before * `UtxoCommonOps::list_unspent_ordered` checks the `check_utxo_maturity` flag to use either `list_all_unspent_ordered` or `list_mature_unspent_ordered` * Rename `UtxoCommonOps::ordered_mature_unspents` to `UtxoCommonOps::list_mature_unspent_ordered`

* Use `tokio::RwLock` instead of `parking_lot::Mutex` to store HD accounts * Move HD wallet related traits and structures from lp_coins.rs to coin_hd_wallet.rs * TODO use `HwRpcTaskAwaitingStatus`, `HwRpcTaskUserAction`, `HwRpcTaskUserActionRequest`, `InitRpcTaskResponse`, `RpcTaskActionError`, `RpcTaskStatusError`, `RpcTaskStatusRequest` everywhere it's possible

sergeyboyko0791 · 2022-02-08T12:21:33Z

Since the last review iteration, I added get_new_hd_address, init_create_new_hd_account, init_create_new_hd_account_status, init_create_new_hd_account_user_action RPC calls.
@artemii235 @shamardy could you please re-review it?

shamardy

Great Work! 🔥

artemii235

I think 7 comments are sufficient for this review iteration 🙂

mm2src/coins/coin_balance.rs

mm2src/coins/hd_pubkey.rs

* Move the `HDWallet` related methods from `HDWalletCoinOps` into `HDWalletOps` * Move the `HDAccount` related methods from `HDWalletCoinOps` into `HDAccountOps` * Rename `get_new_hd_address` to `get_new_address`, `hd_account_balance` to `account_balance`, `init_create_hd_account` to `init_create_account` RPC calls

* Possible refactoring. * Removed more bounds. * Added HDAccount for HDWalletCoinOps to reduce "as HDWalletOps" constructions. * Continue refactoring * Add a default implementation of `HDWalletCoinOps::generate_new_address` * Remove `HDWalletCoinOps::address_balance` * Make `HDWalletBalanceRpcOps` independent of `HDWalletBalanceOps` Co-authored-by: Artem Vitae <artem@vitae.com>

sergeyboyko0791 · 2022-02-11T04:57:34Z

@artemii235 I hope I've resolved all issues

artemii235

Few more (hopefully minor 🙂 ) changes requests.

mm2src/coins/hd_wallet.rs

mm2src/coins/init_create_account.rs

mm2src/coins/utxo/utxo_common.rs

mm2src/coins_activation/src/standalone_coin/init_standalone_coin.rs

mm2src/coins_activation/src/utxo_activation/init_qtum_activation.rs

mm2src/crypto/src/key_pair_ctx.rs

mm2src/rpc/dispatcher/dispatcher.rs

…Id refers to an activated account/address * Add the `WithdrawFrom::AccountId` variant to allow the user to specify account_id, chain and address_id * Add `Bip44DerivationPath`, `Bip44PathToCoin`, `Bip44PathToAccount` * Avoid extracting a pubkey from a Trezor device on `init_withdraw`

# Conflicts: # mm2src/docker_tests/docker_tests_common.rs

* Avoid using `clippy::type_complexity` by refactoring the `RpcTask` module * Typo: address_unspendable_balanAce

* Rename `ExpectedHDWalletDerivationMethod` to `CoinIsActivatedNotWithHDWallet` * Reorder RPCs alphabetically * impl `From<KeyPair>`

sergeyboyko0791 · 2022-02-21T04:34:18Z

I think it's ready for the next review iteration :)
cc @artemii235

artemii235

🔥 Thanks for all fixes!

artemii235

Reapproving.

sergeyboyko0791 added 8 commits December 20, 2021 19:20

Refactor init_utxo according to a unified init_standalone_coin pr…

fc64fb8

…otocol * Move `PrivKeyBuildPolicy` to `lp_coins.rs` * Refactor `grpc_web_multi_url_request` * Add `MmRpcResult::map_err`, `RpcTaskStatus::map_err`

Push missing files

93ba576

Initialize UTXO coin with a pubkey requested from a Trezor device

6e97691

* Move `UtxoCoinBuilder`, `UtxoArcBuilder` from `utxo.rs` to a separate module * Add `extended_pubkey`, `derivation_path` to `HDWalletInfo`

Merge branch 'dev' into init-utxo-with-trezor

ac47794

Add wallet_balance RPC call

55ae994

Fix zhtlc feature compilation errors

d874174

* Add ZCoinBuilder

Minor changes

70abf52

* Optimize UTXO coin builders not to clone `UtxoActivationParams` * Refactor `NativeClient::list_transactions_by_address` to `NativeClient::is_address_list_transactions_empty`

sergeyboyko0791 assigned artemii235, shamardy and sergeyboyko0791 and unassigned artemii235 and shamardy Jan 9, 2022

sergeyboyko0791 requested review from artemii235, Milerius and shamardy January 9, 2022 11:42

sergeyboyko0791 added 4 commits January 9, 2022 19:45

Fix WASM, ZCash compile errors

918b01c

Fix rustfmt warnings

584ad3c

Fix compilation of docker tests

5d1499f

Add QTUM coin initialization with Trezor

497c1f6

* Add `init_qtum`, `init_qtum_status`, `init_qtum_user_action` RPC calls * Add `derivation_path` to `HDAccountBalance`

artemii235 reviewed Jan 11, 2022

View reviewed changes

shamardy requested changes Jan 11, 2022

View reviewed changes

mm2src/coins/coin_balance.rs Outdated Show resolved Hide resolved

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved

sergeyboyko0791 and others added 2 commits January 21, 2022 17:28

Fix recreate_swap_data::convert_maker_to_taker_events

6c913b0

* Return the previous implementation of `grpc_web_multi_url_request` * Split `wallet_balance` into `hd_account_balance` and `check_hd_account_balance` RPC calls

This was linked to issues Jan 25, 2022

mature_confirmations not used #1181

Closed

Hardware wallets support #964

Open

sergeyboyko0791 added 2 commits February 8, 2022 12:00

Use RPC common structures and errors wherever possible

6fbdb08

shamardy previously approved these changes Feb 8, 2022

View reviewed changes

artemii235 reviewed Feb 9, 2022

View reviewed changes

Perform naming refactoring

790c46e

sergeyboyko0791 dismissed shamardy’s stale review via 790c46e February 9, 2022 12:10

sergeyboyko0791 and others added 4 commits February 9, 2022 19:43

Add the HDWalletCoinAndBalanceOps trait alias

e0f1795

Refactor XPubExtractorUnchecked

cce6dd2

Merge branch 'dev' into init-utxo-with-trezor

4d4bc83

artemii235 requested changes Feb 14, 2022

View reviewed changes

artemii235 mentioned this pull request Feb 17, 2022

Add confirmation settings to orderbook #1217

Merged

sergeyboyko0791 added 4 commits February 17, 2022 15:28

Merge branch 'dev' into init-utxo-with-trezor

16738f3

# Conflicts: # mm2src/docker_tests/docker_tests_common.rs

Fix PR issues

13a9236

* Avoid using `clippy::type_complexity` by refactoring the `RpcTask` module * Typo: address_unspendable_balanAce

Fix PR issues, make RpcTaskId unique

d9cb11e

* Rename `ExpectedHDWalletDerivationMethod` to `CoinIsActivatedNotWithHDWallet` * Reorder RPCs alphabetically * impl `From<KeyPair>`

Fix rustfmt

5ad36c6

artemii235 approved these changes Feb 21, 2022

View reviewed changes

artemii235 previously approved these changes Feb 21, 2022

View reviewed changes

Remove KeyPairArc::from_key_pair

4ab9b78

sergeyboyko0791 dismissed artemii235’s stale review via 4ab9b78 February 21, 2022 07:55

artemii235 approved these changes Feb 21, 2022

View reviewed changes

shamardy approved these changes Feb 21, 2022

View reviewed changes

Milerius approved these changes Feb 21, 2022

View reviewed changes

artemii235 merged commit d95da9f into dev Feb 22, 2022

artemii235 deleted the init-utxo-with-trezor branch February 22, 2022 09:17

sergeyboyko0791 mentioned this pull request Mar 11, 2022

Hardware wallets support #964

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement requesting balances of HD wallet addresses #1190

Implement requesting balances of HD wallet addresses #1190

sergeyboyko0791 commented Jan 9, 2022 •

edited

Loading

artemii235 left a comment

artemii235 Jan 11, 2022

sergeyboyko0791 Jan 21, 2022

artemii235 Feb 21, 2022

shamardy left a comment

sergeyboyko0791 commented Feb 8, 2022 •

edited

Loading

shamardy left a comment

artemii235 left a comment

sergeyboyko0791 commented Feb 11, 2022

artemii235 left a comment

sergeyboyko0791 commented Feb 21, 2022

artemii235 left a comment

artemii235 left a comment

Implement requesting balances of HD wallet addresses #1190

Implement requesting balances of HD wallet addresses #1190

Conversation

sergeyboyko0791 commented Jan 9, 2022 • edited Loading

artemii235 left a comment

Choose a reason for hiding this comment

artemii235 Jan 11, 2022

Choose a reason for hiding this comment

sergeyboyko0791 Jan 21, 2022

Choose a reason for hiding this comment

artemii235 Feb 21, 2022

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Feb 8, 2022 • edited Loading

shamardy left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Feb 11, 2022

artemii235 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Feb 21, 2022

artemii235 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Jan 9, 2022 •

edited

Loading

sergeyboyko0791 commented Feb 8, 2022 •

edited

Loading