feat(cycles-minting): Cycles Minting canister refunds automatically. #3484

daniel-wong-dfinity-org · 2025-01-16T18:06:57Z

This happens when the memo (or icrc1_memo) in an ICP transfer is not one of the special values that CMC recognizes as signaling the purpose of the transfer.

This behavior gets triggered when one of the notify_* methods gets called.

The new behavior is not actually active yet. To turn it on, we simply need to set IS_AUTOMATIC_REFUND_ENABLED to true.

Motivation

Avoids ICP getting stuck, which can result from people making mistakes when sending ICP to the CMC.

Implementation Overview

Added issue_automatic_refund_if_memo_not_offerred. As indicated by the name, this is the heart of the implementation of this new functionality. This is called from fetch_transaction. The call is guarded behind the aforementioned IS_AUTOMATIC_REFUND_ENABLED flag.

Added MEANINGFUL_MEMOS constant. This is used to detect when automatic refund should be performed.

Minor Changes

Added AutomaticallyRefunded to NotificationStatus. This ensures that we do NOT duplicate refunds.

Changed one parameter of fetch_transaction from AccountIdentifier to Subaccount, because you cannot go "backwards" from AccountIdentifier to Subaccount.

Factored out the memo + icrc1_memo unifying code which used to live in transaction_has_expected_memo. Now, that code lives in a new get_u64_memo function.

Future Work

Added a couple of other helpers. One is set_block_status_to_processing. This could be used to reduce code duplication in notify_* methods, but such refactoring is left to future PRs.

References

We promised to do this in this forum post.

The other feature that we promised in that thread (use icrc1_memo) was implemented in an earlier PR.

…existing tests still pass, but still need new tests.

…nately, my technique does not seem to cause any interleaving. Will ask for help...

github-actions

If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).

To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:

Done.
No canister behavior changes.

New functionality is disabled via a flag. It will be enabled once security team reviews.

rs/nns/integration_tests/src/cycles_minting_canister.rs

…ngress_as." This reverts commit bc6492c.

aterga · 2025-01-21T16:13:37Z

rs/nns/cmc/src/lib.rs

+//
+// We promise that we will NEVER use 0 as one of these values. (This would be
+// very bad, because if no memo is explicitly supplied, the Transaction might
+// implicitly have 0 in the memo field.)


Why would it be bad, though?

aterga · 2025-01-21T16:17:11Z

rs/nns/cmc/src/lib.rs

 pub const MEMO_CREATE_CANISTER: Memo = Memo(0x41455243); // == 'CREA'
 pub const MEMO_TOP_UP_CANISTER: Memo = Memo(0x50555054); // == 'TPUP'
 pub const MEMO_MINT_CYCLES: Memo = Memo(0x544e494d); // == 'MINT'

+// New values might be added to this later. Do NOT assume that values won't be
+// added to this array later.
+pub const MEANINGFUL_MEMOS: [Memo; 3] =


Why can't we use lazy_static to define these values in a hash map?

Something like

