[r2r] UTXO RPC batch requests #1255

sergeyboyko0791 · 2022-04-14T12:34:10Z

Optimize requesting balances of HD addresses by using RPC batch request.

Optimize requesting balances on init_utxo, init_qtum RPCs by using batch requests
Optimize addresses_balances RPC
Refactor scan_for_new_addresses RPC by spawning it as an RPC task, rename the method to init_scan_for_new_addresses
Refactor list_mature_unspent_ordered, list_all_unspent_ordered, list_unspent_ordered from UtxoCommonOps into the following:
- ListUtxoOps::get_mature_unspent_ordered_map - returns available mature and immature unspents in ascending order for every given addresses: Vec<Address>
- ListUtxoOps::get_all_unspent_ordered_map - returns available unspents in ascending order for every given Vec<Address>
- ListUtxoOps::get_unspent_ordered_list - returns available unspents in ascending order
- ListUtxoOps::get_unspent_ordered_map - returns available unspents in ascending order for every given Vec<Address>
Refactor UtxoCommonOps::get_verbose_transaction_from_cache_or_rpc to UtxoCommonOps::get_verbose_transactions_from_cache_or_rpc. The method requests multiple transactions concurrently
Add UtxoRpcClientOps::list_unspent_group, UtxoRpcClientOps::get_verbose_transactions, UtxoRpcClientOps::display_balances

* Refactor `CryptoCtx` to be a structure that contains `IguanaArc` always and a constructible `HwArc` * Add `init_trezor` RPC call that initializes `HwArc` * Rename `KeyPairCtx` to `IguanaCtx` * Comment out `mm_init_task`, `rpc_command` modules, `mm_init_status`, `mm_init_user_action` RPC calls

* Process batch response in the same order in which the requests were sent

* Add `utxo_common_tests.rs`

* Refactor functions within `utxo::tx_cache` to be able to work concurrently * Refactor `UtxoCommonOps::get_verbose_transactions_from_cache_or_rpc` to be able to work concurrently * Remove `UtxoCommonOps::list_all_unspent_ordered`, `UtxoCommonOps::list_mature_unspent_ordered` * Add the `ListUtxoOps` trait and its methods `get_all_unspent_ordered_map`, `get_mature_unspent_ordered_map`, `get_unspent_ordered_map` * Move and rename `UtxoCommonOps::list_unspent_ordered` to `ListUtxoOps::get_unspent_ordered_list`

# Conflicts: # mm2src/coins/hd_pubkey.rs # mm2src/coins/hd_wallet.rs # mm2src/coins/utxo.rs # mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs # mm2src/coins/utxo/utxo_common.rs # mm2src/coins/utxo/utxo_withdraw.rs # mm2src/crypto/src/crypto_ctx.rs # mm2src/lp_init/init_context.rs # mm2src/lp_init/init_hw.rs # mm2src/lp_native_dex.rs # mm2src/rpc/dispatcher/dispatcher.rs

* Move `account_balance`, `init_withdraw`, `init_create_account` RPCs from `coins` to `coins::rpc_command` * Use `RpcTask` for `init_scan_for_new_addresses`

sergeyboyko0791 · 2022-04-17T13:37:08Z

mm2src/lp_ordermatch/my_orders_storage.rs

@@ -212,6 +212,8 @@ mod native_impl {
                                    my_taker_order_file_path, my_taker_orders_dir};
    use common::fs::{read_dir_json, read_json, remove_file_async, write_json, FsJsonError};

+    const USE_TMP_FILE: bool = false;


Do we need to use tmp files when save orders?

sergeyboyko0791 · 2022-04-17T13:37:21Z

mm2src/lp_swap/saved_swap.rs

