Merge branch 'gdemay/XC-83-ckerc20-tokens-data-struct' into 'master'

refactor(ckerc20): More efficient data structure to store ckERC20 tokens

(XC-83): For ckERC20 the minter needs to retrieve efficiently:
1. Given an ERC-20 smart contract address, the corresponding ckERC20 ledger canister ID and the ckERC20 token symbol (deposits)
2. Given a ckERC20 ledger ID, the corresponding ERC-20 smart contract address and ckERC20 token symbol (withdrawals).

The approach before this MR only allows for efficient retrieval (O(1)) for one of the 2 use-cases. This MR improves this by providing a thin wrapper around `MultiKeyMap` to deduplicate both the primary key (ckERC20 ledger canister ID) and the alternative key (ERC-20 smart contract address) without impacting other usages of `MultiKeyMap`. 

See merge request dfinity-lab/public/ic!19533

rs/ethereum/cketh/minter/src/dashboard.rs

@@ -125,7 +125,8 @@ impl DashboardPendingDeposit {
             token_symbol: match event {
                 ReceivedEvent::Eth(_) => CkTokenSymbol::cketh_symbol_from_state(state),
                 ReceivedEvent::Erc20(e) => state
-                    .ckerc20_token_symbol(&e.erc20_contract_address)
+                    .ckerc20_tokens
+                    .get_alt(&e.erc20_contract_address)
                     .expect("BUG: unknown ERC-20 token")
                     .clone(),
             },
@@ -204,16 +205,20 @@ impl DashboardTemplate {
                     token_symbol: CkTokenSymbol::cketh_symbol_from_state(state),
                     created_at: req.created_at,
                 },
-                WithdrawalRequest::CkErc20(req) => DashboardWithdrawalRequest {
-                    cketh_ledger_burn_index: req.cketh_ledger_burn_index,
-                    destination: req.destination,
-                    value: req.withdrawal_amount.into(),
-                    token_symbol: state
-                        .ckerc20_token_symbol(&req.erc20_contract_address)
-                        .expect("BUG: unknown ERC-20 token")
-                        .clone(),
-                    created_at: Some(req.created_at),
-                },
+                WithdrawalRequest::CkErc20(req) => {
+                    let erc20_contract_address = &req.erc20_contract_address;
+                    DashboardWithdrawalRequest {
+                        cketh_ledger_burn_index: req.cketh_ledger_burn_index,
+                        destination: req.destination,
+                        value: req.withdrawal_amount.into(),
+                        token_symbol: state
+                            .ckerc20_tokens
+                            .get_alt(erc20_contract_address)
+                            .expect("BUG: unknown ERC-20 token")
+                            .clone(),
+                        created_at: Some(req.created_at),
+                    }
+                }
             })
             .collect();
         withdrawal_requests.sort_unstable_by_key(|req| Reverse(req.cketh_ledger_burn_index));
@@ -284,7 +289,8 @@ impl DashboardTemplate {
                     } => (
                         *cketh_ledger_burn_index,
                         state
-                            .ckerc20_token_symbol_for_ledger(ledger_id)
+                            .ckerc20_tokens
+                            .get(ledger_id)
                             .expect("BUG: unknown ERC-20 token")
                             .clone(),
                     ),
