fix: SDK-1341: Duplicate request error messages

.github/workflows/ci.yml

@@ -6,7 +6,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        ic-commit: [ a17247bd86c7aa4e87742bf74d108614580f216d ]
+        ic-commit: [ fed4316368b93245120ceb0596423c715bb31ff0 ]
 
     steps:
       - uses: actions/checkout@v2

cycles-ledger/src/storage.rs

@@ -766,6 +766,19 @@ pub fn transfer(
 ) -> Result<Nat, TransferFromError> {
     use TransferFromError::*;
 
+    let transaction = Transaction {
+        operation: Operation::Transfer {
+            from,
+            to,
+            spender,
+            amount,
+            fee: suggested_fee,
+        },
+        created_at_time,
+        memo,
+    };
+    check_duplicate(&transaction)?;
+
     // check that no account is owned by a denied principal
     if is_denied_account_owner(&from.owner) {
         return Err(transfer_from::denied_owner(from));
@@ -812,18 +825,6 @@ pub fn transfer(
         })
         .map_err(transfer_from::anyhow_error)?;
 
-    let transaction = Transaction {
-        operation: Operation::Transfer {
-            from,
-            to,
-            spender,
-            amount,
-            fee: suggested_fee,
-        },
-        created_at_time,
-        memo,
-    };
-
     let block_index = process_transaction(transaction.clone(), now)?;
 
     // The operations below should not return an error because of the checks
@@ -867,6 +868,20 @@ pub fn approve(
     expected_allowance: Option<u128>,
     expires_at: Option<u64>,
 ) -> Result<Nat, ApproveError> {
+    let transaction = Transaction {
+        operation: Operation::Approve {
+            from,
+            spender,
+            amount,
+            expected_allowance,
+            expires_at,
+            fee: suggested_fee,
+        },
+        created_at_time,
+        memo,
+    };
+    check_duplicate(&transaction)?;
+
     // check that this isn't a self approval
     if from.owner == spender.owner {
         ic_cdk::trap("self approval is not allowed");
@@ -911,19 +926,6 @@ pub fn approve(
         })
         .map_err(approve::anyhow_error)?;
 
-    let transaction = Transaction {
-        operation: Operation::Approve {
-            from,
-            spender,
-            amount,
-            expected_allowance,
-            expires_at,
-            fee: suggested_fee,
-        },
-        created_at_time,
-        memo,
-    };
-
     let block_index = process_transaction(transaction.clone(), now)?;
 
     // The operations below should not return an error because of the checks
@@ -1238,6 +1240,31 @@ fn validate_suggested_fee(op: &Operation) -> Result<Option<u128>, u128> {
     }
 }
 
+fn check_duplicate(transaction: &Transaction) -> Result<(), ProcessTransactionError> {
+    use ProcessTransactionError as PTErr;
+    // sanity check that the transaction can be hashed
+    let tx_hash = match transaction.hash() {
+        Ok(tx_hash) => tx_hash,
+        Err(err) => {
+            let err = err.context(format!("Unable to process transaction {:?}", transaction));
+            log!(P0, "{:#}", err);
+            return Err(PTErr::from(err));
+        }
+    };
+
+    // check whether transaction is a duplicate
+    if let Some((block_index, maybe_canister)) =
+        read_state(|state| state.transaction_hashes.get(&tx_hash))
+    {
+        return Err(PTErr::Duplicate {
+            duplicate_of: block_index,
+            canister_id: maybe_canister.map(Into::into),
+        });
+    }
+
+    Ok(())
+}
+
 fn process_transaction(transaction: Transaction, now: u64) -> Result<u64, ProcessTransactionError> {
     process_block(transaction, now, None)
 }
@@ -1261,25 +1288,7 @@ fn process_block(
         .map(|fee| effective_fee.or(fee))
         .map_err(|expected_fee| PTErr::BadFee { expected_fee })?;
 
-    // sanity check that the transaction can be hashed
-    let tx_hash = match transaction.hash() {
-        Ok(tx_hash) => tx_hash,
-        Err(err) => {
-            let err = err.context(format!("Unable to process transaction {:?}", transaction));
-            log!(P0, "{:#}", err);
-            return Err(PTErr::from(err));
-        }
-    };
-
-    // check whether transaction is a duplicate
-    if let Some((block_index, maybe_canister)) =
-        read_state(|state| state.transaction_hashes.get(&tx_hash))
-    {
-        return Err(PTErr::Duplicate {
-            duplicate_of: block_index,
-            canister_id: maybe_canister.map(Into::into),
-        });
-    }
+    check_duplicate(&transaction)?;
 
     let block = Block {
         transaction,
@@ -1457,6 +1466,13 @@ pub async fn send(
         return Err(InsufficientFunds { balance: balance_of(&from).into() });
     };
 
+    let transaction = Transaction {
+        operation: Operation::Burn { from, amount },
+        created_at_time,
+        memo: Some(encode_send_memo(&to)),
+    };
+    check_duplicate(&transaction)?;
+
     // check that the `from` account has enough funds
     read_state(|state| state.check_debit_from_account(&from, amount_with_fee)).map_err(
         |balance| InsufficientFunds {
@@ -1476,12 +1492,6 @@ pub async fn send(
 
     // 1. burn cycles + fee
 
-    let transaction = Transaction {
-        operation: Operation::Burn { from, amount },
-        created_at_time,
-        memo: Some(encode_send_memo(&to)),
-    };
-
     let block_index = process_block(transaction.clone(), now, Some(config::FEE))?;
 
     if let Err(err) = mutate_state(|state| state.debit(&from, amount_with_fee)) {
@@ -1559,6 +1569,13 @@ pub async fn create_canister(
 ) -> Result<CreateCanisterSuccess, CreateCanisterError> {
     use CreateCanisterError::*;
 
+    let transaction = Transaction {
+        operation: Operation::Burn { from, amount },
+        created_at_time,
+        memo: Some(Memo::from(ByteBuf::from(CREATE_CANISTER_MEMO))),
+    };
+    check_duplicate(&transaction)?;
+
     // 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 { balance: balance_of(&from).into() });
@@ -1583,12 +1600,6 @@ pub async fn create_canister(
 
     // 1. burn cycles + fee
 
-    let transaction = Transaction {
-        operation: Operation::Burn { from, amount },
-        created_at_time,
-        memo: Some(Memo::from(ByteBuf::from(CREATE_CANISTER_MEMO))),
-    };
-
     let block_index = process_block(transaction.clone(), now, Some(config::FEE))?;
 
     if let Err(err) = mutate_state(|state| state.debit(&from, amount_with_fee)) {