@@ -176,6 +176,8 @@ mod native_impl {
    use crate::mm2::lp_swap::{my_swap_file_path, my_swaps_dir};
    use common::fs::{read_dir_json, read_json, write_json, FsJsonError};

+    const USE_TMP_FILE: bool = false;


Same question

shamardy

Great Work! Just a few minor comments

mm2src/coins/rpc_command/account_balance.rs

mm2src/coins/utxo/rpc_clients.rs

artemii235

First review iteration.

artemii235 · 2022-04-22T11:15:25Z

mm2src/coins/z_coin.rs

@@ -1321,6 +1322,38 @@ impl UtxoTxBroadcastOps for ZCoin {
    }
 }

+#[async_trait]
+#[cfg_attr(test, mockable)]
+impl ListUtxoOps for ZCoin {


ZCoin is not assumed to work with transparent UTXOs. This trait should not be implemented for it.

Unfortunately, ZCoin::preimage_trade_fee_required_to_send_outputs is based on utxo_common::preimage_trade_fee_required_to_send_outputs, that requires ListUtxoOps to generate a preimage transaction.
https://github.com/KomodoPlatform/atomicDEX-API/blob/778461afccf42c17da0e02394ef6890a092a7a99/mm2src/coins/z_coin.rs#L1425-L1433

So I think we need to implement this method requesting z_unspents instead and then we can get rid of the ListUtxoOps trait implementation for ZCoin.
Do you think I need to do it within this PR?

I can refactor it during next iteration of Zcoin implementation. Not required for now.

But I already have a different idea: if only one method from a trait is required to split these implementations, then we can maybe add a trait like TradePreimageUtxoOps and implement only it for Zcoin? I didn't dig in the code regarding this, so not sure how feasible it will be. @sergeyboyko0791

Could you please explain more how TradePreimageUtxoOps can allow us to avoid implementing ListUtxoOps for ZCoin? 😅

I need to check this in more details myself. It will require some time to perform this refactoring, so we can leave this as is for now.

mm2src/coins/utxo/bch.rs

mm2src/coins/utxo/rpc_clients.rs

mm2src/coins/utxo/tx_cache.rs

mm2src/coins/utxo/utxo_common.rs

mm2src/rpc/dispatcher/dispatcher.rs

* Do not inherit `UtxoCommonOps` from `ListUtxoOps` (due to `ZCoin` support) * Refactor some of `UtxoRpcClientOps`, `utxo_common` methods by getting rid of nesting * Add the `common::custom_iter::CollectInto` trait * Add `ListUtxoOps::get_mature_unspent_ordered_list`

* Reorder RPCs alphabetically

* Fix `from_id` paging option for `account_balance` RPC * Fix WASM build

* Divide `ListUtxoOps` into `GetUtxoListOps` and `GetUtxoMapOps` * Implement `GetUtxoMapOps` for `UtxoStandardCoin` and `QtumCoin` only

* Rename `UtxoVerboseCache` to `FsVerboseCache` * Add `WasmVerboseCache` that is an alias to `DummyVerboseCache`

sergeyboyko0791 · 2022-05-02T05:56:43Z

@artemii235 @shamardy @ozkanonur @caglaryucekaya PR is ready for the review

artemii235

Few changes requests and questions.

artemii235 · 2022-05-02T05:15:56Z

mm2src/coins/utxo/bch.rs

        let mut result = BchUnspents::default();
        for unspent in utxos {
            if unspent.outpoint.index == 0 {
-                // zero output is reserved for OP_RETURN of specific protocols
-                // so if we get it we can safely consider this as standard BCH UTXO
+                // Zero output is reserved for OP_RETURN of specific protocols


To avoid duplicated if unspent.outpoint.index == 0 check, we can use something like

let mut result = BchUnspents::default(); let mut temporary_undetermined = Vec::new(); let to_verbose: HashSet<H256Json> = utxos .into_iter() .filter_map(|unspent| { if unspent.outpoint.index == 0 { // Zero output is reserved for OP_RETURN of specific protocols // so if we get it we can safely consider this as standard BCH UTXO. // There is no need to request verbose transaction for such UTXO. result.add_standard(unspent); None } else { let hash = unspent.outpoint.hash.reversed().into(); temporary_undetermined.push(unspent); Some(hash) } }) .collect(); let verbose_txs = self .get_verbose_transactions_from_cache_or_rpc(to_verbose) .compat() .await?; for unspent in temporary_undetermined { ... }

What do you think?

I tried to do something like this, but couldn't do it nicely. Will change it, thanks!

artemii235 · 2022-05-02T05:37:34Z

mm2src/common/jsonrpc_client.rs

+        T: DeserializeOwned + Send + 'static;
+}
+
+impl<I> BatchRequest for I


It looks like the iterator is sending actual requests while it's responsibility of rpc_client. It can also be confused with combinators like map/filter/etc.. So the interface should be as follows:

fn batch_rpc<Rpc, T>(&self (Rpc), requests: I) -> RpcRes<Vec<T>>

Yes, this makes sense! I wanted to use the trait this way:

tx_ids .iter() .map(|txid| rpc_req!(self, "getrawtransaction", txid, verbose)) .batch_rpc(self)

But I agree that it might be confusing.

artemii235 · 2022-05-02T05:48:20Z

mm2src/coins/utxo.rs

@@ -124,6 +130,8 @@ const DEFAULT_GAP_LIMIT: u32 = 20;

 pub type GenerateTxResult = Result<(TransactionInputSigner, AdditionalTxData), MmError<GenerateTxError>>;
 pub type HistoryUtxoTxMap = HashMap<H256Json, HistoryUtxoTx>;
+pub type MatureUnspentMap = HashMap<Address, MatureUnspentList>;
+pub type RecentlySpentOutPointsGuard<'a> = AsyncMutexGuard<'a, RecentlySpentOutPoints>;


RecentlySpentOutPoints support only single for_script_pubkey now. It has to be refactored for HD wallets support. Do you plan to do it on next iterations?

Yes, there is a note here. I'm still thinking about the refactoring. Can I refactor it at the next iteration with tx history?

artemii235 · 2022-05-02T08:22:18Z

mm2src/coins/utxo/rpc_clients.rs

@@ -668,6 +746,34 @@ impl UtxoRpcClientOps for NativeClient {
        )
    }

+    fn display_balances(&self, addresses: Vec<Address>, decimals: u8) -> UtxoRpcFut<Vec<(Address, BigDecimal)>> {
+        fn address_balance_from_unspent_map(address: &Address, unspent_map: &UnspentMap, decimals: u8) -> BigDecimal {


If this can be reused somewhere else, it's preferred to move it outside another fn.

I think address_balance_from_unspent_map is pinned to the NativeClient::display_balances, but I think this can be reused later somewhere:

unspents.iter().fold(BigDecimal::from(0), |sum, unspent| { sum + big_decimal_from_sat_unsigned(unspent.value, decimals) })

Moved address_balance_from_unspent_map outside the NativeClient::display_balances method

* Refactor `BchRequest` trait into `JsonRpcBatchClient` * Move `send_batch_request` from `JsonRpcClient` to `JsonRpcBatchClient` * Refactor `BchCoin::utxos_into_bch_unspents`

onur-ozkan

First review iteration.

First of all great work with documenting the codebase! :) I tried to not spam the same topic in the PR comments. So please consider them as general optimizations like inlining public functions, removing the unneeded variables, etc.

About removing the variable, I think we should avoid creating variables unless the code will be very hard to read and maintain. Compilers these days are trying to solve this in order to avoid memory preasure, but it's not guaranteed so we can't rely on that specially in projects like this.

onur-ozkan · 2022-05-02T12:04:05Z

mm2src/coins/qrc20.rs

+    /// Please note the method is overridden for Native mode only.
+    #[cfg(not(target_arch = "wasm32"))]
+    fn tx_cache(&self) -> UtxoVerboseCacheShared {
+        let tx_cache_path = self.ctx().dbdir().join("TX_CACHE");


How about creating a constant for "TX_CACHE", I see it's used multiple times. Would be better if we have constant for this value so we don't need to make so many diffs in order to update this value.

Will it be ok for you if I do the following (but without the constant)?

#[cfg(not(target_arch = "wasm32"))] fn tx_cache(&self) -> UtxoVerboseCacheShared { crate::utxo::tx_cache::fs_tx_cache::FsVerboseCache::new(self.ticker().to_owned(), self.tx_cache_path()) .into_shared() } fn tx_cache_path(&self) -> PathBuf { self.ctx().dbdir().join("TX_CACHE") }

onur-ozkan · 2022-05-02T12:19:47Z

mm2src/coins/qrc20/qrc20_tests.rs

@@ -55,7 +56,7 @@ fn check_tx_fee(coin: &Qrc20Coin, expected_tx_fee: ActualTxFee) {

 #[test]
 fn test_withdraw_impl_fee_details() {
-    Qrc20Coin::list_mature_unspent_ordered.mock_safe(|coin, _| {
+    Qrc20Coin::get_unspent_ordered_list.mock_safe(|coin, _| {


question

I see the functionality has been changed a little bit in this test function. Is that how it's supposed to be?

Sorry, I didn't understand your question. If I'm not mistaken, neither test_withdraw_impl_fee_details nor Qrc20Coin::get_unspent_ordered_list haven't been changed.

What do you mean? 😅

the functionality has been changed a little bit in this test function

What I mean is list_mature_unspent_ordered renamed into get_mature_unspent_ordered_list, but you used get_unspent_ordered_list . I wanted to know if you did it on purpose, I thought you might be confused about the function name. I guess you did it on purpose so no problem 👍

I renamed it because there is similar method that returns UnspentMap of address -> UnspentList. So map_mature_unspent_ordered doesn't fit its purpose, so I had to rename all of those methods to get_[all/mature]_unspent_ordered_[list/map] 😁

onur-ozkan · 2022-05-02T12:22:48Z

mm2src/coins/rpc_command/account_balance.rs

+    ) -> MmResult<HDAccountBalanceResponse, HDAccountBalanceRpcError>;
+}
+
+pub async fn account_balance(


This can be optimized in a following way:

#[inline] pub async fn account_balance( ctx: MmArc, req: HDAccountBalanceRequest, ) -> MmResult<HDAccountBalanceResponse, HDAccountBalanceRpcError> { match lp_coinfind_or_err(&ctx, &req.coin).await? { MmCoinEnum::UtxoCoin(utxo) => utxo.account_balance_rpc(req.params).await, MmCoinEnum::QtumCoin(qtum) => qtum.account_balance_rpc(req.params).await, _ => MmError::err(HDAccountBalanceRpcError::CoinIsActivatedNotWithHDWallet), } }

onur-ozkan · 2022-05-02T12:38:46Z

mm2src/coins/rpc_command/account_balance.rs

+        Coin: HDWalletBalanceOps + CoinWithDerivationMethod<HDWallet = <Coin as HDWalletCoinOps>::HDWallet> + Sync,
+        <Coin as HDWalletCoinOps>::Address: fmt::Display + Clone,
+    {
+        let hd_wallet = coin.derivation_method().hd_wallet_or_err()?;


I would prefer the shorter way as in the following diff:

@@ -73,14 +73,15 @@ pub mod common_impl { Coin: HDWalletBalanceOps + CoinWithDerivationMethod<HDWallet = <Coin as HDWalletCoinOps>::HDWallet> + Sync, <Coin as HDWalletCoinOps>::Address: fmt::Display + Clone, { - let hd_wallet = coin.derivation_method().hd_wallet_or_err()?; - - let account_id = params.account_index; - let chain = params.chain; - let hd_account = hd_wallet - .get_account(account_id) + let hd_account = coin + .derivation_method() + .hd_wallet_or_err()? + .get_account(params.account_index) .await - .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount { account_id })?; + .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount { + account_id: params.account_index, + })?; + let total_addresses_number = hd_account.known_addresses_number(params.chain)?; let from_address_id = match params.paging_options { @@ -90,13 +91,13 @@ pub mod common_impl { let to_address_id = std::cmp::min(from_address_id + params.limit as u32, total_addresses_number); let addresses = coin - .known_addresses_balances_with_ids(&hd_account, chain, from_address_id..to_address_id) + .known_addresses_balances_with_ids(&hd_account, params.chain, from_address_id..to_address_id) .await?; let page_balance = addresses.iter().fold(CoinBalance::default(), |total, addr_balance| { total + addr_balance.balance.clone() }); - let result = HDAccountBalanceResponse { + Ok(HDAccountBalanceResponse { account_index: params.account_index, derivation_path: RpcDerivationPath(hd_account.account_derivation_path()), addresses, @@ -106,8 +107,6 @@ pub mod common_impl { total: total_addresses_number, total_pages: calc_total_pages(total_addresses_number as usize, params.limit), paging_options: params.paging_options, - }; - - Ok(result) + }) } }

to avoid from memory pressure and doing extra process executions. Do you think that this is way more complicated in order to read and maintain code?

No, I think it doesn't make the code more complicated. Will do 🙂

Refactored, but I left

let account_id = params.account_index;

variable, because it's used here

- .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount { account_id })?; + .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount { + account_id: params.account_index, + })?;

onur-ozkan · 2022-05-02T13:00:13Z

mm2src/coins/rpc_command/hd_account_balance_rpc_error.rs

+            RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()),
+            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout),
+            RpcTaskError::NoSuchTask(_) | RpcTaskError::UnexpectedTaskStatus { .. } => {
+                HDAccountBalanceRpcError::Internal(error)


Since the error is only used once, maybe it's better to not create variable and directly do e.to_string() here.

e is moved at this moment. There are other ways to convert the error to string on NoSuchTask and UnexpectedTaskStatus only:

match e { RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()), RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout), error @ RpcTaskError::NoSuchTask(_) | error @ RpcTaskError::UnexpectedTaskStatus { .. } => { HDAccountBalanceRpcError::Internal(error.to_string()) }, RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal), }

or

match e { RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()), RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout), RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal), error => HDAccountBalanceRpcError::Internal(error.to_string()), }

I'd prefer the first solution but it looks scary a bit 😅
What do you think?

e is moved at this moment. There are other ways to convert the error to string on NoSuchTask and UnexpectedTaskStatus only:

match e { RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()), RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout), error @ RpcTaskError::NoSuchTask(_) | error @ RpcTaskError::UnexpectedTaskStatus { .. } => { HDAccountBalanceRpcError::Internal(error.to_string()) }, RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal), }

or

match e { RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()), RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout), RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal), error => HDAccountBalanceRpcError::Internal(error.to_string()), }

