Considering @skilesare and @kayicp’s responses above, I think it might make sense to remove the part in the spec that allows people without holding tokens to make approvals. It should be up to the ledger to decide, so we are as non-constraining as possible, but don’t hurt DoS protection. I’ll change this shortly in the text unless someone here opposes.
Indeed. I think all other operations do require you to have a token. These are transfers, approvals of tokens, cancelling approvals, and transfer_froms. Meaning that if we remove the spec part that collection-level approvals be possible without holding tokens and leave it open we should be in as good a shape as possible in a reverse-gas context and a ledger without fees.
An issue came up in our Token WG meeting today when discussing ICRC-4(Batch Transfers) that we needed to discuss if each transaction needed its own memo. We realized that the current ICRC7 Implementation only has a top level memo for deduplication and that that memo is inherited by each transaction that goes into the log.
TransferArgs = record {
subaccount: opt blob; // the subaccount of the caller (used to identify the spender)
to : Account;
token_ids : vec nat;
// type: leave open for now
memo : opt blob;
created_at_time : opt nat64;
};
We’d love feed back on:
Would it be convenient to have a memo for each token_id in the transfer:
For example, this would allow a canister to write a memo into each transaction during a token distribution event.
If we do put a memo at the token transaction level, do we want the top level memo to make it into the block as well. Assuming yes for completeness, what name should we use for the field in the transaction?(note, ref, comment?).
I’m going to tag @hpeebles and @julianjelfs because I believe one of them told me recently that OpenChat plans to add some interesting features when ICRC-7 comes out. If they don’t already know about this conundrum, then perhaps they want to have input.
icrc7_token_metadata : (token_ids : vec nat)
-> (vec record { nat; opt vec record { text; Value } }) query;
to
icrc7_token_metadata : (token_ids : vec nat)
-> (vec record { token_id : nat; metadata: opt vec record { text; Value } }) query;
to be consistent with the response of icrc7_balance_of for example?
Also, for icrc7_token_metadata we specify the following
In case a token does not exist, its associated metadata vector is null . If a token does not have metadata, its associated metadata vector is the empty vector.
For icrc7_owner_of the behaviour is different
Note that tokens for which an ICRC-1 account cannot be found have a null response.
I think it would be nice to consolidate the behaviour of those two methods, so for example let both of them trap if a token_id does not exist.
I wouldn’t opt for trapping, just leaving out tokens from the record that don’t exist seems like a better approach that allows clients to still get the data for the tokens that did exist, without having to handle this error and submitting the request again. The client can simply see if the token exists, by seeing it in the record or not.
I think this is a good idea and would also be in accordance with this
Batch Query Methods
Duplicate token ids in batch query calls may result in duplicate token ids in the response or may be deduplicated at the discretion of the ledger. The lenght of the response vector may be shorter than that of the input vector in batch query calls, e.g., in case of duplicate token ids in the input and a deduplication being performed by the method’s implementation logic.
@dieter.sommer@benji In thinking about this more, I think leaving the memo as a top level item is best. It is less data to submit and if you want to use the memo to point off to somewhere else(say an invoice with line items in it) you can do so on an implementation basis.
We can likely apply the same to icrc4…have a created at and memo at the top and then a vec of args without created at and memo.
The final WG meeting for the year is taking place on December 19, 2023. The proposed agenda is to look at the few remaining discussion points further up and resolve them.
Good point, we addressed it in the latest draft, but removed the opt from the metadata record as it is not required in this setting. The opt was used before to indicate a non-existing token.
@skilesare: We left it as is with the memo only at the top level as you seem to have reflected about this and come to the conclusion that we don’t need memos in the individual txs. Also no one else had a strong opinion that it should be changed.
When writing up the minutes of the ICRC-1 WG meeting of 20231212, I found one issue we discussed in the scope of batch transactions: The ICRC-1-style deduplication API, also used in ICRC-7, returns the block index of the already processed tx as response for a duplicate. For a batch transaction, which applies to ICRC-7 interfaces, this does not work as a batch leads to many ICRC-3 blocks being generated. We think the spec should be extended such that the response for a duplicate is the first block index of the blocks that the batch has generated.
We will add this to the deduping mechanism of ICRC-7 next year.
I’d like to invite the NFT members to attend the Ledger and Tokenization Meeting tomorrow. We plan to discuss ICRC4 and we need to have a discussion about deduplication for batch transactions. With ICRC4 I’ve tried to follow the same pattern as ICRC7. When there are duplicate function calls they don’t match up with a specific block because each transfer gets its own block. It would be nice to apply the same solution to ICRC4/7/30/X(batch approve for icrc1 if we decide to do that).
During the discussions of ICRC-4 Batch transfers, the group thought that moving both the timestamp created_at_time and the memo into the individual transactions of a batch instead of keeping them only at the top level. The arguments are the following:
Simplifies implementation
Generalizes better as it uses exactly the same logic for tx deduplication and other processing
Conceptually cleaner
The downside is:
Consumes slightly more storage; however, this has found to be a minor issue
If we do this, it would make perfect sense to also do this change for the ICRC-7 standard. This would imply that the APIs of update batch calls would need to change from
TransferArgs = record {
subaccount: opt blob; // the subaccount of the caller (used to identify the spender)
to : Account;
token_ids : vec nat;
// type: leave open for now
memo : opt blob;
created_at_time : opt nat64;
};
type TransferBatchError = variant {
InvalidRecipient;
TooOld;
CreatedInFuture : record { ledger_time: nat64 };
GenericError : record { error_code : nat; message : text };
};
type TransferError = variant {
NonExistingTokenId;
Unauthorized;
Duplicate : record { duplicate_of : nat };
GenericError : record { error_code : nat; message : text };
};
icrc7_transfer : (TransferArgs)
-> (variant { Ok : vec record { token_id : nat; transfer_result : variant { Ok : nat; Err : TransferError } };
Err : TransferBatchError });
to a representation with each transfer arg represented as its own record and also the errors are adjusted accordingly:
TransferArg = record {
token_id: nat;
memo : opt blob;
created_at_time : opt nat64;
}
TransferArgs = record {
subaccount: opt blob; // the subaccount of the caller (used to identify the spender)
to : Account;
transferArgs: vec TransferArg;
// type: leave open for now
};
type TransferBatchError = variant {
InvalidRecipient;
GenericError : record { error_code : nat; message : text };
};
type TransferError = variant {
TooOld;
CreatedInFuture : record { ledger_time: nat64 };
NonExistingTokenId;
Unauthorized;
Duplicate : record { duplicate_of : nat };
GenericError : record { error_code : nat; message : text };
};
icrc7_transfer : (TransferArgs)
-> (variant { Ok : vec record { token_id : nat; transfer_result : variant { Ok : nat; Err : TransferError } };
Err : TransferBatchError });
I wanted to add some additional fields in the collection metadata, want to store some documents and collection images, so wanted to know if I can edit the fields of collection metadata record for my NFT Collection following ICRC 7 standard, or if not is there another way to add this additional data along with collection metadata.
The collection is a Map( vec {(Text; Value)}, so you should be able to put whatever you want in and it can be returned with icrc7_collection_metadata. The other functions you mentioned are there for convenience, but the full metadata is returned by icrc7_collection_metadata. See:
Proposed agenda for the WG meeting tomorrow, January 16, 2024:
Moving created_at_time and the memo into the individual items of a batch update call. See post above. The reason for this proposal is to have it aligned with ICRC-4 on batch transactions for fungible tokens.
This is hopefully the final change before we can go to voting.
I’m starting a new NFT project. I have a lot of work from my prior projects (PokedStudio, Vibesters, etc) that I can work from but it’s all based on Toniq’s EXT standard.
I’d like to transition to a community standard but I’m not sure if ICRC-4 will be formalized in time (next couple months).
Does anyone have an idea for when this might be completed? Thanks in advance.
We hope to vote either before or at the next WG meeting in two weeks. We made a few tweaks last week to better align ICRC7(basic nft) with ICRC4(Batch Fungible Tokens) so that Batch transactions work similarly in both places.