@@ -357,7 +363,8 @@ fn to_dashboard_transaction<T: AsRef<Eip1559TransactionRequest>>(
         let destination = to;
         let value = value.into();
         let token_symbol = state
-            .ckerc20_token_symbol(&tx.destination)
+            .ckerc20_tokens
+            .get_alt(&tx.destination)
             .expect("BUG: unknown ERC-20 token")
             .clone();
         (destination, value, token_symbol)

rs/ethereum/cketh/minter/src/deposit.rs

@@ -52,7 +52,7 @@ async fn mint() {
                 if let Some(result) = read_state(|s| {
                     s.ckerc20_tokens
                         .get_entry_alt(&event.erc20_contract_address)
-                        .map(|(symbol, principal)| (symbol.to_string(), *principal))
+                        .map(|(principal, symbol)| (symbol.to_string(), *principal))
                 }) {
                     result
                 } else {

rs/ethereum/cketh/minter/src/main.rs

@@ -15,6 +15,7 @@ use ic_cketh_minter::endpoints::{
     RetrieveEthRequest, RetrieveEthStatus, WithdrawalArg, WithdrawalDetail, WithdrawalError,
     WithdrawalSearchParameter,
 };
+use ic_cketh_minter::erc20::CkTokenSymbol;
 use ic_cketh_minter::eth_logs::{EventSource, ReceivedErc20Event, ReceivedEthEvent};
 use ic_cketh_minter::guard::retrieve_withdraw_guard;
 use ic_cketh_minter::ledger_client::{LedgerBurnError, LedgerClient};
@@ -300,9 +301,10 @@ async fn withdrawal_status(parameter: WithdrawalSearchParameter) -> Vec<Withdraw
                 withdrawal_id: *request.cketh_ledger_burn_index().as_ref(),
                 recipient_address: request.payee().to_string(),
                 token_symbol: match request {
-                    CkEth(_) => "ckETH".to_string(),
+                    CkEth(_) => CkTokenSymbol::cketh_symbol_from_state(s).to_string(),
                     CkErc20(r) => s
-                        .ckerc20_token_symbol(&r.erc20_contract_address)
+                        .ckerc20_tokens
+                        .get_alt(&r.erc20_contract_address)
                         .unwrap()
                         .to_string(),
                 },

rs/ethereum/cketh/minter/src/map.rs

@@ -104,20 +104,6 @@ impl<Key: Ord, AltKey: Ord, V> MultiKeyMap<Key, AltKey, V> {
             .and_then(|alt_key| self.by_alt_key.get(alt_key).map(|v| (alt_key, v)))
     }
 
-    pub fn get_entry_alt<Q: ?Sized>(&self, alt_key: &Q) -> Option<(&Key, &V)>
-    where
-        AltKey: Borrow<Q>,
-        Q: Ord,
-    {
-        self.iter().find_map(|(key, alt_key_, v)| {
-            if alt_key_.borrow() == alt_key {
-                Some((key, v))
-            } else {
-                None
-            }
-        })
-    }
-
     pub fn get_mut<Q: ?Sized>(&mut self, key: &Q) -> Option<&mut V>
     where
         Key: Borrow<Q>,
@@ -227,3 +213,105 @@ impl<Key: Ord, AltKey: Ord + Clone, V> FromIterator<(Key, AltKey, V)>
         Self { by_key, by_alt_key }
     }
 }
+
+/// A map with two keys: a primary key `Key` and an alternative key `AltKey`.
+/// The stored value `V` is indexed by both keys and can be efficiently retrieved from either key.
+///
+/// In comparison to `MultiKeyMap`, which only duplicates the alternative key,
+/// both the primary key and the alternative key are duplicated.
+/// This makes it possible to efficiently retrieve both the entry (AltKey, V) given the primary key
+/// and the entry (Key, V) given the alternative key.
+///
+/// Internally, this struct is a simple wrapper around `MultiKeyMap<Key, AltKey, (Key, V)>`.
+#[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)]
+pub struct DedupMultiKeyMap<Key, AltKey, V>
+where
+    Key: Ord,
+    AltKey: Ord,
+{
+    map: MultiKeyMap<Key, AltKey, (Key, V)>,
+}
+
+impl<Key: Ord, AltKey: Ord, V> DedupMultiKeyMap<Key, AltKey, V> {
+    pub fn new() -> Self {
+        Self {
+            map: Default::default(),
+        }
+    }
+
+    pub fn try_insert(
+        &mut self,
+        key: Key,
+        alt_key: AltKey,
+        value: V,
+    ) -> Result<(), OccupiedError<Key, AltKey, V>>
+    where
+        Key: Clone,
+        AltKey: Clone,
+    {
+        self.map
+            .try_insert(key.clone(), alt_key.clone(), (key, value))
+            .map_err(|err| OccupiedError {
+                occupied_key: err.occupied_key,
+                value: err.value.1,
+            })
+    }
+
+    pub fn get<Q: ?Sized>(&self, key: &Q) -> Option<&V>
+    where
+        Key: Borrow<Q>,
+        Q: Ord,
+    {
+        self.map.get(key).map(|(_key, value)| value)
+    }
+
+    pub fn get_alt<Q: ?Sized>(&self, alt_key: &Q) -> Option<&V>
+    where
+        AltKey: Borrow<Q>,
+        Q: Ord,
+    {
+        self.map.get_alt(alt_key).map(|(_key, value)| value)
+    }
+
+    pub fn get_entry<Q: ?Sized>(&self, key: &Q) -> Option<(&AltKey, &V)>
+    where
+        Key: Borrow<Q>,
+        Q: Ord,
+    {
+        self.map
+            .get_entry(key)
+            .map(|(alt_key, (_key, value))| (alt_key, value))
+    }
+
+    pub fn get_entry_alt<Q: ?Sized>(&self, alt_key: &Q) -> Option<(&Key, &V)>
+    where
+        AltKey: Borrow<Q>,
+        Q: Ord,
+    {
+        self.map.get_alt(alt_key).map(|(key, value)| (key, value))
+    }
+
+    pub fn contains_alt<Q: ?Sized>(&self, alt_key: &Q) -> bool
+    where
+        AltKey: Borrow<Q>,
+        Q: Ord,
+    {
+        self.map.contains_alt(alt_key)
+    }
+
+    pub fn alt_keys(&self) -> impl Iterator<Item = &AltKey> {
+        self.map.alt_keys()
+    }
+
+    pub fn iter(&self) -> impl Iterator<Item = (&Key, &AltKey, &V)> {
+        self.map
+            .iter()
+            .map(|(key, alt_key, (_key, value))| (key, alt_key, value))
+    }
+}
+
+impl<Key: Ord, AltKey: Ord, V> Default for DedupMultiKeyMap<Key, AltKey, V> {
+    fn default() -> Self {
+        Self::new()
+    }
+}

rs/ethereum/cketh/minter/src/map/tests.rs

@@ -15,21 +15,13 @@ fn should_insert_and_retrieve_by_key_and_alt_key() {
         Some((&AltKey::new('a'), &1))
     );
     assert_eq!(map.get_alt(&AltKey::new('a')), Some(&1));
-    assert_eq!(
-        map.get_entry_alt(&AltKey::new('a')),
-        Some((&PrimaryKey::new(1), &1))
-    );
 
     assert_eq!(map.get(&PrimaryKey::new(2)), Some(&2));
     assert_eq!(
         map.get_entry(&PrimaryKey::new(2)),
         Some((&AltKey::new('b'), &2))
     );
     assert_eq!(map.get_alt(&AltKey::new('b')), Some(&2));
-    assert_eq!(
-        map.get_entry_alt(&AltKey::new('b')),
-        Some((&PrimaryKey::new(2), &2))
-    );
 }
 
 #[test]