I'd prefer the first solution but it looks scary a bit sweat_smile What do you think?

How about the following example:

@@ -94,14 +94,13 @@ impl From<AddressDerivingError> for HDAccountBalanceRpcError { impl From<RpcTaskError> for HDAccountBalanceRpcError { fn from(e: RpcTaskError) -> Self { - let error = e.to_string(); - match e { + match &e { RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()), - RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout), + RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(*timeout), RpcTaskError::NoSuchTask(_) | RpcTaskError::UnexpectedTaskStatus { .. } => { - HDAccountBalanceRpcError::Internal(error) + HDAccountBalanceRpcError::Internal(e.to_string()) }, - RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal), + RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal.to_string()), } } }

Also, currently RpcTaskError::Internal(internal) => ... already does the partially move.

Done
I figured out the following:

let res: Result<String, String> = Err("foo".to_owned()); let own_value: String = match res { Ok(own_ok) => own_ok, // `_` is syntactic sugar that doesn't take ownership of `res`, // so `res` can be used in this branch as it's not even borrowed. Err(_) => res.expect_err(""), };

mm2src/coins/utxo.rs

mm2src/common/jsonrpc_client.rs

onur-ozkan · 2022-05-02T15:38:33Z

mm2src/coins/utxo/utxo_tests.rs

    assert!(unsafe { LIST_MATURE_UNSPENT_ORDERED_CALLED });
 }

 /// `UtxoStandardCoin` hasn't to check UTXO maturity if `check_utxo_maturity` is not set.
 /// https://github.com/KomodoPlatform/atomicDEX-API/issues/1181
 #[test]
 fn test_utxo_standard_without_check_utxo_maturity() {
-    static mut LIST_ALL_UNSPENT_ORDERED_CALLED: bool = false;
+    static mut GET_ALL_UNSPENT_ORDERED_LIST_CALLED: bool = false;


Can you please specify a short doc something like:

/// Checks if `fn get_all_unspent_ordered_list` is called or not.

It could be a little bit hard to understand at the first try for newbies(like me).

Ok, sure :)

