feat(tokens): custom token activation for evm #2141

shamardy · 2024-06-12T14:43:12Z

Todo:

Next PR:

Custom token config storage and deletion. Deletion should be when token is deactivated.
- An RPC to get previously enabled custom tokens

To Test (WIP):

Custom token required_confirmations should equal to platform's if not set in activation request.

mariocynicys

First review iteration

mm2src/coins_activation/src/l2/init_l2_error.rs

mm2src/coins_activation/src/init_token.rs

onur-ozkan · 2024-06-24T07:13:20Z

mm2src/coins/tendermint/tendermint_token.rs

 pub struct TendermintTokenProtocolInfo {
    pub platform: String,
+    // Todo: can decimals be gotten from rpc call for custom tokens?


There is denoms_metadata endpoint available on tendermint, but I am not sure if it works with the custom tokens.

Thanks for the hint, will give it a look.

mm2src/coins_activation/src/init_token.rs

mm2src/coins_activation/src/prelude.rs

mm2src/coins_activation/src/token.rs

mm2src/coins_activation/src/prelude.rs

… `token_conf` parameter instead

…functions * `get_custom_token_info` RPC now returns `config_ticker` when token exists in config. This allows GUI to detect tokens in config and use them for activation instead of the token being wallet-only. * Move new ERC20 functions to erc20.rs for better organization

…nUse`

Also split lp_commands.rs file to multiple files

…add test coverage for `get_custom_token_info` RPC in it

…duplicate contract in the coins config

…t is in coins config

…s another ticker enabled

…gside other renames

- rename `has_conf` to `has_matching_ticker` - rename `CustomTokenInfo` to `TokenInfo` in tests

mariocynicys

LGTM. Just a couple of nits and Qs.

mm2src/coins/lp_coins.rs

mm2src/coins_activation/src/init_token.rs

mm2src/coins/eth/erc20.rs

mariocynicys · 2024-11-08T18:22:58Z

mm2src/mm2_main/src/rpc/lp_commands/custom_token.rs

imo though, splitting an RPC per file is more maintainable/easier to handle.

mm2src/coins/lightning/ln_conf.rs

mm2src/coins/lp_coins.rs

mm2src/coins/tendermint/tendermint_coin.rs

mm2src/mm2_test_helpers/src/structs.rs

…d structs

shamardy · 2024-11-11T14:05:35Z

@mariocynicys all the nits are fixed and Qs answered. I will merge this PR once CI passes and docs issue is open.

onur-ozkan

Q: Did you think about populating coins file for custom/unsupported coins? For example, we could provide an RPC interface where people can add any custom coin to our coins file and that eventually would allow them to enable and use that coin in KDF. Wouldn't that be much simpler to handle and maintain in KDF? And that would work fine for any of our supported protocols (e.g., cosmos, evm, etc.).

onur-ozkan · 2024-11-12T08:17:30Z

mm2src/coins/eth/erc20.rs

+pub(crate) async fn get_token_decimals(web3: &Web3<Web3Transport>, token_addr: Address) -> Result<u8, String> {
+    let tokens = call_erc20_function(web3, token_addr, "decimals").await?;
+    match tokens[0] {
+        Token::Uint(dec) => Ok(dec.as_u64() as u8),
+        _ => ERR!("Invalid decimals type {:?}", tokens),
+    }
+}
+
+async fn get_token_symbol(coin: &EthCoin, token_addr: Address) -> Result<String, String> {
+    let web3 = try_s!(coin.web3().await);
+    let tokens = call_erc20_function(&web3, token_addr, "symbol").await?;
+    match &tokens[0] {
+        Token::String(symbol) => Ok(symbol.clone()),
+        _ => ERR!("Invalid symbol type {:?}", tokens),
+    }
+}


I wouldn't try to access index 0 directly as it would panic when tokens is empty.

unfortunately, we seem to have these all over the eth code (e.g. \[\d\] in eth_taker_swap_v2.rs), and we consider this safe for some reason :/.
i verified how such vecs come and their sizes in a previous PR, but i would still be on the safe side and not have them addressed like that. also reducing tragic effects from dev errors.

I wouldn't try to access index 0 directly as it would panic when tokens is empty.

Missed this when copying the code. Will fix only the instances in this PR and fix all others in another PR.

Fixed here f38fc3f

mm2src/coins_activation/src/prelude.rs

mm2src/mm2_main/src/rpc/lp_commands/tokens.rs

shamardy · 2024-11-13T15:30:27Z

Q: Did you think about populating coins file for custom/unsupported coins? For example, we could provide an RPC interface where people can add any custom coin to our coins file and that eventually would allow them to enable and use that coin in KDF. Wouldn't that be much simpler to handle and maintain in KDF? And that would work fine for any of our supported protocols (e.g., cosmos, evm, etc.).

I did consider this and can be part of the next PR mentioned here #2141 (comment)

Next PR:

Custom token config storage and deletion. Deletion should be when token is deactivated.

An RPC to get previously enabled custom tokens

But I think separating them is better so that updating the coins file is simple since it will remain static.

…and `get_token_symbol`

borngraced

Great work! some notes

mm2src/coins/eth/erc20.rs

onur-ozkan · 2024-11-14T06:09:17Z

Q: Did you think about populating coins file for custom/unsupported coins? For example, we could provide an RPC interface where people can add any custom coin to our coins file and that eventually would allow them to enable and use that coin in KDF. Wouldn't that be much simpler to handle and maintain in KDF? And that would work fine for any of our supported protocols (e.g., cosmos, evm, etc.).

I did consider this and can be part of the next PR mentioned here #2141 (comment)

Next PR:

Custom token config storage and deletion. Deletion should be when token is deactivated.

An RPC to get previously enabled custom tokens

But I think separating them is better so that updating the coins file is simple since it will remain static.

I would have consider this seriously before making further progress I guess. For example, this PR is EVM only and already adds ~1k diff on the codebase and it will increase even more once we start supporting other protocols and adding a support for custom coin. All that would be dead simple with adding an RPC function for populating the coins file. If we don't want to make modifications on the coins file, I guess that's fine; we could save them in-memory (the value where we search/find coins from) without modifying the actual file on FS. Is there any big advantage that current approach brings compare to what I just suggested?

shamardy · 2024-11-14T12:18:20Z

I would have consider this seriously before making further progress I guess. For example, this PR is EVM only and already adds ~1k diff on the codebase and it will increase even more once we start supporting other protocols and adding a support for custom coin.

If we didn't count refactors and tests, it's only ~500 diff, the token info RPC code will be reusable across protocols as well and it's absolutely needed for GUI ref. #2141 (comment), same for CustomTokenError,impl CoinProtocol,fn create_custom_token_config. Implementations are done for the generic init_token function and for any other token protocol we will add the protocol specific implementations only and the rest should work.

All that would be dead simple with adding an RPC function for populating the coins file. If we don't want to make modifications on the coins file, I guess that's fine; we could save them in-memory (the value where we search/find coins from) without modifying the actual file on FS.

The custom config need to be persisted across restarts, which leaves us with two options: modifying the coins file or creating dedicated storage/DB for custom tokens. The process requires more than just adding an RPC function for coins file population - we need comprehensive validation for duplicate tickers, contracts, and other potential conflicts. Most of the validation logic will be necessary regardless of approach. We will also need to make coins config a struct and not just json to deserialize it probably, I reused the CoinProtocol to make it a similar approach as part of the coin config structure is used in the RPC which is an approach that is different but not by too much.

I am not against changing the whole approach if it's clear how it will be implemented, but I can't be sure that using the coins file will not bring more complications of it's own that we didn't anticipate.

onur-ozkan · 2024-11-14T12:42:12Z

I would have consider this seriously before making further progress I guess. For example, this PR is EVM only and already adds ~1k diff on the codebase and it will increase even more once we start supporting other protocols and adding a support for custom coin.

If we didn't count refactors and tests, it's only ~500 diff, the token info RPC code will be reusable across protocols as well and it's absolutely needed for GUI ref. #2141 (comment), same for CustomTokenError,impl CoinProtocol,fn create_custom_token_config. Implementations are done for the generic init_token function and for any other token protocol we will add the protocol specific implementations only and the rest should work.

All that would be dead simple with adding an RPC function for populating the coins file. If we don't want to make modifications on the coins file, I guess that's fine; we could save them in-memory (the value where we search/find coins from) without modifying the actual file on FS.

The custom config need to be persisted across restarts, which leaves us with two options: modifying the coins file or creating dedicated storage/DB for custom tokens. The process requires more than just adding an RPC function for coins file population - we need comprehensive validation for duplicate tickers, contracts, and other potential conflicts. Most of the validation logic will be necessary regardless of approach. We will also need to make coins config a struct and not just json to deserialize it probably, I reused the CoinProtocol to make it a similar approach as part of the coin config structure is used in the RPC which is an approach that is different but not by too much.

I am not against changing the whole approach if it's clear how it will be implemented, but I can't be sure that using the coins file will not bring more complications of it's own that we didn't anticipate.

I am suggesting that because in the PR it says "for EVM", and in one of the notes there is a comment says something like "we support tokens only for now, not coins". Because of that, I am suggesting that we can handle every coin, token and protocol in a same way we do with coins file, so we don't need to worry about type of the protocol/coin/token. If it needs to be persistent, we could create another file next to coins file called custom_coins and populate that one. As for the validations, I think they should be done on coins file too. IMO there shouldn't be any difference on parsing and validation custom and original coins values.

shamardy · 2024-11-14T13:13:38Z

and in one of the notes there is a comment says something like "we support tokens only for now, not coins"

At the current stage, adding custom coins should never be supported as it can be dangerous and shouldn't be allowed to be added by the normal user. @cipig does a lot of checks and tests before adding a coin to coins config repo. It's also the reason why custom tokens will be added as wallet only since there are a lot of non standard tokens that will not work right with swaps.

Because of that, I am suggesting that we can handle every coin, token and protocol in a same way we do with coins file, so we don't need to worry about type of the protocol/coin/token.

I am not against that ref. #2141 (comment)

if it needs to be persistent, we could create another file next to coins file called custom_coins and populate that one.

This will be done in the next PR maybe with the refactors for coin config structs. Should we use a file or db, we have to use db for wasm though.

As for the validations, I think they should be done on coins file too. IMO there shouldn't be any difference on parsing and validation custom and original coins values.

Agreed and I actually thought of this :)

As I said, adding custom coins is dangerous but we should add some validations such as (for utxo):

1 - Transactions/blocks deserializations
2 - Fee validation from a transaction fetched from chain
3 - Validation of address formats and encodings
4- Transaction signing validation, etc..

This is to make sure the coin will work fine for all wallet functionalities, it will also require adding the RPC urls by the user. For swaps, it's a different thing and will require more thinking about.

Can I do this in a later PR? right now EVM custom tokens are needed to be released with 1inch integration so that users can swap any token, wallet only means not working for our cross chain swaps only but will be allowed to work with same chain dexes.

dimxy · 2024-11-15T09:09:04Z

Q: Did you think about populating coins file for custom/unsupported coins? For example, we could provide an RPC

I think it's better to store customs tokens in a separate file/db (and retain the original coins file intact).
So if installation gets corrupted the user can reinstall the app to repair (including the original coins file).
I think reinstallation should preserve the custom coins data but users should also have a option to clear app's data to restore to the original state (there is also a question if the customs coins format has changed - we need to provide its upgrade on reinstall)

custom token activation as wallet only for evm and tendermint

c7638a5

shamardy added the in progress Changes will be made from the author label Jun 12, 2024

mariocynicys reviewed Jun 18, 2024

View reviewed changes

mm2src/coins_activation/src/l2/init_l2_error.rs Outdated Show resolved Hide resolved

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

onur-ozkan reviewed Jun 24, 2024

View reviewed changes

dimxy reviewed Jul 2, 2024

View reviewed changes

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

shamardy added 16 commits August 2, 2024 03:50

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

228b862

fix typo in ProtocolMissMatch

7e95c7f

review fix: use ok_or_else instead of matching protocol_from_request

8c489cf

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

3038aaa

Remove getting coin_conf inside initialize_erc20_token and pass a…

2ae59f0

… `token_conf` parameter instead

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

b0bdc82

wip: make custom token work for EVM only and add some validation steps

126570a

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

37b4531

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

545b87e

default required_confirmations should be from platform coin

8238ff8

wip: add get_custom_token_info RPC

12a9461

use CoinProtocol in CustomTokenInfoRequest

c888f8a

remove some todos that are not relevant anymore to the PR

245c106

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

b7c37ee

Rename EnableTokenError::TokenIsAlreadyActivated to `TickerAlreadyI…

5fb0bdd

…nUse`

shamardy force-pushed the feat-custom-evm-token branch from 5879540 to 5fb0bdd Compare October 28, 2024 21:17

shamardy added 8 commits October 29, 2024 00:41

move custom_token.rs to lp_commands

f676212

Also split lp_commands.rs file to multiple files

Add test for enabling and disabling custom erc20 tokens

f05b672

Make a coin not in config as wallet only and add test for it

eef94b2

rename test_enable_disable_custom_erc20 to test_custom_erc20 and …

c62f4a7

…add test coverage for `get_custom_token_info` RPC in it

Add test to verify error handling for enabling a custom token with a …

551b6a7

…duplicate contract in the coins config

Add test coverage for get_custom_token_info RPC if the same contrac…

a8396b0

…t is in coins config

Add test coverage for enabling a custom token with a contract that ha…

d2a678f

…s another ticker enabled

move new custom erc20 tests to eth_docker_tests.rs

05949db

shamardy added 5 commits November 5, 2024 00:07

review fixes: move coin conf inside TokenActivationParams

c721a50

review fixes: refactor coin_conf_with_protocol

bb2146f

review fixes: rename get_custom_token_info to get_token_info alon…

1c07703

…gside other renames

- fix coin_conf_with_protocol

d647242

- rename `has_conf` to `has_matching_ticker` - rename `CustomTokenInfo` to `TokenInfo` in tests

review fix: remove is_custom from requests

e9b6dc7

shamardy mentioned this pull request Nov 7, 2024

feat(LRAPI): add 1inch classic swap rpc #2222

Open

3 tasks

mariocynicys previously approved these changes Nov 8, 2024

View reviewed changes

review fixes: remove redundant PartialEqs from CoinProtocol neste…

a362e5c

…d structs

shamardy dismissed mariocynicys’s stale review via a362e5c November 11, 2024 13:32

shamardy added 3 commits November 11, 2024 15:43

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

2ff5448

review fix: add display to all variants of CustomTokenError

36e28b5

review fix: rename Erc20TokenBasicInfo to Erc20TokenInfo

84847ed

mariocynicys previously approved these changes Nov 11, 2024

View reviewed changes

onur-ozkan reviewed Nov 12, 2024

View reviewed changes

dimxy reviewed Nov 12, 2024

View reviewed changes

mm2src/coins_activation/src/prelude.rs Show resolved Hide resolved

dimxy reviewed Nov 12, 2024

View reviewed changes

mm2src/mm2_main/src/rpc/lp_commands/tokens.rs Show resolved Hide resolved

review fix: avoid direct access of index 0 in get_token_decimals …

f38fc3f

…and `get_token_symbol`

shamardy dismissed mariocynicys’s stale review via f38fc3f November 13, 2024 15:38

borngraced requested changes Nov 13, 2024

View reviewed changes

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

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

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

shamardy added 2 commits November 14, 2024 02:07

review fix: minor refactors

8b11a6c

Merge remote-tracking branch 'origin/dev' into feat-custom-evm-token

3b94972

borngraced approved these changes Nov 15, 2024

View reviewed changes

mariocynicys approved these changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tokens): custom token activation for evm #2141

feat(tokens): custom token activation for evm #2141

shamardy commented Jun 12, 2024 •

edited

Loading

mariocynicys left a comment

onur-ozkan Jun 24, 2024

shamardy Aug 2, 2024

mariocynicys left a comment

mariocynicys Nov 8, 2024

shamardy commented Nov 11, 2024

onur-ozkan left a comment

onur-ozkan Nov 12, 2024

mariocynicys Nov 12, 2024

shamardy Nov 13, 2024

shamardy Nov 13, 2024

shamardy commented Nov 13, 2024

Next PR:

borngraced left a comment

onur-ozkan commented Nov 14, 2024

Next PR:

shamardy commented Nov 14, 2024 •

edited

Loading

onur-ozkan commented Nov 14, 2024

shamardy commented Nov 14, 2024

dimxy commented Nov 15, 2024

feat(tokens): custom token activation for evm #2141

Are you sure you want to change the base?

feat(tokens): custom token activation for evm #2141

Conversation

shamardy commented Jun 12, 2024 • edited Loading

Todo:

Next PR:

To Test (WIP):

mariocynicys left a comment

Choose a reason for hiding this comment

onur-ozkan Jun 24, 2024

Choose a reason for hiding this comment

shamardy Aug 2, 2024

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

mariocynicys Nov 8, 2024

Choose a reason for hiding this comment

shamardy commented Nov 11, 2024

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan Nov 12, 2024

Choose a reason for hiding this comment

mariocynicys Nov 12, 2024

Choose a reason for hiding this comment

shamardy Nov 13, 2024

Choose a reason for hiding this comment

shamardy Nov 13, 2024

Choose a reason for hiding this comment

shamardy commented Nov 13, 2024

Next PR:

borngraced left a comment

Choose a reason for hiding this comment

onur-ozkan commented Nov 14, 2024

Next PR:

shamardy commented Nov 14, 2024 • edited Loading

onur-ozkan commented Nov 14, 2024

shamardy commented Nov 14, 2024

dimxy commented Nov 15, 2024

shamardy commented Jun 12, 2024 •

edited

Loading

shamardy commented Nov 14, 2024 •

edited

Loading