icrc2#10
Conversation
|
Hey @JingJingZhang9, In the meantime, I will review the PR and leave any feedback. |
| "defaults": { | ||
| "build": { | ||
| "packtool": "mops sources", | ||
| "packtool": "vessel sources", |
There was a problem hiding this comment.
Currently, mops is the preferred manager because of its ability to host packages that can be easily retrieved without the added effort of setting up complex package sets.
We would lose all these benefits if we switch back to vessel.
| # ICRC-2 Implementation | ||
| This repo contains the implementation of the | ||
| [ICRC-1](https://github.com/dfinity/ICRC-1) token standard. | ||
| [ICRC-2](https://github.com/dfinity/ICRC-1/blob/main/standards/ICRC-2/README.md). |
There was a problem hiding this comment.
Instead of replacing the link, we could add a statement that says the repo contains implementations of ICRC-1 and ICRC-2 and link to them
| ## References | ||
| - [ICRC-1](https://github.com/NatLabs/icrc1) | ||
| - [ICRC1 test](https://github.com/NatLabs/icrc1/blob/main/example/icrc1/main.mo) |
There was a problem hiding this comment.
No need to remove the references as they are links to other icrc1 implementations
There was a problem hiding this comment.
For the readme, I think we can leave the previous information about ICRC-1 and then add sections with information for ICRC-2
| import ArrayModule "array/Array"; | ||
| import Itertools "itertools/Iter"; | ||
| import StableBuffer "stable/StableBuffer"; | ||
| import STMap "stable/StableTrieMap"; |
There was a problem hiding this comment.
Downloading the dependencies directly into the repo leads to increased project size and makes it harder to update or rollback package versions
| public type TransferFromArgs = { | ||
| from_subaccount : Account; | ||
| to : Account; | ||
| amount : Balance; | ||
| fee : ?Balance; | ||
| memo : ?Blob; | ||
|
|
||
| /// The time at which the transaction was created. | ||
| /// If this is set, the canister will check for duplicate transactions and reject them. | ||
| created_at_time : ?Nat64; | ||
| }; |
There was a problem hiding this comment.
icrc2_transfer_from
type TransferFromArgs = record { spender_subaccount : opt blob; from : Account; to : Account; amount : nat; fee : opt nat; memo : opt blob; created_at_time : opt nat64; };
Missing the spender_subaccount and the from_subaccount should be replaced with the from account.
| switch (allowance_pair.expires_at) { | ||
| case (null) {}; | ||
| case (?expires_at_time) { | ||
| switch (tx_req.created_at_time) { | ||
| case (null) {}; | ||
| case (?created_at_time) { | ||
| if (created_at_time > expires_at_time) { | ||
| return #err(#InsufficientFunds { balance = 0 }); | ||
| }; | ||
| }; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
In this condition, we need to check if the allowance time of the spender has expired or not. To do that we need to check if the expires_at field is less than the current time. We can get the current time by calling the Time.now() function.
| case (null) {}; | ||
| case (?created_at_time) { | ||
| if (created_at_time > expires_at_time) { | ||
| return #err(#InsufficientFunds { balance = 0 }); |
There was a problem hiding this comment.
In situations where there is not an explicit error type, you can use the
#GenericError : { error_code : Nat; message : Text };| #ok(); | ||
| }; | ||
|
|
||
| public func validate_approve_request( |
There was a problem hiding this comment.
The validate_approve_request and validate_transfer_from_request functions need to add conditions to check for transaction deduplication. This was introduced in the ICRC-1 standard.
There was a problem hiding this comment.
Actually, the transfer_from function in lib.mo uses the Transfer.validate_request() which checks for transaction deduplication. It's just the approve function that doesn't have it.
| created_at_time = tx_transfer_from_req.created_at_time; | ||
| }; | ||
|
|
||
| let normal_tx_kind = #transfer; |
There was a problem hiding this comment.
This should say #transfer_from instead so deduplication does not happen for a #transfer kind with matching fields. The TxKind type should be updated for ICRC-2 to include it.
|
- Changes made on May 29 have been merged into the main branch.
ICRC2 alterations have been performed and committed to a distinct branch
in another repository.
Let me know if you have any questions.
Message ID: ***@***.***>
… |
2 participants
No description provided.