onur-ozkan · 2022-05-02T15:41:22Z

mm2src/coins/utxo/utxo_tests.rs

+
+    let rpc_client = native_client_for_test();
+
+    let expected: Vec<(Address, BigDecimal)> = vec![


I would put expected value right before the assertion rather than in the middle of the test flow. Just my opinion, not necessary change. :)

The expected value is used in the line below
https://github.com/KomodoPlatform/atomicDEX-API/blob/7224b85e39a2406b5b9e8cb9a8e69cd59fda514f/mm2src/coins/utxo/utxo_tests.rs#L3793-L3800

This could be done in more lines:

let addresses = vec![ "RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(), "RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(), "RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(), "RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(), ]; let addresses = expected.iter().map(|(address, _)| address.clone()).collect(); let actual = rpc_client .display_balances(addresses, TEST_COIN_DECIMALS) .wait() .unwrap(); let expected: Vec<(Address, BigDecimal)> = vec![ ("RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(), BigDecimal::from(5.77699)), ("RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(), BigDecimal::from(0)), ("RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(), BigDecimal::from(0.77699)), ("RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(), BigDecimal::from(0.99998)), ]; assert_eq!(actual, expected);

What do you think?

Looks okay. You can consider also the following:

fn test_native_display_balances() { let rpc_client = native_client_for_test(); - let expected: Vec<(Address, BigDecimal)> = vec![ - ("RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(), BigDecimal::from(5.77699)), - ("RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(), BigDecimal::from(0)), - ("RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(), BigDecimal::from(0.77699)), - ("RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(), BigDecimal::from(0.99998)), + let addresses: Vec<Address> = vec![ + "RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(), + "RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(), + "RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(), + "RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(), ]; - let addresses = expected.iter().map(|(address, _)| address.clone()).collect(); let actual = rpc_client - .display_balances(addresses, TEST_COIN_DECIMALS) + .display_balances(addresses.clone(), TEST_COIN_DECIMALS) .wait() .unwrap(); + let expected_balances: Vec<BigDecimal> = vec![ + BigDecimal::from(5.77699 as f64), + BigDecimal::from(0 as i32), + BigDecimal::from(0.77699 as f64), + BigDecimal::from(0.99998 as f64), + ]; + + assert_eq!(addresses.len(), expected_balances.len()); + + let mut expected: Vec<(Address, BigDecimal)> = Vec::new(); + addresses.iter().enumerate().for_each(|(index, address)| { + expected.push((*address, expected_balances[index])); + }); + assert_eq!(actual, expected); }

Isn't my solution more compact and easy to read? 😅

I just wanted to share a code flow from another aspect, all looks confirmable. The decision is up to you, I am fine with any of them :)