rs/ethereum/cketh/minter/src/state.rs

@@ -6,7 +6,7 @@ use crate::eth_rpc_client::responses::{TransactionReceipt, TransactionStatus};
 use crate::lifecycle::upgrade::UpgradeArg;
 use crate::lifecycle::EthereumNetwork;
 use crate::logs::DEBUG;
-use crate::map::MultiKeyMap;
+use crate::map::DedupMultiKeyMap;
 use crate::numeric::{
     BlockNumber, Erc20Value, LedgerBurnIndex, LedgerMintIndex, TransactionNonce, Wei,
 };
@@ -93,10 +93,10 @@ pub struct State {
     pub ledger_suite_orchestrator_id: Option<Principal>,
 
     /// ERC-20 tokens that the minter can mint:
-    /// - primary key: ckERC20 token symbol
+    /// - primary key: ledger ID for the ckERC20 token
     /// - secondary key: ERC-20 contract address on Ethereum
-    /// - value: ledger ID for the ckERC20 token
-    pub ckerc20_tokens: MultiKeyMap<CkTokenSymbol, Address, Principal>,
+    /// - value: ckERC20 token symbol
+    pub ckerc20_tokens: DedupMultiKeyMap<Principal, Address, CkTokenSymbol>,
 }
 
 #[derive(Debug, Eq, PartialEq)]
