Enhance enable with tokens methods

[shamardy]

fixes #1748

• This PR introduces several enhancements to the enable_bch_with_tokens, enable_eth_with_tokens, and enable_tendermint_with_assets requests.
• Firstly, token balance requests are now performed concurrently, which is expected to resolve any timeout issues that were occurring in GUIs if a shorter timeout is selected.
• Additionally, a new parameter called get_balances has been added to these requests. When set to false, balances and addresses are not requested or returned in the response. This option can be useful for users who prefer to poll my_balance for balances after activation, as it can speed up coin activations.
• Please note that the default value of get_balances is true, meaning that if this parameter is not set, the previous behavior will be maintained.

[shamardy]

Because of how enable_tendermint_with_assets ActivationResult is set, I return balances as 0 if get_balances is false.

I didn't want to make the below 2 params optional until I get your opinion on this @ozkanonur https://github.com/KomodoPlatform/atomicDEX-API/blob/4bede306a9f4bc52a5121cf0f39bd60aa3a716f1/mm2src/coins_activation/src/tendermint_with_assets_activation.rs#L130-L131

[onur-ozkan]

I think it makes more sense to make those balance fields null on the response for every coin type to have consistent response type which is also easier way to implement this endpoints for UI devs. So they will be 100% sure if these fields were null, they didnt enable the balance on requests.

[onur-ozkan]

LGTM.

I have 2 suggestions,

• early return on balance check for less nested code like:

if !get_balance {
   return Ok(result);
}

// calculate the balances

Ok(result)

• integration tests for get_balance behaviour

[shamardy]

A few notes:

• get_balances can't be false when tx_history is true/enabled since tx history fetching requires getting balance on activation to check for balance changes and to start fetching transactions if there was a change. If the user sets tx_history: true, and get_balances: false an error will be returned on activation. Alternatively, I can fetch balances as a first step of getting tx history if they were not fetched on activation, but I thought since it will be fetched anyways including it in the response would be the right approach. What do you think about this?
• Addresses are not returned when get_balances is false too (except for tendermint since the response is different), this is because balances are returned as a map address -> balance, I can return addresses as a hashset/vec when get_balances: false instead if anybody think this will be beneficial.
• @ozkanonur for tendermint, it seems that balance get fetched at the start of tendermint_history_loop https://github.com/KomodoPlatform/atomicDEX-API/blob/433474618a42b67a3a371d550b309dc88243b7ef/mm2src/coins/tendermint/tendermint_tx_history_v2.rs#L882 is there a reason for this? should I pass it from activation result instead?

cc @smk762 @cipig

[onur-ozkan]

Addresses are not returned when get_balances is false too (except for tendermint since the response is different)

This is because addresses do not change for tokens on cosmos. I think this should be same for other chains if the address behaviour as same as cosmos.

@ozkanonur for tendermint, it seems that balance get fetched at the start of tendermint_history_loop

Can't recall any reason. It's been long time, maybe some stuff even changed. You can remove ctx and current_balance params and pass the balances.

--

edit: just checked the source code, maybe the reason was calling async function inside of the async function rather than executing in non-async one.

[shamardy]

Can't recall any reason. It's been long time, maybe some stuff even changed. You can remove ctx and current_balance params and pass the balances.

--

edit: just checked the source code, maybe the reason was calling async function inside of the async function rather than executing in non-async one.

It seems to be related to start_history_background_fetching passing only the platform balance as a BigDecimal
https://github.com/KomodoPlatform/atomicDEX-API/blob/150d928d2aa6bfefa9400ce257a491d115c98a5c/mm2src/coins_activation/src/platform_coin_with_tokens.rs#L326-L330
While tendermint tx history uses all balances (platform and tokens) to detect balance changes https://github.com/KomodoPlatform/atomicDEX-API/blob/433474618a42b67a3a371d550b309dc88243b7ef/mm2src/coins/tendermint/tendermint_tx_history_v2.rs#L104

This is because addresses do not change for tokens on cosmos. I think this should be same for other chains if the address behaviour as same as cosmos.

Address is different only for BCH since SLP tokens have a different prefix than BCH but it's only one address for all SLP tokens. So for BCH with tokens we have 2 addresses, one for BCH and one for Tokens and they are really the same address and only differ in how they are displayed.

