ICRC-2: Approve and Transfer From #12

receronp · 2023-05-26T16:36:02Z

This PR adds the ICRC-2 Standard Approve and Transfer From methods (icrc2_approve, icrc2_transfer_from, icrc2_allowance) as specified in the DFINITY ICRC-2 Standard Methods. Methods from ICRC-1 are supported by reusing code from the ICRC-1 codebase, with icrc1_supported_standards modified to report name = "ICRC-2" and corresponding url.
Actor tests for the new standard methods have also been created, covering ApproveError and TransferFromError cases, as well as the examples defined in the DFINITY ICRC-2 Standard Examples.

receronp · 2023-05-29T19:52:48Z

Closing this PR in favor of #18

tomijaga · 2023-06-06T21:58:30Z

Hey @receronp,
I'd like to review the ICRC-2 before moving over to the ICRC-3 implementation.

receronp · 2023-06-19T19:03:27Z

Hi @tomijaga,
Sounds good, let me know if you have any suggestions.

tomijaga · 2023-07-02T00:49:24Z

Hey @receronp,
I've completed the review and added a few more comments.

Your implementation looks really good and it accurately implements the ICRC-2 standard.
However, there are some updates needed for storing permitted transfers and approvals.
Currently, permitted transfers are stored as icrc1 transfers, which means the spender field is not included, and successful approval requests are not being stored. I have provided the necessary changes to the Transaction type in a review comment so that approved requests can be stored.

Here's a link to the reference implementation for both the icrc1 and icrc2 standards. Let me know if you have any questions

receronp · 2023-07-06T17:16:37Z

Hi @tomijaga,
I've looked at the provided reference implementation and am working on the updates for the Transaction type to enable the spender field and successful approvals. I however cannot seem to find the review comments that you mention.
Could you share those once more?
Thanks for the help :)

tomijaga · 2023-06-10T18:56:58Z

tests/ICRC2/Archive.ActorTest.mo

The Archive test doesn't need to be duplicated for ICRC-2 since it wasn't updated in the implementation, and no new test cases were added.

tomijaga · 2023-06-22T14:45:28Z

src/ICRC2/Approve.mo

+        // TODO: Verify if approval fee should be validated as a transfer fee.
+        if (not Transfer.validate_fee(token, app_req.fee)) {
+            return #err(
+                #BadFee {
+                    expected_fee = token._fee;
+                }
+            );
+        };


For the approval fee, we can add a specific field in the TokenData that the user can set when the ICRC2.init() function is called.

I looked at the ICRC-2 standard again and it doesn't have a icrc2_approval_fee function. Adding a new fee might deviate from the standard so let's stick with your original decision to use the transfer fee.

tomijaga · 2023-06-22T14:56:13Z

src/ICRC2/lib.mo

+    public func init(args : T.InitArgs) : T.TokenData {
+        let {
+            name;
+            symbol;
+            decimals;
+            fee;
+            minting_account;
+            max_supply;
+            initial_balances;
+            min_burn_amount;
+            advanced_settings;
+        } = args;
+
+        var _burned_tokens = 0;
+        var permitted_drift = 60_000_000_000;
+        var transaction_window = 86_400_000_000_000;
+
+        switch (advanced_settings) {
+            case (?options) {
+                _burned_tokens := options.burned_tokens;
+                permitted_drift := Nat64.toNat(options.permitted_drift);
+                transaction_window := Nat64.toNat(options.transaction_window);
+            };
+            case (null) {};
+        };
+
+        if (not Account.validate(minting_account)) {
+            Debug.trap("minting_account is invalid");
+        };
+
+        let accounts : T.AccountBalances = StableTrieMap.new();
+        let approvals : T.ApprovalAllowances = StableTrieMap.new();
+
+        var _minted_tokens = _burned_tokens;
+
+        for ((i, (account, balance)) in Itertools.enumerate(initial_balances.vals())) {
+
+            if (not Account.validate(account)) {
+                Debug.trap(
+                    "Invalid Account: Account at index " # debug_show i # " is invalid in 'initial_balances'"
+                );
+            };
+
+            let encoded_account = Account.encode(account);
+
+            StableTrieMap.put(
+                accounts,
+                Blob.equal,
+                Blob.hash,
+                encoded_account,
+                balance,
+            );
+
+            _minted_tokens += balance;
+        };
+
+        {
+            name = name;
+            symbol = symbol;
+            decimals;
+            var _fee = fee;
+            max_supply;
+            var _minted_tokens = _minted_tokens;
+            var _burned_tokens = _burned_tokens;
+            min_burn_amount;
+            minting_account;
+            accounts;
+            approvals;
+            metadata = Utils.init_metadata(args);
+            supported_standards = Utils.init_standards();
+            transactions = SB.initPresized(MAX_TRANSACTIONS_IN_LEDGER);
+            permitted_drift;
+            transaction_window;
+            archive = {
+                var canister = actor ("aaaaa-aa");
+                var stored_txs = 0;
+            };
+        };
+    };


