Enhance enable with tokens methods by shamardy · Pull Request #1762 · GLEECBTC/komodo-defi-framework

shamardy · 2023-04-21T22:32:14Z

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.

…s balances from rpcs concurrently

shamardy · 2023-04-24T18:42:07Z

mm2src/coins_activation/src/tendermint_with_assets_activation.rs

+                    .collect(),
+            )
+        } else {
+            (CoinBalance::default(), HashMap::default())


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

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

…eck, integration tests for get_balances

shamardy · 2023-04-25T17:40:03Z

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 · 2023-04-25T21:03:00Z

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 · 2023-04-26T14:26:01Z

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 · 2023-04-26T14:31:24Z

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 · 2023-04-26T14:32:32Z

balances are returned as a map address -> balance

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

shamardy · 2023-04-26T14:37:39Z

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.

mm2src/coins_activation/src/platform_coin_with_tokens.rs

smk762 · 2023-04-26T15:00:56Z

cc: @yurii-khi ^

…alances is false

mm2src/mm2_main/tests/mm2_tests/eth_tests.rs

mm2src/mm2_test_helpers/src/structs.rs

rozhkovdmitrii · 2023-04-26T14:57:25Z

mm2src/mm2_test_helpers/src/for_tests.rs

    json::from_str(&enable.1).unwrap()
 }

+pub async fn enable_bch_with_tokens_without_balance(


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

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.

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

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?

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

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 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.

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

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.

mm2src/mm2_test_helpers/src/structs.rs

shamardy · 2023-04-26T18:28:26Z

@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

…m response if get_balances is false

shamardy · 2023-04-26T22:31:12Z

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

mm2src/coins/eth.rs

…to be used in the loop without cloning

mm2src/coins/eth.rs

mm2src/coins/solana.rs

rozhkovdmitrii

LGTM, thank you for a great work!

shamardy · 2023-04-28T15:49:55Z

@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 · 2023-04-28T16:44:57Z

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 · 2023-04-28T22:29:37Z

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 · 2023-05-01T13:41:42Z

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 · 2023-05-01T19:56:27Z

@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

add get_balances param to enable with tokens methods, request token…

4bede30

…s balances from rpcs concurrently

shamardy commented Apr 24, 2023

View reviewed changes

shamardy marked this pull request as ready for review April 24, 2023 18:42

shamardy requested review from cipig, onur-ozkan, rozhkovdmitrii and smk762 April 24, 2023 18:42

onur-ozkan added the under review label Apr 25, 2023

onur-ozkan previously approved these changes Apr 25, 2023

View reviewed changes

make balance reponse fields optional, early return on get_balances ch…

150d928

…eck, integration tests for get_balances

shamardy dismissed onur-ozkan’s stale review via 150d928 April 25, 2023 17:28

onur-ozkan reviewed Apr 26, 2023

View reviewed changes

mm2src/coins_activation/src/platform_coin_with_tokens.rs Outdated Show resolved Hide resolved

shamardy added 2 commits April 26, 2023 18:49

request bch balance at the start of bch_and_slp_history_loop if get_b…

19fd34e

…alances is false

Merge remote-tracking branch 'origin/dev' into fix-enhance-with-tokens

c5e8fe1

rozhkovdmitrii reviewed Apr 26, 2023

View reviewed changes

fix solana compilation

d8da3c4

onur-ozkan previously approved these changes Apr 26, 2023

View reviewed changes

fix enable with tokens responses to remove only the balance field fro…

abfe1be

…m response if get_balances is false

shamardy dismissed onur-ozkan’s stale review via abfe1be April 26, 2023 22:29

rozhkovdmitrii reviewed Apr 27, 2023

View reviewed changes

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

capture self of get_tokens_balance_list by reference using a closure …

445d3b6

…to be used in the loop without cloning

rozhkovdmitrii reviewed Apr 28, 2023

View reviewed changes

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

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

rozhkovdmitrii reviewed Apr 28, 2023

View reviewed changes

mm2src/coins/solana.rs Show resolved Hide resolved

make get_tokens_balance_list code better

914e956

rozhkovdmitrii previously approved these changes Apr 28, 2023

View reviewed changes

shamardy added 2 commits April 28, 2023 18:39

Merge remote-tracking branch 'origin/dev' into fix-enhance-with-tokens

d7812fd

add enhance enable with tokens methods changelog

8dfbed1

shamardy dismissed rozhkovdmitrii’s stale review via 8dfbed1 April 28, 2023 15:48

cipig previously approved these changes Apr 28, 2023

View reviewed changes

shamardy added 2 commits May 1, 2023 19:18

Merge remote-tracking branch 'origin/dev' into fix-enhance-with-tokens

3864170

return token tickers if get_balances is false

6444336

shamardy dismissed cipig’s stale review via 6444336 May 1, 2023 19:25

shamardy requested review from cipig and onur-ozkan May 1, 2023 19:54

cipig previously approved these changes May 2, 2023

View reviewed changes

onur-ozkan previously approved these changes May 2, 2023

View reviewed changes

smk762 previously approved these changes May 2, 2023

View reviewed changes

update CHANGELOG.md

c308987

shamardy dismissed stale reviews from smk762, onur-ozkan, and cipig via c308987 May 2, 2023 11:16

shamardy merged commit e041dbb into dev May 2, 2023

shamardy deleted the fix-enhance-with-tokens branch May 2, 2023 11:20

shamardy mentioned this pull request May 5, 2023

[release] v1.0.4-beta #1804

Merged

shamardy added 1.0.4-beta and removed under review labels May 5, 2023

Conversation

shamardy commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

shamardy commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onur-ozkan commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented Apr 26, 2023

Uh oh!

onur-ozkan commented Apr 26, 2023

Uh oh!

smk762 commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

smk762 commented Apr 26, 2023

Uh oh!

Uh oh!

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!

shamardy Apr 28, 2023 • 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!

Uh oh!

shamardy commented Apr 26, 2023

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy commented Apr 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rozhkovdmitrii left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy commented Apr 28, 2023

Uh oh!

cipig left a comment

Choose a reason for hiding this comment

Uh oh!

smk762 commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented Apr 28, 2023

Uh oh!

smk762 commented May 1, 2023

shamardy commented Apr 21, 2023 •

edited

Loading

shamardy commented Apr 25, 2023 •

edited

Loading

onur-ozkan commented Apr 25, 2023 •

edited

Loading

smk762 commented Apr 26, 2023 •

edited

Loading

shamardy commented Apr 26, 2023 •

edited

Loading

shamardy Apr 28, 2023 •

edited

Loading

smk762 commented Apr 28, 2023 •

edited

Loading

shamardy commented May 1, 2023 •

edited

Loading