@@ -223,44 +223,27 @@ impl State {
         &self,
         ckerc20_ledger_id: &Principal,
     ) -> Option<CkErc20Token> {
-        //TODO XC-83: refactor data structure for ckerc20_tokens
-        let results: Vec<_> = self
-            .supported_ck_erc20_tokens()
-            .filter(|supported_ckerc20_token| {
-                &supported_ckerc20_token.ckerc20_ledger_id == ckerc20_ledger_id
+        self.ckerc20_tokens
+            .get_entry(ckerc20_ledger_id)
+            .map(|(erc20_address, symbol)| CkErc20Token {
+                erc20_contract_address: *erc20_address,
+                ckerc20_ledger_id: *ckerc20_ledger_id,
+                erc20_ethereum_network: self.ethereum_network,
+                ckerc20_token_symbol: symbol.clone(),
             })
-            .collect();
-        assert!(
-            results.len() <= 1,
-            "BUG: multiple ckERC20 tokens with the same ledger ID {ckerc20_ledger_id}"
-        );
-        results.into_iter().next()
     }
 
     pub fn supported_ck_erc20_tokens(&self) -> impl Iterator<Item = CkErc20Token> + '_ {
         self.ckerc20_tokens
             .iter()
-            .map(|(symbol, erc20_address, ledger_id)| CkErc20Token {
+            .map(|(ledger_id, erc20_address, symbol)| CkErc20Token {
                 erc20_contract_address: *erc20_address,
                 ckerc20_ledger_id: *ledger_id,
                 erc20_ethereum_network: self.ethereum_network,
                 ckerc20_token_symbol: symbol.clone(),
             })
     }
 
-    pub fn ckerc20_token_symbol(&self, erc20_contract_address: &Address) -> Option<&CkTokenSymbol> {
-        self.ckerc20_tokens
-            .get_entry_alt(erc20_contract_address)
-            .map(|(symbol, _)| symbol)
-    }
-
-    pub fn ckerc20_token_symbol_for_ledger(&self, ledger_id: &Principal) -> Option<&CkTokenSymbol> {
-        self.ckerc20_tokens
-            .iter()
-            .find(|(_, _, id)| *id == ledger_id)
-            .map(|(symbol, _, _)| symbol)
-    }
-
     /// Quarantine the deposit event to prevent double minting.
     /// WARNING!: It's crucial that this method does not panic,
     /// since it's called inside the clean-up callback, when an unexpected panic did occur before.
@@ -410,27 +393,25 @@ impl State {
             "ERROR: Expected {}, but got {}",
             self.ethereum_network, ckerc20_token.erc20_ethereum_network
         );
-        let duplicate_ledger_id: MultiKeyMap<_, _, _> = self
-            .ckerc20_tokens
-            .iter()
-            .filter(|(_erc20_address, _ckerc20_token_symbol, &ledger_id)| {
-                ledger_id == ckerc20_token.ckerc20_ledger_id
-            })
-            .collect();
+        let ckerc20_with_same_symbol = self
+            .supported_ck_erc20_tokens()
+            .filter(|ckerc20| ckerc20.ckerc20_token_symbol == ckerc20_token.ckerc20_token_symbol)
+            .collect::<Vec<_>>();
         assert_eq!(
-            duplicate_ledger_id,
-            MultiKeyMap::default(),
-            "ERROR: ledger ID {} is already in use",
-            ckerc20_token.ckerc20_ledger_id
+            ckerc20_with_same_symbol,
+            vec![],
+            "ERROR: ckERC20 token symbol {} is already used by {:?}",
+            ckerc20_token.ckerc20_token_symbol,
+            ckerc20_with_same_symbol
         );
         assert_eq!(
             self.ckerc20_tokens.try_insert(
-                ckerc20_token.ckerc20_token_symbol,
-                ckerc20_token.erc20_contract_address,
                 ckerc20_token.ckerc20_ledger_id,
+                ckerc20_token.erc20_contract_address,
+                ckerc20_token.ckerc20_token_symbol,
             ),
             Ok(()),
-            "ERROR: some ckERC20 tokens use the same ERC-20 address or symbol"
+            "ERROR: some ckERC20 tokens use the same ckERC20 ledger ID or ERC-20 address"
         );
     }
 
@@ -440,7 +421,8 @@ impl State {
             .iter()
             .map(|(erc20_contract, balance)| {
                 let symbol = self
-                    .ckerc20_token_symbol(erc20_contract)
+                    .ckerc20_tokens
+                    .get_alt(erc20_contract)
                     .unwrap_or_else(|| {
                         panic!("BUG: missing symbol for ERC-20 contract {}", erc20_contract)
                     });