We can reduce the code here by calling the init function for ICRC1 and updating the result as needed

Suggested change

public func init(args : T.InitArgs) : T.TokenData {

let {

name;

symbol;

decimals;

fee;

minting_account;

max_supply;

initial_balances;

min_burn_amount;

advanced_settings;

} = args;

var _burned_tokens = 0;

var permitted_drift = 60_000_000_000;

var transaction_window = 86_400_000_000_000;

switch (advanced_settings) {

case (?options) {

_burned_tokens := options.burned_tokens;

permitted_drift := Nat64.toNat(options.permitted_drift);

transaction_window := Nat64.toNat(options.transaction_window);

};

case (null) {};

};

if (not Account.validate(minting_account)) {

Debug.trap("minting_account is invalid");

};

let accounts : T.AccountBalances = StableTrieMap.new();

let approvals : T.ApprovalAllowances = StableTrieMap.new();

var _minted_tokens = _burned_tokens;

for ((i, (account, balance)) in Itertools.enumerate(initial_balances.vals())) {

if (not Account.validate(account)) {

Debug.trap(

"Invalid Account: Account at index " # debug_show i # " is invalid in 'initial_balances'"

);

};

let encoded_account = Account.encode(account);

StableTrieMap.put(

accounts,

Blob.equal,

Blob.hash,

encoded_account,

balance,

);

_minted_tokens += balance;

};

{

name = name;

symbol = symbol;

decimals;

var _fee = fee;

max_supply;

var _minted_tokens = _minted_tokens;

var _burned_tokens = _burned_tokens;

min_burn_amount;

minting_account;

accounts;

approvals;

metadata = Utils.init_metadata(args);

supported_standards = Utils.init_standards();

transactions = SB.initPresized(MAX_TRANSACTIONS_IN_LEDGER);

permitted_drift;

transaction_window;

archive = {

var canister = actor ("aaaaa-aa");

var stored_txs = 0;

};

};

};

public func init(args : T.InitArgs) : T.TokenData {

let icrc1_token = ICRC1.init(args);

let approvals : T.ApprovalAllowances = StableTrieMap.new();

return { icrc1_token with approvals }

};

tomijaga · 2023-06-22T15:58:29Z

src/ICRC2/Approve.mo

+        // TODO: Verify if approval memo should be validated for approvals.
+        if (not Transfer.validate_memo(app_req.memo)) {
+            return #err(
+                #GenericError({
+                    error_code = 0;
+                    message = "Memo must not be more than 32 bytes";
+                })
+            );
+        };


Yh, validating the memo here is fine. Ideally, an approval request should function under the same conditions as a transfer request.

tomijaga · 2023-06-22T16:11:19Z

src/ICRC2/Approve.mo

