feat(cycles-minting-canister): When memo does not match, fall back to…

… icrc1_memo. (#3336)

# References

This implements the first of two features mentioned in [this forum
thread][thread].

[thread]:
https://forum.dfinity.org/t/extend-cycles-minting-canister-functionality/37749

rs/nns/cmc/BUILD.bazel

@@ -50,6 +50,14 @@ BUILD_DEPENDENCIES = [
     "@crate_index//:build-info-build",
 ]
 
+DEV_DEPENDENCIES = [
+    # Keep sorted.
+    "//rs/types/types_test_utils",
+    "@crate_index//:candid_parser",
+    "@crate_index//:futures",
+    "@crate_index//:serde_bytes",
+]
+
 ALIASES = {}
 
 cargo_build_script(
@@ -86,7 +94,7 @@ rust_canister(
 rust_test(
     name = "cmc_test",
     crate = ":cmc",
-    deps = DEPENDENCIES + ["@crate_index//:futures"],
+    deps = DEPENDENCIES + DEV_DEPENDENCIES,
 )
 
 rust_test(
@@ -96,10 +104,5 @@ rust_test(
     env = {
         "CARGO_MANIFEST_DIR": "rs/nns/cmc",
     },
-    deps = [
-        # Keep sorted.
-        "//rs/types/types_test_utils",
-        "@crate_index//:candid_parser",
-        "@crate_index//:futures",
-    ],
+    deps = DEPENDENCIES + DEV_DEPENDENCIES,
 )

rs/nns/cmc/Cargo.toml

@@ -47,6 +47,7 @@ yansi = "0.5.0"
 candid_parser = { workspace = true }
 futures = { workspace = true }
 ic-types-test-utils = { path = "../../types/types_test_utils" }
+serde_bytes = { workspace = true }
 
 [[bin]]
 name = "cycles-minting-canister"

rs/nns/cmc/src/main.rs

@@ -33,7 +33,7 @@ use ic_nns_constants::{
 use ic_types::{CanisterId, Cycles, PrincipalId, SubnetId};
 use icp_ledger::{
     AccountIdentifier, Block, BlockIndex, BlockRes, CyclesResponse, Memo, Operation, SendArgs,
-    Subaccount, Tokens, TransactionNotification, DEFAULT_TRANSFER_FEE,
+    Subaccount, Tokens, Transaction, TransactionNotification, DEFAULT_TRANSFER_FEE,
 };
 use icrc_ledger_types::icrc1::account::Account;
 use lazy_static::lazy_static;
@@ -1569,6 +1569,62 @@ fn memo_to_intent_str(memo: Memo) -> String {
     }
 }
 
+/// Returns Ok if transaction matches expected_memo.
+///
+/// First, looks at the memo field. If that does not match, falls back to the
+/// icrc1_field. If that is of length 8, converts assuming little endian, and if
+/// that matches, returns Ok.
+fn transaction_has_expected_memo(
+    transaction: &Transaction,
+    expected_memo: Memo,
+) -> Result<(), NotifyError> {
+    fn stringify_memo(memo: Memo) -> String {
+        format!("{} ({})", memo_to_intent_str(memo), memo.0)
+    }
+
+    if transaction.memo == expected_memo {
+        return Ok(());
+    }
+
+    // Fall back to icrc1_memo.
+
+    // Read the field.
+    let Some(icrc1_memo) = &transaction.icrc1_memo else {
+        return Err(NotifyError::InvalidTransaction(format!(
+            "The transaction's memo ({}) does not have the required value ({}).",
+            stringify_memo(transaction.memo),
+            stringify_memo(expected_memo),
+        )));
+    };
+
+    // Convert it to Memo.
+    type U64Array = [u8; std::mem::size_of::<u64>()];
+    let observed_icrc1_memo = U64Array::try_from(icrc1_memo.as_ref()).map_err(|_err| {
+        NotifyError::InvalidTransaction(format!(
+            "The transaction's memo ({}) does not have the required value ({}).",
+            stringify_memo(transaction.memo),
+            stringify_memo(expected_memo),
+        ))
+    })?;
+    let observed_icrc1_memo = Memo(u64::from_le_bytes(observed_icrc1_memo));
+
+    // Compare to the required value.
+    if observed_icrc1_memo == expected_memo {
+        return Ok(());
+    }
+
+    Err(NotifyError::InvalidTransaction(format!(
+        "Neither the memo ({}) nor the icrc1_memo ({}) of the transaction \
+         has the required value ({}).",
+        stringify_memo(transaction.memo),
+        stringify_memo(observed_icrc1_memo),
+        stringify_memo(expected_memo),
+    )))
+}
+
+/// Returns amount, and source of the transfer in (ICP) ledger.
+///
+/// Returns Ok if the arguments are matched. (Otherwise, returns Err).
 async fn fetch_transaction(
     block_index: BlockIndex,
     expected_destination_account: AccountIdentifier,
@@ -1588,22 +1644,15 @@ async fn fetch_transaction(
             ))
         }
     };
+
     if to != expected_destination_account {
         return Err(NotifyError::InvalidTransaction(format!(
             "Destination account in the block ({}) different than in the notification ({})",
             to, expected_destination_account
         )));
     }
-    let memo = block.transaction().memo;
-    if memo != expected_memo {
-        return Err(NotifyError::InvalidTransaction(format!(
-            "Intent in the block ({} == {}) different than in the notification ({} == {})",
-            memo.0,
-            memo_to_intent_str(memo),
-            expected_memo.0,
-            memo_to_intent_str(expected_memo),
-        )));
-    }
+
+    transaction_has_expected_memo(block.transaction().as_ref(), expected_memo)?;
 
     Ok((amount, from))
 }
@@ -2443,6 +2492,7 @@ mod tests {
     use super::*;
     use ic_types_test_utils::ids::{subnet_test_id, user_test_id};
     use rand::Rng;
+    use serde_bytes::ByteBuf;
     use std::str::FromStr;
 
     pub(crate) fn init_test_state() {
@@ -3158,4 +3208,172 @@ mod tests {
         )
         .expect("The CMC canister interface is not compatible with the cmc.did file");
     }
+
+    #[test]
+    fn test_transaction_has_expected_memo_happy() {
+        // Not relevant to this test.
+        let operation = Operation::Mint {
+            to: AccountIdentifier::new(PrincipalId::new_user_test_id(668_857_347), None),
+            amount: Tokens::from_e8s(123_456),
+        };
+
+        // Case A: Legacy memo is used.
+        let transaction_with_legacy_memo = Transaction {
+            memo: Memo(42),
+            icrc1_memo: None,
+
+            // Irrelevant to this test.
+            operation: operation.clone(),
+            created_at_time: None,
+        };
+
+        assert_eq!(
+            transaction_has_expected_memo(&transaction_with_legacy_memo, Memo(42),),
+            Ok(()),
+        );
+
+        // Case B: When the user uses icrc1's memo to indicate the purpose of
+        // the transfer, and as a result the legacy memo field is implicitly set
+        // to 0.
+        let transaction_with_icrc1_memo = Transaction {
+            memo: Memo(0),
+            icrc1_memo: Some(ByteBuf::from(43_u64.to_le_bytes().to_vec())),
+
+            // Irrelevant to this test.
+            operation: operation.clone(),
+            created_at_time: None,
+        };
+
+        assert_eq!(
+            transaction_has_expected_memo(&transaction_with_icrc1_memo, Memo(43),),
+            Ok(()),
+        );
+    }
+
+    #[test]
+    fn test_transaction_has_expected_memo_sad() {
+        // Not relevant to this test.
+        let operation = Operation::Mint {
+            to: AccountIdentifier::new(PrincipalId::new_user_test_id(668_857_347), None),
+            amount: Tokens::from_e8s(123_456),
+        };
+
+        // Case A: Legacy memo is used.
+        {
+            let transaction = Transaction {
+                memo: Memo(77),
+                icrc1_memo: None,
+
+                // Irrelevant to this test.
+                operation: operation.clone(),
+                created_at_time: None,
+            };
+            let result = transaction_has_expected_memo(&transaction, Memo(42));
+
+            let original_err = match result {
+                Err(NotifyError::InvalidTransaction(err)) => err,
+                wrong => panic!("{:?}", wrong),
+            };
+
+            let lower_err = original_err.to_lowercase();
+            for key_word in ["memo", "77", "42"] {
+                assert!(
+                    lower_err.contains(key_word),
+                    "{} not in {:?}",
+                    key_word,
+                    original_err
+                );
+            }
+        }
+
+        // Case B: When the user uses icrc1's memo to indicate the purpose of
+        // the transfer, and as a result the legacy memo field is implicitly set
+        // to 0.
+        {
+            let transaction = Transaction {
+                memo: Memo(0),
+                icrc1_memo: Some(ByteBuf::from(78_u64.to_le_bytes().to_vec())),
+
+                // Irrelevant to this test.
+                operation: operation.clone(),
+                created_at_time: None,
+            };
+
+            let result = transaction_has_expected_memo(&transaction, Memo(42));
+
+            let original_err = match result {
+                Err(NotifyError::InvalidTransaction(err)) => err,
+                wrong => panic!("{:?}", wrong),
+            };
+
+            let lower_err = original_err.to_lowercase();
+            for key_word in ["memo", "78", "42"] {
+                assert!(
+                    lower_err.contains(key_word),
+                    "{} not in {:?}",
+                    key_word,
+                    original_err
+                );
+            }
+        }
+
+        // Case C: icrc1's memo is used, but is not of length 8, and we
+        // therefore do not consider it to contain a (little endian) u64.
+        {
+            let transaction = Transaction {
+                memo: Memo(0),
+                icrc1_memo: Some(ByteBuf::from(vec![1, 2, 3])),
+
+                // Irrelevant to this test.
+                operation: operation.clone(),
+                created_at_time: None,
+            };
+
+            let result = transaction_has_expected_memo(&transaction, Memo(42));
+
+            let original_err = match result {
+                Err(NotifyError::InvalidTransaction(err)) => err,
+                wrong => panic!("{:?}", wrong),
+            };
+
+            let lower_err = original_err.to_lowercase();
+            for key_word in ["memo", "0", "42"] {
+                assert!(
+                    lower_err.contains(key_word),
+                    "{} not in {:?}",
+                    key_word,
+                    original_err
+                );
+            }
+        }
+
+        // Case D: legacy memo is 0, and ircr1_memo is None.
+        {
+            let transaction = Transaction {
+                memo: Memo(0),
+                icrc1_memo: None,
+
+                // Irrelevant to this test.
+                operation: operation.clone(),
+                created_at_time: None,
+            };
+
+            let result = transaction_has_expected_memo(&transaction, Memo(42));
+
+            let original_err = match result {
+                Err(NotifyError::InvalidTransaction(err)) => err,
+                wrong => panic!("{:?}", wrong),
+            };
+
+            let lower_err = original_err.to_lowercase();
+            for key_word in ["memo", "0", "42"] {
+                assert!(
+                    lower_err.contains(key_word),
+                    "{} not in {:?}",
+                    key_word,
+                    original_err
+                );
+            }
+        }
+    }
 }