diff --git a/.gitignore b/.gitignore index bb6b6f4..29fb0f5 100644 --- a/.gitignore +++ b/.gitignore @@ -9,8 +9,9 @@ target/ # MSVC Windows builds of rustc generate these, which store debugging information *.pdb -# Ignore the dfx build artifacts +# Ignore build artifacts .dfx +**/.icp/cache/ # Ignore state machine binary ic-test-state-machine diff --git a/CHANGELOG.md b/CHANGELOG.md index 66b2cf9..1ec0f71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ## [Unreleased] - ReleaseDate * Add support for canister settings `wasm_memory_limit`, `wasm_memory_threshold`, `log_visibility`, and `environment_variables`. +* Fixed a bug where `InsufficientFunds` errors took precedence over `BadFee` errors in `icrc1_transfer`, `icrc2_approve`, and `icrc2_transfer_from`. ## [1.0.6] - 2025-09-19 * Add support for `initial_balances` in `InitArgs`. When specifying initial balances it is up to the installer to ensure that the cycles ledger has sufficient cycles available to spend these cycles. diff --git a/cycles-ledger/src/storage.rs b/cycles-ledger/src/storage.rs index de19a75..9733829 100644 --- a/cycles-ledger/src/storage.rs +++ b/cycles-ledger/src/storage.rs @@ -962,6 +962,13 @@ pub fn transfer( } } + // BadFee takes precedence over InsufficientFunds per ICRC-1 spec + if suggested_fee.is_some() && suggested_fee != Some(config::FEE) { + return Err(BadFee { + expected_fee: Nat::from(config::FEE), + }); + } + // if `amount` + `fee` overflows then the user doesn't have enough funds let Some(amount_with_fee) = amount.checked_add(config::FEE) else { return Err(InsufficientFunds { @@ -1074,6 +1081,13 @@ pub fn approve( }); } + // BadFee takes precedence over InsufficientFunds per ICRC-1 spec + if suggested_fee.is_some() && suggested_fee != Some(config::FEE) { + return Err(ApproveError::BadFee { + expected_fee: Nat::from(config::FEE), + }); + } + // check that the `from` account has enough funds to pay the fee let balance = balance_of(&from); if balance < config::FEE { @@ -1116,9 +1130,6 @@ pub fn approve( #[derive(Debug)] enum ProcessTransactionError { - BadFee { - expected_fee: u128, - }, Duplicate { duplicate_of: u64, canister_id: Option, @@ -1148,9 +1159,6 @@ impl std::error::Error for ProcessTransactionError { impl std::fmt::Display for ProcessTransactionError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::BadFee { expected_fee } => { - write!(f, "Invalid fee, expected fee is {}", expected_fee) - } Self::Duplicate { duplicate_of, .. } => write!( f, "Input transaction is a duplicate of transaction at index {}", @@ -1325,9 +1333,6 @@ impl From for TransferFromError { use ProcessTransactionError::*; match error { - BadFee { expected_fee } => Self::BadFee { - expected_fee: Nat::from(expected_fee), - }, Duplicate { duplicate_of, .. } => Self::Duplicate { duplicate_of: Nat::from(duplicate_of), }, @@ -1343,7 +1348,6 @@ impl From for WithdrawFromError { use ProcessTransactionError::*; match error { - BadFee { .. } => ic_cdk::trap("BadFee should not happen for withdraw"), Duplicate { duplicate_of, .. } => Self::Duplicate { duplicate_of: Nat::from(duplicate_of), }, @@ -1366,9 +1370,6 @@ impl From for ApproveError { use ProcessTransactionError::*; match error { - BadFee { expected_fee } => Self::BadFee { - expected_fee: Nat::from(expected_fee), - }, Duplicate { duplicate_of, .. } => Self::Duplicate { duplicate_of: Nat::from(duplicate_of), }, @@ -1383,9 +1384,6 @@ impl From for WithdrawError { use ProcessTransactionError::*; match error { - BadFee { expected_fee } => Self::BadFee { - expected_fee: Nat::from(expected_fee), - }, Duplicate { duplicate_of, .. } => Self::Duplicate { duplicate_of: Nat::from(duplicate_of), }, @@ -1400,13 +1398,6 @@ impl From for CreateCanisterError { use ProcessTransactionError::*; match error { - BadFee { expected_fee } => Self::GenericError { - error_code: CreateCanisterError::BAD_FEE_ERROR.into(), - message: format!( - "BadFee. Expected fee: {}. Should never happen.", - expected_fee - ), - }, Duplicate { duplicate_of, canister_id, @@ -1425,13 +1416,6 @@ impl From for CreateCanisterFromError { use ProcessTransactionError::*; match error { - BadFee { expected_fee } => Self::GenericError { - error_code: CreateCanisterError::BAD_FEE_ERROR.into(), - message: format!( - "BadFee. Expected fee: {}. Should never happen.", - expected_fee - ), - }, Duplicate { duplicate_of, canister_id, @@ -1514,20 +1498,15 @@ impl From for CreateCanisterFromError { } } -// Validates the suggested fee and returns the effective fee. -// If the validation fails then return Err with the expected fee. -fn validate_suggested_fee(op: &Operation) -> Result, u128> { +/// Returns the effective fee for the operation. +/// Callers must validate the suggested fee before calling this function. +fn effective_fee(op: &Operation) -> Option { use Operation as Op; match op { - Op::Mint { .. } => Ok(Some(0)), - Op::Burn { .. } => Ok(Some(config::FEE)), - Op::Transfer { fee, .. } | Op::Approve { fee, .. } => { - if fee.is_some() && fee != &Some(config::FEE) { - return Err(config::FEE); - } - Ok(fee.is_none().then_some(config::FEE)) - } + Op::Mint { .. } => Some(0), + Op::Burn { .. } => Some(config::FEE), + Op::Transfer { fee, .. } | Op::Approve { fee, .. } => fee.is_none().then_some(config::FEE), } } @@ -1557,8 +1536,6 @@ fn check_duplicate(transaction: &Transaction) -> Result<(), ProcessTransactionEr } fn process_transaction(transaction: Transaction, now: u64) -> Result { - use ProcessTransactionError as PTErr; - // The ICRC-1 and ICP Ledgers trap when the memo validation fails // so we do the same. if let Err(err) = validate_memo(&transaction.memo) { @@ -1567,8 +1544,7 @@ fn process_transaction(transaction: Transaction, now: u64) -> Result