At first, I thought this had to do with HD wallets, but it seems for HD wallets, only one address is initialized with the enable calls and other methods/ways are used to switch between addresses and get balances of all address.

I also think address should be separated from balances in the response as you said @ozkanonur, but this may be used in the GUIs, and I wanted to maintain compatibility. What do you think about this @cipig @smk762?

[onur-ozkan]

but this may be used in the GUIs, and I wanted to maintain compatibility.

right, I guess this will block this PR for some long time since a lot of testing/changes needs to be made from GUI devs.

[smk762]

balances are returned as a map address -> balance

Could we retain this with null for balance when set to false on activation?

[shamardy]

Could we retain this with null for balance when set to false on activation?

Yes, I think this can be done. But I will not be able to skip serializing for balances so the field will be returned as null. I can also return a hashset/vec with only the address/addresses if balances are not requested as I said in this comment #1762 (comment), but I guess null is better so that that the same methods in GUIs that parse these fields can be used.

[smk762]

cc: @yurii-khi ^

[rozhkovdmitrii]

enable_bch_with_tokens_without_balance must be moved to slp_tests.rs, just as enable_eth_with_tokens_without_balance is properly placed in eth_test.rs

[shamardy]

I placed it under enable_bch_with_tokens so it can be used in bch_and_slp_tests if needed. https://github.com/KomodoPlatform/atomicDEX-API/blob/c5e8fe1cd2a2450813d732010d56b7e9405c9fab/mm2src/mm2_test_helpers/src/for_tests.rs#L1705 mm2_test_helpers is a common crate for test functions/structs/helpers, maybe I should move enable_eth_with_tokens_without_balance and enable_eth_with_tokens there instead. What do you think? The idea is that functions that wrap requests should be accessible in tests anywhere if needed.

[rozhkovdmitrii]

You are right, both must reside there in slp_tests.rs

[shamardy]

You are right, both must reside there in slp_tests.rs

Sorry, I didn't understand. Are you suggesting moving enable_eth_with_tokens_without_balance and enable_eth_with_tokens to mm2_test_helpers or moving enable_bch_with_tokens_without_balance to slp_tests.rs?

[rozhkovdmitrii]

enable_bch_with_tokens_without_balance -> slp_tests.rs
enable_tendermint_without_balance -> tendermint_tests.rs

[shamardy]

done

[rozhkovdmitrii]

It also would make a code much more clean if you put enable_bch_with_tokens_without_balance right after the only place where it's called like that:

[shamardy]

It also would make a code much more clean if you put enable_bch_with_tokens_without_balance right after the only place where it's called like that:

It's considered good practice for test utility functions such as enable_bch_with_tokens_without_balance to be put at the beginning of the test module before the actual unit test functions, this is also the convention followed through out the project. This improves code readability where you see all the functions that can be reused at the start of the file.

[rozhkovdmitrii]

I would not suggest if it's used somewhere else in fact and then would not be considered as part of a certain test

[shamardy]

It can easily be used in other tests in the future since this function's cost is lower with no calls for balances if they are not needed, that's why a clear structure from the start is better. Also, if you check other popular rust repos and popular rust books examples, they all do it this way.
An alternative idiomatic way is to have the function nested inside the unit test if it's used only once, but for rpc calls, from past experience I can see that they get used multiple times later.

[shamardy]

@rozhkovdmitrii suggested in DM that this is the ideal response when get_balances: false and I agree with him, I will now look at if it can be done in the code easily or not

cc: @smk762 @yurii-khi

[onur-ozkan]

thanks for the great effort

[shamardy]

@rozhkovdmitrii I fixed the response to be like how you suggested here #1762 (comment)

[rozhkovdmitrii]

LGTM, thank you for a great work!

[shamardy]

@smk762 or @cipig can you please test that this works as intended?

[cipig]

tried enable_eth_with_tokens on MATIC with a bunch of tokens and get_balances: false
works fine and it's almost instant
this is the output:

{"mmrpc":"2.0","result":{"current_block":42064394,"eth_addresses_infos":{"0xadb681c3A1ec9BbC4105B8E8EB5fC7178125b450":{"derivation_method":{"type":"Iguana"},"pubkey":"04de96cb66dcfaceaa8b3d4993ce8914cd5fe84e3fd53cefdae45add8032792a12255d74dc8e38f2d06ff73964a7514cfce5c3057c814bbfe7c4cef42723a516c9"}},"erc20_addresses_infos":{"0xadb681c3A1ec9BbC4105B8E8EB5fC7178125b450":{"derivation_method":{"type":"Iguana"},"pubkey":"04de96cb66dcfaceaa8b3d4993ce8914cd5fe84e3fd53cefdae45add8032792a12255d74dc8e38f2d06ff73964a7514cfce5c3057c814bbfe7c4cef42723a516c9"}}},"id":null}

it lacks coin names, but it's fine for me, on CLI with trading bot

[smk762]

Can you please clarify the decided approach and expected response when "get_balances": false, and "tx_history":true?
And should this apply to tendermint_with_assets too?

[shamardy]

it lacks coin names, but it's fine for me, on CLI with trading bot

That's not ideal. Will try to fix that if I can.

Can you please clarify the decided approach and expected response when "get_balances": false, and "tx_history":true?
And should this apply to tendermint_with_assets too?

For all the methods tendermint or not, it works as expected now, meaning no error will be returned and tx_history will be enabled while the balances are not returned. Activation should take no time when "get_balances":false too as expected, independent of tx_history, since there will be no balance requests during activation.

[smk762]

Balance display is correctly applied when get_balance is true or false on tendermint, erc, and slp methods, regardless of tx_history value.

Confirm here also that activated tokens are not part of the output when get_balance is false.

[shamardy]

@smk762 @cipig tickers are now returned if get_balances:false. Can you please do a last check that everything works fine?

@ozkanonur can you please check the latest changes related to tendermint?

[cipig]

the tickers are now shown, like this:

{"mmrpc":"2.0","result":{"current_block":42209419,"eth_addresses_infos":{"0xadb681c3A1ec9BbC4105B8E8EB5fC7178125b450":{"derivation_method":{"type":"Iguana"},"pubkey":"04de96cb66dcfaceaa8b3d4993ce8914cd5fe84e3fd53cefdae45add8032792a12255d74dc8e38f2d06ff73964a7514cfce5c3057c814bbfe7c4cef42723a516c9"}},"erc20_addresses_infos":{"0xadb681c3A1ec9BbC4105B8E8EB5fC7178125b450":{"derivation_method":{"type":"Iguana"},"pubkey":"04de96cb66dcfaceaa8b3d4993ce8914cd5fe84e3fd53cefdae45add8032792a12255d74dc8e38f2d06ff73964a7514cfce5c3057c814bbfe7c4cef42723a516c9","tickers":["UNI-PLG20","AAVE-PLG20","COMP-PLG20","JMXN-PLG20","REQ-PLG20","PGX-PLG20","USDC-PLG20","EUROE-PLG20","SUSHI-PLG20","OCEAN-PLG20","JAUD-PLG20","JTRY-PLG20","JBRL-PLG20","JSEK-PLG20","WOO-PLG20","JPLN-PLG20","ATOM-PLG20","JUSD-PLG20","LINK-PLG20","XIDR-PLG20","SAND-PLG20","NZDS-PLG20","TUSD-PLG20","CRV-PLG20","FXS-PLG20","USDT-PLG20","LDO-PLG20","GRT-PLG20","CADC-PLG20","JCNY-PLG20","SNX-PLG20","HEX-PLG20","BRZ-PLG20","JJPY-PLG20","YFI-PLG20","ETH-PLG20","APE-PLG20","JGBP-PLG20","JNZD-PLG20","SOL-PLG20","GLM-PLG20","JEUR-PLG20","JGOLD-PLG20","XSGD-PLG20","AGEUR-PLG20","KNC-PLG20","1INCH-PLG20","JSGD-PLG20","ANKR-PLG20","PAXG-PLG20","ILN-PLG20","RNDR-PLG20","JPYC-PLG20","BAL-PLG20","MANA-PLG20","TEL-PLG20","JCAD-PLG20","MASK-PLG20","JCHF-PLG20","GNS-PLG20","EURS-PLG20","JKRW-PLG20","DAI-PLG20","WBTC-PLG20","TRYB-PLG20"]}}},"id":null}

[onur-ozkan]

@ozkanonur can you please check the latest changes related to tendermint?

they look fine