-
Notifications
You must be signed in to change notification settings - Fork 327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cycles-minting): Cycles Minting canister refunds automatically. #3484
base: master
Are you sure you want to change the base?
Conversation
ec54c97
to
76796dc
Compare
…existing tests still pass, but still need new tests.
76796dc
to
300b362
Compare
3535075
to
66544fc
Compare
f7ade83
to
dc2e92c
Compare
dc2e92c
to
9291d8d
Compare
…nately, my technique does not seem to cause any interleaving. Will ask for help...
f50d3b7
to
3be81cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This happens when the
memo
(oricrc1_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 fromfetch_transaction
. The call is guarded behind the aforementionedIS_AUTOMATIC_REFUND_ENABLED
flag.Added
MEANINGFUL_MEMOS
constant. This is used to detect when automatic refund should be performed.Minor Changes
Added
AutomaticallyRefunded
toNotificationStatus
. This ensures that we do NOT duplicate refunds.Changed one parameter of
fetch_transaction
fromAccountIdentifier
toSubaccount
, because you cannot go "backwards" fromAccountIdentifier
toSubaccount
.Factored out the
memo
+icrc1_memo
unifying code which used to live intransaction_has_expected_memo
. Now, that code lives in a newget_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 innotify_*
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.