onur-ozkan · 2022-05-02T15:59:00Z

mm2src/coins/utxo/rpc_clients.rs

+            unspents
+                .into_iter()
+                .map(|hash_unspents| {
+                    let hash_unspents: Vec<_> = hash_unspents


Can be simplified as:

hash_unspents .into_iter() .unique_by(|unspent| (unspent.tx_hash, unspent.tx_pos)) .collect::<Vec<_>>()

Oooh right... Thanks)

* Move `address_balance_from_unspent_map` outside the `NativeClient::display_balance` function

* Avoid adding unnecessary variables * Add `inline` attribute where it's needed

sergeyboyko0791 · 2022-05-04T15:12:32Z

@artemii235 @ozkanonur @shamardy the PR is ready for re-review. Please check if I resolved all issues

shamardy

LGTM! Only one non-blocker comment about inline(always) #1255 (comment)

onur-ozkan

Looks good to me. Great work!

artemii235

One question and two changes.

mm2src/coins/rpc_command/account_balance.rs

mm2src/coins/utxo/utxo_common.rs

* Refactor `utxo_common::addresses_balances`

* Make `OutPoint` copiable

artemii235

👍

sergeyboyko0791 added 10 commits March 21, 2022 12:44

WIP Add JSONRPC batch requests

bb6f421

Add JsonRpcClient::send_batch_request

94af4fd

Add and implement UtxoRpcClientOps::display_balances

5d23039

* Process batch response in the same order in which the requests were sent

Fix wasm build

dd5a57c

* Add `utxo_common_tests.rs`

Move some coin's RPCs to separate modules

82beca0

* Move `account_balance`, `init_withdraw`, `init_create_account` RPCs from `coins` to `coins::rpc_command` * Use `RpcTask` for `init_scan_for_new_addresses`

Fix WASM build, ZCoin warnings

ff8400b

Merge branch 'dev' into utxo-rpc-batch

ee4d632

sergeyboyko0791 commented Apr 17, 2022

View reviewed changes

Minor changes, cover some functions with comments

36e7e48

sergeyboyko0791 changed the title ~~[wip] UTXO RPC batch requests~~ [r2r] UTXO RPC batch requests Apr 18, 2022

Merge branch 'dev' into utxo-rpc-batch

7571821

shamardy reviewed Apr 21, 2022

View reviewed changes

mm2src/coins/rpc_command/account_balance.rs Outdated Show resolved Hide resolved

mm2src/coins/utxo/rpc_clients.rs Show resolved Hide resolved

mm2src/coins/utxo/rpc_clients.rs Outdated Show resolved Hide resolved

artemii235 suggested changes Apr 22, 2022

View reviewed changes

sergeyboyko0791 added 2 commits April 25, 2022 10:40

Fix PR issues

f7f2dc2

* Do not inherit `UtxoCommonOps` from `ListUtxoOps` (due to `ZCoin` support) * Refactor some of `UtxoRpcClientOps`, `utxo_common` methods by getting rid of nesting * Add the `common::custom_iter::CollectInto` trait * Add `ListUtxoOps::get_mature_unspent_ordered_list`

Minor change

987cc1e

* Reorder RPCs alphabetically

sergeyboyko0791 changed the title ~~[r2r] UTXO RPC batch requests~~ [wip] UTXO RPC batch requests Apr 26, 2022

sergeyboyko0791 added 5 commits April 26, 2022 22:45

Fix PR issues

013e214

* Fix `from_id` paging option for `account_balance` RPC * Fix WASM build

Merge branch 'dev' into utxo-rpc-batch

3ecf7a3

Fix PR issues

abe381d

* Divide `ListUtxoOps` into `GetUtxoListOps` and `GetUtxoMapOps` * Implement `GetUtxoMapOps` for `UtxoStandardCoin` and `QtumCoin` only

Fix tests compilation error

73e9daf

Merge branch 'dev' into utxo-rpc-batch

787b3b6

sergeyboyko0791 mentioned this pull request Apr 29, 2022

Refactor coins::utxo::output_script #1270

Open

sergeyboyko0791 added 3 commits May 1, 2022 13:16

Add UtxoVerboseCache to avoid locking global mutex

b852984

Add DummyVerboseCache that is used within tests

b0be26e

* Rename `UtxoVerboseCache` to `FsVerboseCache` * Add `WasmVerboseCache` that is an alias to `DummyVerboseCache`

Minor changes

64d2cc3

artemii235 mentioned this pull request May 2, 2022

Divide existing traits to smaller ones to avoid implementing unnecessary functionality. #1274

Open

sergeyboyko0791 changed the title ~~[wip] UTXO RPC batch requests~~ [r2r] UTXO RPC batch requests May 2, 2022

artemii235 suggested changes May 2, 2022

View reviewed changes

Fix PR issues

d9004d6

* Refactor `BchRequest` trait into `JsonRpcBatchClient` * Move `send_batch_request` from `JsonRpcClient` to `JsonRpcBatchClient` * Refactor `BchCoin::utxos_into_bch_unspents`

onur-ozkan reviewed May 2, 2022

View reviewed changes

Fix PR issues

7224b85

* Move `address_balance_from_unspent_map` outside the `NativeClient::display_balance` function

sergeyboyko0791 changed the title ~~[r2r] UTXO RPC batch requests~~ [wip] UTXO RPC batch requests May 3, 2022

sergeyboyko0791 added 4 commits May 4, 2022 17:10

Fix PR issues

537a3f2

* Avoid adding unnecessary variables * Add `inline` attribute where it's needed

Minor changes within utxo_tests

0aaefda

Minor changes within accoutn_balance_rpc

7ac3a6e

Merge branch 'dev' into utxo-rpc-batch

bc89772

sergeyboyko0791 changed the title ~~[wip] UTXO RPC batch requests~~ [r2r] UTXO RPC batch requests May 4, 2022

shamardy previously approved these changes May 5, 2022

View reviewed changes

Avoid using #[inline(alaways)] where it's not necessary

5a70b25

sergeyboyko0791 dismissed shamardy’s stale review via 5a70b25 May 6, 2022 09:40

onur-ozkan previously approved these changes May 9, 2022

View reviewed changes

Merge branch 'dev' into utxo-rpc-batch

f5c635e

sergeyboyko0791 dismissed onur-ozkan’s stale review via f5c635e May 9, 2022 10:22

artemii235 suggested changes May 9, 2022

View reviewed changes

mm2src/coins/rpc_command/account_balance.rs Show resolved Hide resolved

mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved

mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved

sergeyboyko0791 added 2 commits May 10, 2022 10:38

Minor changes

996f590

* Refactor `utxo_common::addresses_balances`

Minor changes

798f14d

* Make `OutPoint` copiable

artemii235 approved these changes May 10, 2022

View reviewed changes

artemii235 merged commit b434af8 into dev May 10, 2022

artemii235 deleted the utxo-rpc-batch branch May 10, 2022 13:40

sergeyboyko0791 mentioned this pull request Jul 9, 2022

[r2r] update metrics related dep && refactoring #1312

Merged

onur-ozkan mentioned this pull request Sep 6, 2023

feat(nft): exclude spam and phishing, refactored db #1959

Merged

6 tasks


		let rpc_client = native_client_for_test();

		let expected: Vec<(Address, BigDecimal)> = vec![

[r2r] UTXO RPC batch requests #1255

[r2r] UTXO RPC batch requests #1255

Uh oh!

Conversation

sergeyboyko0791 commented Apr 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeyboyko0791 May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergeyboyko0791 commented May 2, 2022

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeyboyko0791 May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeyboyko0791 May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sergeyboyko0791 May 1, 2022 •

edited

Loading

sergeyboyko0791 May 2, 2022 •

edited

Loading

sergeyboyko0791 May 3, 2022 •

edited

Loading

onur-ozkan May 3, 2022 •

edited

Loading