Skip to content

Commit

Permalink
fix(cketh): Undo breaking change in get_minter_info (#2907)
Browse files Browse the repository at this point in the history
Fix an undesired breaking changed introduced by #2747 :

1. The fields `eth_helper_contract_address` and
`erc20_helper_contract_address` in `get_minter_info` were wrongly reused
to point to the new helper smart contract
[0x18901044688D3756C35Ed2b36D93e6a5B8e00E68](https://etherscan.io/address/0x18901044688D3756C35Ed2b36D93e6a5B8e00E68)
that supports deposit with subaccounts and that was added as part of
proposal
[134264](https://dashboard.internetcomputer.org/proposal/134264).
2. This broke clients that relied on that information to make deposit of
ETH or ERC-20 because the new helper smart contract has a different ABI.
This is visible by such a
[transaction](https://etherscan.io/tx/0x0968b25814221719bf966cf4bbd2de8290ed2ab42c049d451d64e46812d1574e),
where the transaction tried to call the method `deposit` (`0xb214faa5`)
that does exist on the [deprecated ETH helper smart
contract](https://etherscan.io/address/0x7574eB42cA208A4f6960ECCAfDF186D627dCC175)
but doesn't on the new contract (it should have been `depositEth`
(`0x17c819c4`)).
3. The fix simply consists in reverting the changes regarding the values
of the fields `eth_helper_contract_address` and
`erc20_helper_contract_address` in `get_minter_info` (so that they point
back to
[0x7574eB42cA208A4f6960ECCAfDF186D627dCC175](https://etherscan.io/address/0x7574eB42cA208A4f6960ECCAfDF186D627dCC175)
and
[0x6abDA0438307733FC299e9C229FD3cc074bD8cC0](https://etherscan.io/address/0x6abDA0438307733FC299e9C229FD3cc074bD8cC0),
respectively) and adding new fields to contain the state of the log
scraping (address and last scraped block number) for the new helper
smart contract.

---------

Co-authored-by: Thomas Locher <thomas.locher@dfinity.org>
  • Loading branch information
gregorydemay and THLO authored Nov 30, 2024
1 parent 25c1bb0 commit 5ce01d0
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 21 deletions.
6 changes: 6 additions & 0 deletions rs/ethereum/cketh/minter/cketh_minter.did
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@ type MinterInfo = record {
// Address of the ERC20 helper smart contract
erc20_helper_contract_address: opt text;

// Address of the ETH or ERC20 deposit with subaccount helper smart contract.
deposit_with_subaccount_helper_contract_address: opt text;

// Information of supported ERC20 tokens.
supported_ckerc20_tokens: opt vec CkErc20Token;

Expand Down Expand Up @@ -201,6 +204,9 @@ type MinterInfo = record {
// Last scraped block number for logs of the ERC20 helper contract.
last_erc20_scraped_block_number: opt nat;

// Last scraped block number for logs of the deposit with subaccount helper contract.
last_deposit_with_subaccount_scraped_block_number: opt nat;

// Canister ID of the ckETH ledger.
cketh_ledger_id: opt principal;

Expand Down
2 changes: 2 additions & 0 deletions rs/ethereum/cketh/minter/src/endpoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ pub struct MinterInfo {
pub smart_contract_address: Option<String>,
pub eth_helper_contract_address: Option<String>,
pub erc20_helper_contract_address: Option<String>,
pub deposit_with_subaccount_helper_contract_address: Option<String>,
pub supported_ckerc20_tokens: Option<Vec<CkErc20Token>>,
pub minimum_withdrawal_amount: Option<Nat>,
pub ethereum_block_height: Option<CandidBlockTag>,
Expand All @@ -76,6 +77,7 @@ pub struct MinterInfo {
pub erc20_balances: Option<Vec<Erc20Balance>>,
pub last_eth_scraped_block_number: Option<Nat>,
pub last_erc20_scraped_block_number: Option<Nat>,
pub last_deposit_with_subaccount_scraped_block_number: Option<Nat>,
pub cketh_ledger_id: Option<Principal>,
pub evm_rpc_id: Option<Principal>,
}
Expand Down
4 changes: 4 additions & 0 deletions rs/ethereum/cketh/minter/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,16 @@ async fn get_minter_info() -> MinterInfo {
last_eth_scraped_block_number,
erc20_helper_contract_address,
last_erc20_scraped_block_number,
deposit_with_subaccount_helper_contract_address,
last_deposit_with_subaccount_scraped_block_number,
} = s.log_scrapings.info();

MinterInfo {
minter_address: s.minter_address().map(|a| a.to_string()),
smart_contract_address: eth_helper_contract_address.clone(),
eth_helper_contract_address,
erc20_helper_contract_address,
deposit_with_subaccount_helper_contract_address,
supported_ckerc20_tokens,
minimum_withdrawal_amount: Some(s.cketh_minimum_withdrawal_amount.into()),
ethereum_block_height: Some(s.ethereum_block_height.into()),
Expand All @@ -249,6 +252,7 @@ async fn get_minter_info() -> MinterInfo {
erc20_balances,
last_eth_scraped_block_number,
last_erc20_scraped_block_number,
last_deposit_with_subaccount_scraped_block_number,
cketh_ledger_id: Some(s.cketh_ledger_id),
evm_rpc_id: s.evm_rpc_id,
}
Expand Down
21 changes: 8 additions & 13 deletions rs/ethereum/cketh/minter/src/state/eth_logs_scraping/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,28 +69,21 @@ impl LogScrapings {
let last_scraped_block_number = Some(Nat::from(state.last_scraped_block_number()));
(contract_address, last_scraped_block_number)
};
let (
deposit_with_subaccount_contract_address,
deposit_with_subaccount_last_scraped_block_number,
) = to_info(self.get(LogScrapingId::EthOrErc20DepositWithSubaccount));
if deposit_with_subaccount_contract_address.is_some() {
return LogScrapingInfo {
eth_helper_contract_address: deposit_with_subaccount_contract_address.clone(),
last_eth_scraped_block_number: deposit_with_subaccount_last_scraped_block_number
.clone(),
erc20_helper_contract_address: deposit_with_subaccount_contract_address,
last_erc20_scraped_block_number: deposit_with_subaccount_last_scraped_block_number,
};
}
let (eth_helper_contract_address, last_eth_scraped_block_number) =
to_info(self.get(LogScrapingId::EthDepositWithoutSubaccount));
let (erc20_helper_contract_address, last_erc20_scraped_block_number) =
to_info(self.get(LogScrapingId::Erc20DepositWithoutSubaccount));
let (
deposit_with_subaccount_helper_contract_address,
last_deposit_with_subaccount_scraped_block_number,
) = to_info(self.get(LogScrapingId::EthOrErc20DepositWithSubaccount));
LogScrapingInfo {
eth_helper_contract_address,
last_eth_scraped_block_number,
erc20_helper_contract_address,
last_erc20_scraped_block_number,
deposit_with_subaccount_helper_contract_address,
last_deposit_with_subaccount_scraped_block_number,
}
}
}
Expand Down Expand Up @@ -202,4 +195,6 @@ pub struct LogScrapingInfo {
pub last_eth_scraped_block_number: Option<Nat>,
pub erc20_helper_contract_address: Option<String>,
pub last_erc20_scraped_block_number: Option<Nat>,
pub deposit_with_subaccount_helper_contract_address: Option<String>,
pub last_deposit_with_subaccount_scraped_block_number: Option<Nat>,
}
21 changes: 14 additions & 7 deletions rs/ethereum/cketh/minter/src/state/eth_logs_scraping/tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::state::eth_logs_scraping::{LogScrapingId, LogScrapingInfo, LogScrapings};

#[test]
fn should_use_info_from_deposit_with_subaccount_when_contract_address_present() {
fn should_not_change_other_field_when_deposit_with_subaccount_present() {
const LAST_SCRAPED_BLOCK_NUMBER: u32 = 21_235_426_u32;
const ETH_HELPER_SMART_CONTRACT: &str = "0x7574eB42cA208A4f6960ECCAfDF186D627dCC175";
const ERC20_HELPER_SMART_CONTRACT: &str = "0x6abDA0438307733FC299e9C229FD3cc074bD8cC0";
Expand All @@ -15,6 +15,9 @@ fn should_use_info_from_deposit_with_subaccount_when_contract_address_present()
LogScrapingInfo {
last_eth_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER.into()),
last_erc20_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER.into()),
last_deposit_with_subaccount_scraped_block_number: Some(
LAST_SCRAPED_BLOCK_NUMBER.into()
),
..Default::default()
}
);
Expand All @@ -32,13 +35,18 @@ fn should_use_info_from_deposit_with_subaccount_when_contract_address_present()
)
.unwrap();

let info_before = scrapings.info();
assert_eq!(
scrapings.info(),
info_before,
LogScrapingInfo {
eth_helper_contract_address: Some(ETH_HELPER_SMART_CONTRACT.to_string()),
last_eth_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER.into()),
erc20_helper_contract_address: Some(ERC20_HELPER_SMART_CONTRACT.to_string()),
last_erc20_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER.into()),
deposit_with_subaccount_helper_contract_address: None,
last_deposit_with_subaccount_scraped_block_number: Some(
LAST_SCRAPED_BLOCK_NUMBER.into()
),
}
);

Expand All @@ -58,14 +66,13 @@ fn should_use_info_from_deposit_with_subaccount_when_contract_address_present()
assert_eq!(
scrapings.info(),
LogScrapingInfo {
eth_helper_contract_address: Some(
deposit_with_subaccount_helper_contract_address: Some(
DEPOSIT_WITH_SUBACCOUNT_HELPER_SMART_CONTRACT.to_string()
),
last_eth_scraped_block_number: Some((LAST_SCRAPED_BLOCK_NUMBER + 1).into()),
erc20_helper_contract_address: Some(
DEPOSIT_WITH_SUBACCOUNT_HELPER_SMART_CONTRACT.to_string()
last_deposit_with_subaccount_scraped_block_number: Some(
(LAST_SCRAPED_BLOCK_NUMBER + 1).into()
),
last_erc20_scraped_block_number: Some((LAST_SCRAPED_BLOCK_NUMBER + 1).into()),
..info_before
}
);
}
46 changes: 45 additions & 1 deletion rs/ethereum/cketh/minter/tests/ckerc20.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ use ic_cketh_test_utils::{
format_ethereum_address_to_eip_55, CkEthSetup, CKETH_MINIMUM_WITHDRAWAL_AMOUNT,
DEFAULT_DEPOSIT_BLOCK_NUMBER, DEFAULT_DEPOSIT_FROM_ADDRESS, DEFAULT_DEPOSIT_LOG_INDEX,
DEFAULT_DEPOSIT_TRANSACTION_HASH, DEFAULT_ERC20_DEPOSIT_LOG_INDEX,
DEFAULT_ERC20_DEPOSIT_TRANSACTION_HASH, DEFAULT_USER_SUBACCOUNT, EFFECTIVE_GAS_PRICE,
DEFAULT_ERC20_DEPOSIT_TRANSACTION_HASH, DEFAULT_USER_SUBACCOUNT,
DEPOSIT_WITH_SUBACCOUNT_HELPER_CONTRACT_ADDRESS, EFFECTIVE_GAS_PRICE,
ERC20_HELPER_CONTRACT_ADDRESS, ETH_HELPER_CONTRACT_ADDRESS, GAS_USED,
LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL, MINTER_ADDRESS,
};
Expand Down Expand Up @@ -1681,6 +1682,7 @@ fn should_retrieve_minter_info() {
erc20_helper_contract_address: Some(format_ethereum_address_to_eip_55(
ERC20_HELPER_CONTRACT_ADDRESS
)),
deposit_with_subaccount_helper_contract_address: None,
supported_ckerc20_tokens: Some(supported_ckerc20_tokens),
minimum_withdrawal_amount: Some(Nat::from(CKETH_MINIMUM_WITHDRAWAL_AMOUNT)),
ethereum_block_height: Some(Finalized),
Expand All @@ -1690,12 +1692,54 @@ fn should_retrieve_minter_info() {
erc20_balances: Some(erc20_balances),
last_eth_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()),
last_erc20_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()),
last_deposit_with_subaccount_scraped_block_number: Some(
LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()
),
cketh_ledger_id: Some(ckerc20.cketh_ledger_id()),
evm_rpc_id: ckerc20.cketh.evm_rpc_id.map(Principal::from),
}
);
}

// If a contract needs to be changed, the contract ABI must be preserved,
// otherwise this will be a breaking change for clients (e.g. front-ends).
#[test]
#[allow(deprecated)]
fn should_not_change_address_of_other_contracts_when_adding_new_contract() {
let ckerc20 = CkErc20Setup::default();
let info_at_start = ckerc20.cketh.get_minter_info();
assert_eq!(
info_at_start,
MinterInfo {
smart_contract_address: Some(format_ethereum_address_to_eip_55(
ETH_HELPER_CONTRACT_ADDRESS
)),
eth_helper_contract_address: Some(format_ethereum_address_to_eip_55(
ETH_HELPER_CONTRACT_ADDRESS
)),
erc20_helper_contract_address: Some(format_ethereum_address_to_eip_55(
ERC20_HELPER_CONTRACT_ADDRESS
)),
..info_at_start.clone()
}
);

let ckerc20 = ckerc20.add_support_for_subaccount();

assert_eq!(
ckerc20.get_minter_info(),
MinterInfo {
deposit_with_subaccount_helper_contract_address: Some(
format_ethereum_address_to_eip_55(DEPOSIT_WITH_SUBACCOUNT_HELPER_CONTRACT_ADDRESS)
),
last_deposit_with_subaccount_scraped_block_number: Some(
LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()
),
..info_at_start
}
)
}

#[test]
fn should_scrape_from_last_scraped_after_upgrade() {
let mut ckerc20 = CkErc20Setup::default().add_supported_erc20_tokens();
Expand Down
4 changes: 4 additions & 0 deletions rs/ethereum/cketh/minter/tests/cketh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,7 @@ fn should_retrieve_minter_info() {
ETH_HELPER_CONTRACT_ADDRESS
)),
erc20_helper_contract_address: None,
deposit_with_subaccount_helper_contract_address: None,
supported_ckerc20_tokens: None,
minimum_withdrawal_amount: Some(Nat::from(CKETH_MINIMUM_WITHDRAWAL_AMOUNT)),
ethereum_block_height: Some(Finalized),
Expand All @@ -1096,6 +1097,9 @@ fn should_retrieve_minter_info() {
erc20_balances: None,
last_eth_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()),
last_erc20_scraped_block_number: Some(LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()),
last_deposit_with_subaccount_scraped_block_number: Some(
LAST_SCRAPED_BLOCK_NUMBER_AT_INSTALL.into()
),
cketh_ledger_id: Some(cketh.ledger_id.into()),
evm_rpc_id: cketh.evm_rpc_id.map(Principal::from),
}
Expand Down

0 comments on commit 5ce01d0

Please sign in to comment.