Added erc20 token amount type #1233

batconjurer · 2023-03-20T10:00:41Z

This PR closes #1085, #1089, and #1090.

A new type, Erc20Amount has been added for amounts of ERC20 tokens according to the EIP-20 specification. Whitelisted ERC20 tokens must now keep a denomination in storage. These denominations must also be kept in sync with the oracle, which needs them for processing ethereum events.

…point

sug0

might need another review pass, since token whitelists need some changes to allow transfers to ethereum, even if a whitelist update removes a token whose balance is still greater than 0 for some namada client

sug0 · 2023-03-27T08:08:45Z

ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs

@@ -336,6 +434,67 @@ where
    Ok(changed_key)
 }

+fn act_on_whitelist_updates<D, H>(
+    wl_storage: &mut WlStorage<D, H>,
+    whitelist: HashMap<EthAddress, Erc20Amount>,


iteration order doesn't seem to matter here, but for the correctness of any potential change to this code, I would make this a BTreeMap instead

eventually, we also need to update the token whitelist nonce in storage, I believe

sug0 · 2023-03-27T08:20:30Z

apps/src/lib/cli.rs

@@ -2274,7 +2277,7 @@ pub mod args {
        /// The sender of the transfer
        pub sender: WalletAddress,
        /// The amount to be transferred
-        pub amount: Amount,
+        pub amount: Decimal,


let result: Result<Decimal, _> = "1.000000000012345".parse()

I tested this locally, and parsing worked. what was the issue with parsing amounts with more than 0 decimal places?

sug0 · 2023-03-27T08:25:28Z

apps/src/lib/cli.rs

+            let erc20_amount = TOKEN_AMOUNT_OPT.parse(matches);
+            let amount = AMOUNT_OPT.parse(matches);
+            if let Some(amt) = erc20_amount {
+                Self::Erc20(amt)
+            } else if let Some(amt) = amount {
+                Self::Native(amt)
+            } else {
+                panic!("No amount provided")
+            }


if CLI receives two distinct type of amount arguments, only erc20 amounts will be considered. for better UX, perhaps it's better to fail parsing if both of them are Some?

https://docs.rs/clap/latest/clap/error/struct.Error.html
maybe we could also use this API instead of panicking

sug0 · 2023-03-27T08:42:30Z

apps/src/lib/node/ledger/ethereum_oracle/events.rs

@@ -124,31 +128,34 @@ pub mod eth_events {
            block_height: Uint256,
            data: &[u8],
            min_confirmations: Uint256,
+            erc20_tokens: &HashMap<EthAddress, u8>,


This file was already heavily refactored here: #1252
shit, what do we do about this?

sug0 · 2023-03-27T09:06:55Z

apps/src/lib/node/ledger/shell/init_chain.rs

@@ -157,7 +157,7 @@ where
        if let Some(config) = genesis.ethereum_bridge_params {
            tracing::debug!("Initializing Ethereum bridge storage.");
            config.init_storage(&mut self.wl_storage);
-            self.update_eth_oracle();
+            self.update_eth_oracle(true);


nit: I'm not generally a big fan of these "naked" boolean values. what does it mean for update_eth_oracle to take a value of true, without having any context on what the argument's name is?

sug0 · 2023-03-27T09:47:03Z

core/src/types/ethereum_events.rs

+impl From<Uint> for u64 {
+    fn from(uint: Uint) -> Self {
+        // N.B. this will panic if `uint` > `u64::MAX`.
+        ethUint::from(uint).as_u64()
+    }
+}
+


it's a bit odd for a From implementation to panic. can we do without it? or implement TryFrom instead

sug0 · 2023-03-27T09:54:38Z

ethereum_bridge/src/protocol/transactions/update.rs

+    update: impl FnOnce(Option<T>) -> T,
+) -> Result<T>


is this change because erc20s don't have a default value?

sug0 · 2023-03-27T13:28:05Z

ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs

+        EthereumEvent::UpdateBridgeWhitelist { whitelist, .. } => {
+            Ok(act_on_whitelist_updates(
+                wl_storage,
+                whitelist
+                    .iter()
+                    .map(|TokenWhitelist { token, cap }| (*token, *cap))
+                    .collect(),
+            ))
+        }


add a TODO here, since we need to update the token whitelist nonce in storage (I think?)

sug0 · 2023-03-27T13:38:00Z

ethereum_bridge/src/storage/eth_bridge_queries.rs

+            self.wl_storage.ethbridge_queries().get_wnam_address()
+        {
+            if addr == *token_address {
+                return Some(6);


nit: maybe implement a const fn token::Amount::denomination() -> u8 { 6 }?

sug0 · 2023-03-27T13:39:38Z

shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs

+                            |e| {
+                                eyre!(
+                                    "Could not process the amount of NAM to \
+                                     be escrowed into the bridge pool: {:?}",


nit: assuming this implements Error, it's best to display the actual error string

Suggested change

be escrowed into the bridge pool: {:?}",

be escrowed into the bridge pool: {}",

batconjurer · 2023-03-28T08:09:25Z

might need another review pass, since token whitelists need some changes to allow transfers to ethereum, even if a whitelist update removes a token whose balance is still greater than 0 for some namada client

Yes, but that will be another PR.

sug0 · 2023-04-13T13:16:21Z

This PR contains an implementation that closes #1290

sug0 · 2023-07-15T13:42:24Z

Superseded by #1718 and #1282

[feat]: Added erc20 token amount type

daa795c

batconjurer marked this pull request as draft March 20, 2023 10:01

batconjurer added 7 commits March 20, 2023 14:10

[chore]: Added tests for the whitelist updates in the oracle

60113a0

[feat]: Add whitelisting updates and covered with tests

9c82f36

[fix]: Fixed unit test which drops channel prematurely

91ae98e

[fix]: Fixing whitelisting, e2e tests, the self hosted eth events end…

ebd5d59

…point

[fix]: Fixed erc20 token transfers

f098085

[fix]: Fixed multitoken e2e tests

973eafd

[fix]: Fixed abcipp and formatting

c94ca1d

batconjurer requested review from sug0 and tzemanovic March 24, 2023 12:43

batconjurer marked this pull request as ready for review March 24, 2023 12:46

[chore]: Reverted unneccessary changes to the wasm_for_tests

780ee10

sug0 requested changes Mar 27, 2023

View reviewed changes

tzemanovic mentioned this pull request Mar 28, 2023

alternative impl of erc20 token amount #1259

Closed

cwgoes mentioned this pull request May 18, 2023

Feature merge priority queue #1361

Closed

10 tasks

sug0 closed this Jul 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added erc20 token amount type #1233

Added erc20 token amount type #1233

batconjurer commented Mar 20, 2023 •

edited

Loading

sug0 left a comment

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

sug0 Mar 27, 2023

batconjurer commented Mar 28, 2023

sug0 commented Apr 13, 2023

sug0 commented Jul 15, 2023

	be escrowed into the bridge pool: {:?}",
	be escrowed into the bridge pool: {}",

Added erc20 token amount type #1233

Added erc20 token amount type #1233

Conversation

batconjurer commented Mar 20, 2023 • edited Loading

sug0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batconjurer commented Mar 28, 2023

sug0 commented Apr 13, 2023

sug0 commented Jul 15, 2023

batconjurer commented Mar 20, 2023 •

edited

Loading