Skip to content

Commit

Permalink
Merge branch 'gdemay/XC-96-ckerc20-withdrawal-without-reimbursements'…
Browse files Browse the repository at this point in the history
… into 'master'

feat(ckerc20): ckERC20 withdrawals without reimbursement of transaction fees [override-didc-check]

(XC-96): Follow-up on [MR-18621](https://gitlab.com/dfinity-lab/public/ic/-/merge_requests/18621) to improve the user experience during a ckERC20 withdrawal
1. No longer require a high minimum ckETH withdrawal amount, which was previously set to `30_000_000_000_000_000` Wei (roughly 100 USD), to be able to make a ckERC20 withdrawal. Instead, the current gas fee is retrieved only if the current estimate is too old, and the necessary amount of ckETH will be burned to pay for the transaction fee.
2. Although the minter still needs to over-estimate the transaction fee, to ensure that future transactions are not blocked, the resulting transaction fee is in the same order of magnitude as the effective transaction fee (typically within a factor 2) and this MR therefore simplifies the withdrawal flow further by no longer reimbursing unspent transaction fees for ckERC20 withdrawals.

Remark on `[override-didc-check]`: the changes in the Candid interface only impact the ckERC20 feature that is not yet released and are therefore not breaking. 

See merge request dfinity-lab/public/ic!18632
  • Loading branch information
gregorydemay committed Apr 10, 2024
2 parents 2a63bb3 + 860af97 commit fd03bd3
Show file tree
Hide file tree
Showing 14 changed files with 463 additions and 270 deletions.
6 changes: 5 additions & 1 deletion rs/ethereum/cketh/minter/cketh_minter.did
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ type WithdrawalError = variant {
// Recipient's address is blocked.
// No withdrawal can be made to that address.
RecipientAddressBlocked : record { address : text };
// The minter is overloaded, retry the request.
// The minter or the ckETH ledger is temporarily unavailable, retry the request.
// The payload contains a human-readable message explaining what caused the unavailability.
TemporarilyUnavailable : text;
};
Expand Down Expand Up @@ -270,6 +270,10 @@ type WithdrawErc20Error = variant {
// The minter could not burn the requested amount of ckERC20 tokens.
// The `cketh_block_index` identifies the burn that occurred on the ckETH ledger and that will be reimbursed.
CkErc20LedgerError : record { cketh_block_index : nat; error : LedgerError };

// The minter is temporarily unavailable, retry the request.
// The payload contains a human-readable message explaining what caused the unavailability.
TemporarilyUnavailable : text;
};

type LedgerError = variant {
Expand Down
5 changes: 3 additions & 2 deletions rs/ethereum/cketh/minter/src/endpoints/ckerc20.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl From<Erc20WithdrawalRequest> for RetrieveErc20Request {
}
}

#[derive(CandidType, Deserialize, Debug, PartialEq)]
#[derive(CandidType, Deserialize, Clone, Debug, PartialEq)]
pub enum WithdrawErc20Error {
TokenNotSupported {
supported_tokens: Vec<crate::endpoints::CkErc20Token>,
Expand All @@ -39,9 +39,10 @@ pub enum WithdrawErc20Error {
cketh_block_index: Nat,
error: LedgerError,
},
TemporarilyUnavailable(String),
}

#[derive(CandidType, Deserialize, Debug, PartialEq)]
#[derive(CandidType, Deserialize, Clone, Debug, PartialEq)]
pub enum LedgerError {
InsufficientFunds {
balance: Nat,
Expand Down
19 changes: 14 additions & 5 deletions rs/ethereum/cketh/minter/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ use ic_cketh_minter::state::transactions::{
Erc20WithdrawalRequest, EthWithdrawalRequest, Reimbursed, ReimbursementRequest,
};
use ic_cketh_minter::state::{lazy_call_ecdsa_public_key, mutate_state, read_state, State, STATE};
use ic_cketh_minter::tx::lazy_refresh_gas_fee_estimate;
use ic_cketh_minter::withdraw::{
process_reimbursement, process_retrieve_eth_requests, CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT,
process_reimbursement, process_retrieve_eth_requests, CKERC20_WITHDRAWAL_TRANSACTION_GAS_LIMIT,
CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT,
};
use ic_cketh_minter::{endpoints, erc20};
use ic_cketh_minter::{
Expand Down Expand Up @@ -316,7 +318,9 @@ async fn withdraw_erc20(
}
})?;
let cketh_ledger = read_state(LedgerClient::cketh_ledger_from_state);
let erc20_tx_fee = estimate_erc20_transaction_fee();
let erc20_tx_fee = estimate_erc20_transaction_fee().await.ok_or_else(|| {
WithdrawErc20Error::TemporarilyUnavailable("Failed to retrieve current gas fee".to_string())
})?;
let now = ic_cdk::api::time();
log!(INFO, "[withdraw_erc20]: burning {:?} ckETH", erc20_tx_fee);
match cketh_ledger
Expand Down Expand Up @@ -411,9 +415,14 @@ async fn withdraw_erc20(
}
}

fn estimate_erc20_transaction_fee() -> Wei {
//TODO XC-58: better fee estimation
read_state(|s| s.minimum_withdrawal_amount)
async fn estimate_erc20_transaction_fee() -> Option<Wei> {
lazy_refresh_gas_fee_estimate()
.await
.map(|gas_fee_estimate| {
gas_fee_estimate
.to_price(CKERC20_WITHDRAWAL_TRANSACTION_GAS_LIMIT)
.max_transaction_fee()
})
}

#[query]
Expand Down
54 changes: 28 additions & 26 deletions rs/ethereum/cketh/minter/src/state/transactions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,10 @@ pub enum CreateTransactionError {

#[derive(Clone, Debug, Eq, PartialEq)]
pub enum ResubmitTransactionError {
InsufficientTransactionAmount {
InsufficientTransactionFee {
ledger_burn_index: LedgerBurnIndex,
transaction_nonce: TransactionNonce,
transaction_amount: Wei,
allowed_max_transaction_fee: Wei,
max_transaction_fee: Wei,
},
}
Expand Down Expand Up @@ -373,6 +373,26 @@ impl EthTransactions {
self.reimbursed.values().cloned().collect()
}

fn find_reimbursed_transaction_by_cketh_ledger_burn_index(
&self,
searched_burn_index: &LedgerBurnIndex,
) -> Option<&Reimbursed> {
self.reimbursed
.iter()
.find_map(|(index, value)| match index {
ReimbursementIndex::CkEth { ledger_burn_index }
if ledger_burn_index == searched_burn_index =>
{
Some(value)
}
ReimbursementIndex::CkErc20 {
cketh_ledger_burn_index,
..
} if cketh_ledger_burn_index == searched_burn_index => Some(value),
_ => None,
})
}

pub fn record_withdrawal_request<R: Into<WithdrawalRequest>>(&mut self, request: R) {
let request = request.into();
let burn_index = request.cketh_ledger_burn_index();
Expand Down Expand Up @@ -537,10 +557,10 @@ impl EthTransactions {
actual_max_transaction_fee,
}) => {
transactions_to_resubmit.push(Err(
ResubmitTransactionError::InsufficientTransactionAmount {
ResubmitTransactionError::InsufficientTransactionFee {
ledger_burn_index: *burn_index,
transaction_nonce: *nonce,
transaction_amount: allowed_max_transaction_fee,
allowed_max_transaction_fee,
max_transaction_fee: actual_max_transaction_fee,
},
));
Expand Down Expand Up @@ -636,23 +656,6 @@ impl EthTransactions {
}
}
WithdrawalRequest::CkErc20(request) => {
if let Some(unused_tx_fee) = request
.max_transaction_fee
.checked_sub(finalized_tx.transaction_price().max_transaction_fee())
{
if unused_tx_fee > Wei::ZERO {
self.record_reimbursement_request(
ReimbursementIndex::CkEth { ledger_burn_index },
ReimbursementRequest {
ledger_burn_index,
to: request.from,
to_subaccount: request.from_subaccount.clone(),
reimbursed_amount: unused_tx_fee.change_units(),
transaction_hash: Some(receipt.transaction_hash),
},
);
}
}
if receipt.status == TransactionStatus::Failure {
self.record_reimbursement_request(
ReimbursementIndex::CkErc20 {
Expand Down Expand Up @@ -693,7 +696,7 @@ impl EthTransactions {
let reimbursement_request = self
.reimbursement_requests
.remove(&index)
.expect("BUG: missing reimbursement request");
.unwrap_or_else(|| panic!("BUG: missing reimbursement request with index {index:?}"));
let burn_in_block = index.burn_in_block();
assert_eq!(
self.reimbursed.insert(
Expand Down Expand Up @@ -727,16 +730,15 @@ impl EthTransactions {
}

if let Some(tx) = self.finalized_tx.get_alt(burn_index) {
if let Some(reimbursed) = self.reimbursed.get(&ReimbursementIndex::CkEth {
ledger_burn_index: *burn_index,
}) {
if let Some(reimbursed) =
self.find_reimbursed_transaction_by_cketh_ledger_burn_index(burn_index)
{
return RetrieveEthStatus::TxFinalized(TxFinalizedStatus::Reimbursed {
reimbursed_in_block: reimbursed.reimbursed_in_block.get().into(),
transaction_hash: tx.transaction_hash().to_string(),
reimbursed_amount: reimbursed.reimbursed_amount.into(),
});
}
//TODO XC-78: status for pending transactioon fee reimbursement for ckERC20?
if tx.transaction_status() == &TransactionStatus::Failure {
return RetrieveEthStatus::TxFinalized(TxFinalizedStatus::PendingReimbursement(
EthTransaction {
Expand Down

0 comments on commit fd03bd3

Please sign in to comment.