lazy_static! { RESERVED_MEMOS: BTreeMap<String, Memo> = btreemap! { "CREATE_CANISTER".to_string() => Memo(0x41455243), // ... }; }

aterga · 2025-01-21T16:19:02Z

rs/nns/cmc/src/main.rs

@@ -139,6 +139,9 @@ pub enum NotificationStatus {
    NotifiedCreateCanister(Result<CanisterId, NotifyError>),
    /// The cached result of a completed cycles mint.
    NotifiedMint(NotifyMintCyclesResult),
+    /// The transaction did not have a supported memo (or icrc1_memo).
+    /// Therefore, we decided to send the ICP back to its source (minus fee).
+    AutomaticallyRefunded(Option<BlockIndex>),


Nit.

AutomaticallyRefunded seems like the consequence of not having a supported memo.

Should the status be called MissingMemo?

Suggested change

AutomaticallyRefunded(Option<BlockIndex>),

MissingMemo(Option<BlockIndex>),

Could you specify in the doc string what the argument Option<BlockIndex> represents?

It's not "MissingMemo" but rather "BadMemo".

I agree that "AutomaticallyRefunded" is the consequence, and therefore does not tell the user how to fix their next request. If it told the user what the problem was, it would be fine.

aterga · 2025-01-21T16:22:05Z

rs/nns/cmc/src/main.rs

+    // to interleave. In such cases, we want subsequent operations to see that
+    // an operation is already in flight. Therefore, before making any canister
+    // calls, we check that the block does not already have a status, and set
+    // its status to Processing. Only then do we proceed with calling the other


Super helpful comment, thanks for adding it!

Only then ...

Should is be mentioned what happens otherwise?

max-dfinity · 2025-01-21T22:49:06Z

rs/nns/integration_tests/src/cycles_minting_canister.rs

+    // understanding of how many canisters there are originally.
+    assert_canister_statuses_fixed("start");
+
+    let assert_balance = |nominal_amount_tokens: u64, fee_count: u64, narrative_phase: &str| {


nit: could "narrative_phase" be renamed to "test_phase" or something? There are a couple steps between "test" and "narrative" as concepts.

max-dfinity · 2025-01-21T22:50:25Z

rs/nns/integration_tests/src/cycles_minting_canister.rs

+    // Step 2: Run code under test.
+
+    // Step 2.1: Send ICP from USER1 to CMC, but use a garbage memo. There is no
+    // immediate explosion; "the bomb is only being planted" so to speak.


this language sounds rather alarming, perhaps incendiary :)

It also sounds like a bad memo is much more exciting than it is

max-dfinity · 2025-01-21T23:30:29Z

rs/nns/cmc/src/main.rs

+    incoming_block_index: BlockIndex,
+    // This is needed because transaction only has an AccountIdentifier.
+    // Although it is possible to go from PrincipalId + Subaccount to
+    // AccountIdentifier, the reverse is not possible. This is a super confusing


This commentary "super confusing" is not really useful to have in the code here.

max-dfinity

LGTM, other than small changes about commentary in code comments

github-actions bot added the feat label Jan 16, 2025

daniel-wong-dfinity-org force-pushed the cmc-automatic-refund-daniel-wong branch 2 times, most recently from ec54c97 to 76796dc Compare January 17, 2025 13:52

Implemented automatic refund in Cycles Minting canister. Builds, and …

300b362

…existing tests still pass, but still need new tests.

daniel-wong-dfinity-org force-pushed the cmc-automatic-refund-daniel-wong branch from 76796dc to 300b362 Compare January 17, 2025 13:53

Some tests. Also, simplified set_block_status_to_processing.

66544fc

daniel-wong-dfinity-org force-pushed the cmc-automatic-refund-daniel-wong branch from 3535075 to 66544fc Compare January 17, 2025 15:18

daniel-wong-dfinity-org added 4 commits January 17, 2025 16:19

docstring

88d9253

more test

3fc1cae

lint

e6eb70e

lint again

54d8d29

daniel-wong-dfinity-org force-pushed the cmc-automatic-refund-daniel-wong branch from f7ade83 to dc2e92c Compare January 20, 2025 17:16

Test CMC automatic refund.

9291d8d

daniel-wong-dfinity-org force-pushed the cmc-automatic-refund-daniel-wong branch from dc2e92c to 9291d8d Compare January 20, 2025 17:20

daniel-wong-dfinity-org added 4 commits January 20, 2025 20:09

Adjust test, depending on whether automatic refunds are enabled.

0536eb2

lint

247e73e

Strengthing test by making many calls to the code under test. Unfortu…

ff02f49

…nately, my technique does not seem to cause any interleaving. Will ask for help...

lint

3be81cc

daniel-wong-dfinity-org force-pushed the cmc-automatic-refund-daniel-wong branch from f50d3b7 to 3be81cc Compare January 20, 2025 20:11

daniel-wong-dfinity-org marked this pull request as ready for review January 20, 2025 20:12

daniel-wong-dfinity-org requested a review from a team as a code owner January 20, 2025 20:12

github-actions bot previously requested changes Jan 20, 2025

View reviewed changes

github-actions bot added the @nns-team label Jan 20, 2025

daniel-wong-dfinity-org commented Jan 20, 2025

View reviewed changes

rs/nns/integration_tests/src/cycles_minting_canister.rs Outdated Show resolved Hide resolved

daniel-wong-dfinity-org mentioned this pull request Jan 20, 2025

feat(cycles-minting-canister): When memo does not match, fall back to icrc1_memo. #3336

Merged

lint

d9dd565

daniel-wong-dfinity-org enabled auto-merge January 20, 2025 21:07

daniel-wong-dfinity-org added 2 commits January 21, 2025 12:07

minor comment change

8a76367

Do not set status of block to Processing if it is too old.

b6f0fb2

daniel-wong-dfinity-org added 4 commits January 21, 2025 13:27

comment

928da52

Trying and failing to replace send_ingress_safe with submit_ingress_as.

bc6492c

lint

d72841c

Revert "Trying and failing to replace send_ingress_safe with submit_i…

eb682c1

…ngress_as." This reverts commit bc6492c.

aterga reviewed Jan 21, 2025

View reviewed changes

max-dfinity reviewed Jan 21, 2025

View reviewed changes

max-dfinity approved these changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cycles-minting): Cycles Minting canister refunds automatically. #3484

feat(cycles-minting): Cycles Minting canister refunds automatically. #3484

daniel-wong-dfinity-org commented Jan 16, 2025 •

edited

Loading

github-actions bot left a comment

aterga Jan 21, 2025

aterga Jan 21, 2025

aterga Jan 21, 2025

aterga Jan 21, 2025

max-dfinity Jan 21, 2025

aterga Jan 21, 2025

max-dfinity Jan 21, 2025

max-dfinity Jan 21, 2025

max-dfinity Jan 21, 2025

max-dfinity left a comment

	AutomaticallyRefunded(Option<BlockIndex>),
	MissingMemo(Option<BlockIndex>),

feat(cycles-minting): Cycles Minting canister refunds automatically. #3484

Are you sure you want to change the base?

feat(cycles-minting): Cycles Minting canister refunds automatically. #3484

Conversation

daniel-wong-dfinity-org commented Jan 16, 2025 • edited Loading

Motivation

Implementation Overview

Minor Changes

Future Work

References

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-dfinity left a comment

Choose a reason for hiding this comment

daniel-wong-dfinity-org commented Jan 16, 2025 •

edited

Loading