+    };
+
+    /// Checks if an approve request is valid
+    public func validate_request(


We can update this to prevent the minting account from making approval requests or being included as the spender in approval requests.

After giving it more thought, I think it would be best to leave this decision to the token owner. I currently can't think of a use case where the minting_account permits other accounts to mint tokens but some may come up in the future.

tomijaga · 2023-07-01T16:52:53Z

src/ICRC2/Types.mo

+
+    public type TransferArgs = Types1.TransferArgs;
+
+    public type Transfer = Types1.Transfer;


Let's update the Transfer type so that it stores the additional spender field from a transfer_from request. I think the spender field should be optional to differentiate between normal and permitted transfers.

tomijaga · 2023-07-01T17:00:25Z

src/ICRC2/Types.mo

+    /// Internal representation of a transaction request
+    public type TransactionRequest = Types1.TransactionRequest;
+
+    public type Transaction = Types1.Transaction;


Let's update the Transaction type with an approve field so the canister can store successful approval requests

public type Transaction = Types1.Transaction and { approve : ?Approve }; public type Approve = { from : Account; spender : Account; amount : Balance; expected_allowance : ?Nat; expires_at : ?Nat64; fee : ?Balance; memo : ?Memo; created_at_time : ?Nat64; };

tomijaga · 2023-07-08T09:35:51Z

Hey @receronp,
My bad, I forgot to publish the review.
I've published it now so you should be able to see the comments

skilesare · 2023-07-17T17:16:34Z

@raceronp. Have you been able to address these issue? If so, I'll suggest that the bounty be awarded to you for the ICRC2 part and we can split out ICRC3...they have just released some updates to the format for ICRC3 last week so there may need to be some adjustments.

receronp · 2023-07-18T16:22:10Z

@skilesare I am still working on the review provided by @tomijaga. Expect to have this completed by the end of the week.
I am however still not assigned to the bounty, would that be required before completion and award?

skilesare · 2023-07-24T19:30:31Z

I'll be reassigning it to you

tomijaga · 2023-08-02T19:02:46Z

Hey @receronp,
Have you made progress on the discussed changes?

Let me know if I can help with anything that's unclear.

receronp · 2023-08-03T21:50:16Z

Sorry guys, been doing changes in my local repo, and because some of them are breaking changes I've not pushed them here.
I expect to have a working solution in the coming week.

sardariuss · 2023-08-25T15:30:52Z

Just came to say that there is a spender_subaccount field in the TransferFromArgs type that is not present in the ICRC2 standard and seems to not be used in the code.

tomijaga · 2023-10-24T13:14:49Z

Hey @receronp,
I wanted to check in and see if you had a chance to make the changes for the implementation yet.
If you haven't, please push any changes you have to the repo. I'll take a look and finish up the rest.

receronp added 9 commits May 23, 2023 15:45

Fix example add mops deps, amend default args txt

4c59fd2

Initial ICRC-2 commit, methods based on ICRC-1

c6839cc

Adds icrc2_approve method, utils, types, helpers

57c4989

Adds icrc2_allowance, updates utils, helpers

b4a1e60

Adds icrc2_transfer_from, updates utils, helpers

c9a8d31

No changes (file formatting)

b7738a7

Adds icrc2_approve and allowance tests

2c8fda0

Adds icrc2_transfer_from tests

6f064ad

Adds ICRC-2 Example Tests, update allowance checks

fe98141

receronp closed this May 29, 2023

tomijaga reopened this Jun 6, 2023

tomijaga requested a review from skilesare July 1, 2023 21:08

Merge branch 'main' into icrc2

ffcd688

receronp force-pushed the icrc2 branch from 10f58ed to ffcd688 Compare July 6, 2023 15:30

tomijaga reviewed Jul 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICRC-2: Approve and Transfer From #12

ICRC-2: Approve and Transfer From #12

receronp commented May 26, 2023

receronp commented May 29, 2023

tomijaga commented Jun 6, 2023

receronp commented Jun 19, 2023

tomijaga commented Jul 2, 2023

receronp commented Jul 6, 2023

tomijaga Jun 10, 2023

tomijaga Jun 22, 2023

tomijaga Jul 1, 2023

tomijaga Jun 22, 2023

tomijaga Jun 22, 2023

tomijaga Jun 22, 2023

tomijaga Jul 1, 2023

tomijaga Jul 1, 2023

tomijaga Jul 1, 2023

tomijaga Jul 2, 2023

tomijaga commented Jul 8, 2023

skilesare commented Jul 17, 2023

receronp commented Jul 18, 2023

skilesare commented Jul 24, 2023

tomijaga commented Aug 2, 2023

receronp commented Aug 3, 2023

sardariuss commented Aug 25, 2023 •

edited

Loading

tomijaga commented Oct 24, 2023


		public type TransferArgs = Types1.TransferArgs;

		public type Transfer = Types1.Transfer;

ICRC-2: Approve and Transfer From #12

Are you sure you want to change the base?

ICRC-2: Approve and Transfer From #12

Conversation

receronp commented May 26, 2023

receronp commented May 29, 2023

tomijaga commented Jun 6, 2023

receronp commented Jun 19, 2023

tomijaga commented Jul 2, 2023

receronp commented Jul 6, 2023

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

tomijaga commented Jul 8, 2023

skilesare commented Jul 17, 2023

receronp commented Jul 18, 2023

skilesare commented Jul 24, 2023

tomijaga commented Aug 2, 2023

receronp commented Aug 3, 2023

sardariuss commented Aug 25, 2023 • edited Loading

tomijaga commented Oct 24, 2023

sardariuss commented Aug 25, 2023 •

edited

Loading