Call for Participation: NFT Token Standard Working Group [Status Updated]

Developers
42

@skilesare, good question! This would only concern the variant return types, where the variants would need to be made optional so that in case of an extension of the variant clients don’t break. The cases that are potentially concerned are all similar in that they have an Ok with the transaction index and an Err with different variants and a GenericError. The Ok seems to me to be pretty much what we would ever want in this standard, the Err already as the GenericError so that one could argue that not more is needed. So I’d argue we can go without making the variants in those cases optional.

Other opinions?

2 Likes
43

Thanks a lot, will be going through them and fix the ones that do not need discussion and put the others up for discussion.

2 Likes
44

That is not true. We check both spender AND subaccount at transfer.

3 Likes
45

Hi @cryptoschindler!

Thanks a lot for your thorough review that gave us very valuable feedback! Here’s a response to your points, some of them have been resolved, others still need decisions in the group, see my open points concluding the post.

  • should we add an entry to the metadata that enumerates the generic errors by a specific implementation? together with a specific endpoint to query for them?
    Do we already have generic errors defined by now? Don’t think so. If so, it would make sense.
  • should we have endpoint for single token queries? e.g. icrc7_get_approval

I thought the idea was to have just batch methods.

The only difference in usage when using the non-batch methods would then be to not require the vector for the token_ids, but a single argument. The opt parameters for pagination can be left out. Considering this, they are not really necessary.

Implementation wise it would not be a big thing to add those methods.

Thoughts?

  • transaction deduplication section mentions ICRC1

Good catch! Changed to ICRC-7.

  • icrc7_collection_metadata
  • seems cumbersome, try assembling ICRC7 suggested fields
  • why not follow cap and have (Text,Value) tuple or vec record { text; Value } at the top level
  • do we need to prefix suggested metadata fields with icrc7 as well?

Changed to vec record { text; Value } which was my intention following the discussion, but must have missed this change originally.

We should use the same namespacing mechanism as defined for ICRC-1 metadata and use the icrc7 namespace in analogy.

  • icrc7_token_metadata
  • doesnt specify Metadata type
  • what is the behaviour if token id does not exist? will it just not be in the response or will there be an explicit error?
  • should this be compliant with behaviour of icrc7_owner, meaning return a Result

This was a remainder when we have the Metadata type.
Switched to Value.

For a non-existing token id we have the following options:

  • Return no value
  • null value for metadata; this requires to make the metadata optional
  • Return an error (like for some other queries)

We should align the behaviour for all relevant interfaces. We had errors contained in the queries using a variant, but then removed them as the API was rather convoluted. That’s an important point to address.

  • icrc7_owner_of
  • says null is returned if token doesnt exist, but lists variant with Err case that contains NonExistingTokenId

Like above, we should consolidate the error handling for queries.

  • icrc7_balance_of
  • why not 0 instead of null

null gives the caller more information, namely that the token does not exist. But 0 would be appropriate as well.

  • icrc7_tokens & icrc7_tokens_of
  • should we add default value for take to metadata and create own method to query? like for batch size and approvals?

Yes, it’s there now.

  • icrc7_approve_tokens
  • multiple approvals can exist for the same token_id but different spender s and subaccount
    • ^ what does this mean? different spender already suggest difference subaccount?
  • emphasize that spender subaccount is arbitrary as we only check principal when actual transfer happens
  • what error do we throw if the token id doesn’t exist?
  • in what case do we throw TemporarilyUnavailable?
  • expires_at semantic should be explained

Made the subaccount aspect clearer and changed it to from_subaccounts.

We also check subaccount, not just principal.

Error handling should be unified for all query methods.

  • icrc7_approve_collection
  • text seems wrong, duplicate response text
  • candid method has the wrong name, candid types are missing

Thanks, copy-paste error. Text is fixed.

The method uses the same Candid type as another one where the type is defined. I wondered whether we should repeat the types, but that may lead to inconsistencies, thus may not be the best idea. Maybe refer to where the type is defined. What’s your opinion?

  • icrc7_transfer
  • here we say if the token doesn’t exist, we throw Unauthorized. should this be adapted to the above methods where i raised the issue?
  • do we need deduplication? if the token id is transfered, calling transfer should already throw Unauthorized :thinking:
  • we should add the created_at_time hint for the approve methods as well

Yes, we need to unify the error responses for all applicable methods.

Deduplication: Very good question. Reading your thoughts and rethinking this, I think we might not strictly need it because of the non-fungibility of tokens. However, it is something that may still make some things nicer: E.g., a double-submitted approve does not create two blocks, but only one.

  • icrc7_revoke_token_approvals
  • mention that only owner can revoke approvals
  • mention what happens when token does not exist
  • why is spender opt?

Mentioned, good point.

The opt is intended to revoke approvals for the respective token for all spenders (and subaccounts) in case left empty.

In that case, we can ask the question whether we can remove the icrc7_revoke_all_token_approvals method probably as it is covered by this one. It adds the removal of all approvals for all tokens, however, which this cannot do. Removed it is the value is diminished.

  • icrc7_revoke_collection_approvals
  • why does this return a vector? should be analaogue to approve_collection, so either add it there as well or remove for both
  • should be called icrc7_revoke_collection_approval instead of approvals
  • candid types are missing

The method can revoke multiple approvals in case the opt parameters are not provided. Thus, the name is fine. The signature has been fixed, but is still a vector: Each revoked approval is reflected as one response in the vector. Does that make sense to you?

  • icrc7_revoke_all_collection_approvals
  • this should either contain more information in the return type, so that it is clear to which collection wide approval the response belongs (e.g. via from_subaccount and spender) or fail/succeed atomically and only return one result instead of a vector

Both revoke_all methods have been removed as they are handled with the opt parameters in the other revoke methods.

  • icrc7_is_approved
  • typo in ac

Fixed.

Overall, the main open points now are the following:

  • Error handling for query calls needs to be made consistent
    • Error variant with an error type per query or a generic error type for all queries could make sense if people think this adds value. It complicates the API, though, compared to the current one.
  • How to handle non-existing tokens consistently
    • Error
    • Null fro response
  • TemporarilyUnavailable: Do we need this or can we remove it from the standard?
  • Using icrc1 namespace for existing metadata fields defined in ICRC-1?
1 Like
46

I personally like to keep errors out of queries, data is either there or it isn’t. Compared to calls, we’re not mutating any state where things can go wrong.

2 Likes
47

In theory it’s not needed since a token can only be sent once but in the weird edge case you (user A) sent a token to user B and you buy it back a few seconds later (in)directly from user B. Then re-submitting the transfer would incorrectly send the token to user B. I doubt this happens in real life applications with end users but it could happen with some web 2 services that frequently move around tokens in some automated process.

2 Likes
48

As far as I remember correctly this was also there for two usages besides the account migration:

  • ability to rate limit calls per accounts/tokens/something
  • if ICRC-8 intend to built upon ICRC-7, this would be for example used to indicate token cannot be transferred

How does ICRC-1 handle account migration in e.g. the ICP ledger?

Adding TemporarilyUnavailable just to support migrations seems a bit over complex for clients, now they also need to handle this data response while the information of this response type doesn’t have any valuable data to handle this edge case.

Maybe it makes more sense to define fallback behavior for each method e.g. icrc7_owner_of could return an account with owner = canister principal and subaccount = icp account hash. In theory the canister could even handle incoming tokens for these accounts and assign them to the correct user, basically supporting the old icp account hash without the need to keep two lists of owners formats in memory.

1 Like
49

That was the reason that most queries currently don’t have an error variant. I find the API with this much simpler than with an error variant as in the case of update calls.

Agreed, the case you sketch here is an edge case. Accidental resubmission in mobile apps due to connectivity issues, the main reason why we need deduping for fungible tokens, will likely not be an issue in NFT ledgers. Even if ownership is transferred forth and back, this is an edge case. I don’t have a strong opinion on whether we should keep or not keep deduplication.

1 Like
50

Dear Working Group!

We did another thorough iteration over the draft.

ICRC-7 most recent draft
Diff of version of 20231023 to version of 20231016

@skilesare, @sea-snake, @cryptoschindler, Matthew, all, please have another thorough look over the draft.

Aspects addressed:

  • Items discussed in the recent WG meeting of Oct 10
  • Comments by @cryptoschindler and @sea-snake and related follow-up discussions
  • Overall polishing and making the text more consistent
  • Wording, clarity
2 Likes
51

WG meeting tomorrow, Oct 24

Dear Working Group!

For the meeting tomorrow the goal is to have a really final iteration over the standard draft and then get it into voting.

See my post above about the recent draft and a diff with the version of Oct 16.

2 Likes
52

some questions i had:

  • why do we distinguish between max batch size for update and query calls, but not for the default and max take values? → only exists for queries
  • why is icrc7_max_take_value only specified for paginated query methods? → only exists for queries, will be rephrased
  • can we turn icrc7_collection_metadata : () -> (metadata : vec record { key: text; value: Value } ) query into icrc7_collection_metadata : () -> (metadata: vec record {text;Value } ) query → done
  • can we rename GetOwnerError to OwnerOfError → unclear
  • should we add an error for when the implementation specific max batch size is exceeded by the caller or should we trap? if we add an error, we have to restructure current return types and wrap them in result type → add paragraph that implementation should trap if limits are exceeded
    • see icrc7_owner_of for example, imagine max batch size is 30 but we submit 50 tokens. what is expected to happen?
    • we should specify expected behaviour for all batch methods imo
    • same for max take
    • same for max approvals
  • should we explictily document error variants and their meaning?

icrc7_approve_tokens

  • explain semantic of expires_at
  • i think we need a section explaining the reasoning behind having to specify from_subaccount when in theory it can be inferred from the token_id. something along what @benji said in todays call that you have to think in the granularity of Accounts, not Principals (?) otherwise it feels unnecessary to explicitly specify it in some places
  • why is token_id in the response optional?
  • add NonExistingTokenId error
  • semantics of memo
  • semantic of not specifying from_subaccount
    icrc7_approve_collection
  • remove this, there is no notion of token

    For the same token, spender, and subaccount triple a new approval shall always overwrite the old one.

  • we shouldn’t reuse ApprovalError here if we add NonExistingTokenId to it, as this variant doesn’t make sense in the context of this method

icrc7_transfer

  • why do we have to specify from, shouldn’t the ledger be able to infer this from token_ids
  • remove type from TransferArgs
  • add NonExistingTokenId error instead of using Unauthorized
  • for approval we say approval_response, for transfer we say transfer_result, for owner_of we say account. we should be consistent with our naming in general

icrc7_revoke_token_approvals

  • from_subaccount in the response seems unnecessary (with new semantic discussed today)

icrc7_revoke_collection_approvals

  • from_subaccount in the response seems unnecessary (with new semantic discussed today)

icrc7_is_approved

  • from_subaccount should be optional
5 Likes
53

Thanks for the thorough review and catching some more things!

Here is a first response, most things could be resolved. Some remain open as listed further below.

can we rename GetOwnerError to OwnerOfError → unclear

  • Done, no reason why not to rename as it is now more consistent.
  • For making errors more consistent as discussed in the call: Maybe we should split ApprovalError to ApproveTokensError and ApproveCollectionError, which is more consistent and would capture both approve methods. Also see your comment further below which gets addressed with that.

add paragraph that implementation should trap if limits are exceeded

  • see icrc7_owner_of for example, imagine max batch size is 30 but we submit 50 tokens. what is expected to happen?
  • we should specify expected behaviour for all batch methods imo
  • same for max take
  • same for max approvals
  • Added.
  • Other points should be addressed with some text added. Is that concrete enough in your view?

should we explicitly document error variants and their meaning?

  • Yes, I raised that question also some time back but no one has done it so far.
  • Now done (should have addressed all of them by now, but need to double check).
  • All error types should have overlapping fields now in same order.

icrc7_approve_tokens

  • explain semantic of expires_at
  • i think we need a section explaining the reasoning behind having to specify from_subaccount when in theory it can be inferred from the token_id. something along what @benji said in todays call that you have to think in the granularity of Accounts, not Principals (?) otherwise it feels unnecessary to explicitly specify it in some places
  • why is token_id in the response optional?
  • add NonExistingTokenId error
  • semantics of memo
  • semantic of not specifying from_subaccount

All of the above should have been addressed now. We might still want to iterate over the motivation on why the from_subaccount is always needed.

icrc7_approve_collection

  • remove this, there is no notion of token

For the same token, spender, and subaccount triple a new approval shall always overwrite the old one.

  • we shouldn’t reuse ApprovalError here if we add NonExistingTokenId to it, as this variant doesn’t make sense in the context of this method

Addressed everything.

One thing to note is that the semantics of approve tokens / approve collection differs also in that for an approve tokens, each token id can have an approval per spender, but only on the from_subaccount the token is on. The approve collection, however is different in that a caller can approve the collection for each pair of (spender, from_subaccount) if we define the semantics in a generic way. Open for discussion.

icrc7_transfer

  • why do we have to specify from, shouldn’t the ledger be able to infer this from token_ids
  • remove type from TransferArgs
  • add NonExistingTokenId error instead of using Unauthorized
  • for approval we say approval_response, for transfer we say transfer_result, for owner_of we say account. we should be consistent with our naming in general

The from parameter makes the intent explicit, which is required. Think of a token being approved to multiple spenders, then one of them transfers it. Then the token looses its approval, but a transfer attempt by another previously authorized spender would try to transfer it from the wrong party. It would result in an Unauthorized in either case, but in the case without the explicit from a completely unintended transfer is attempted. Does that make sense?

type removed, good catch

Error variants updated, Unauthorized is still required as well.

icrc7_revoke_token_approvals

  • from_subaccount in the response seems unnecessary (with new semantic discussed today)

Yes, removed it to reflect the new semantics.

icrc7_revoke_collection_approvals

  • from_subaccount in the response seems unnecessary (with new semantic discussed today)
    icrc7_is_approved

Yes, removed it to reflect the new semantics.

from_subaccount should be optional

Yes!

Doing another iteration tomorrow and then you could have another one. Slowly we are converging to something nice.

Open

Can we agree that token-based approvals can have one per spender, while collection-based ones can have one per (spender, from_subaccount) pair? Also, the latter can be done without holding tokens. This has never been discussed, but may make sense as such approval persists if you add or remove tokens to the collection. Not allowing to make an approval when holding 0 tokens would be odd somehow.

Do we need deduplication? Once a tx has transferred a token, it is gone, so a duplicate would not harm. For approvals likewise, a duplicate creates another entry in the history and overrides the previous one with the same information. This is still open.

Do we need NotMigrated in other methods than icrc7_owner_of? Considering that approvals and transfers can derive the ICP address from the caller and from_subaccount those operations should work fine with not migrated tokens. What did we discuss in the meeting, my notes are unclear on this point.

4 Likes
54 
icrc7_token_metadata : (token_ids : vec nat)
    -> (vec record { nat; Value }) query;

I thought we had agreed that the return should be vec record { nat; opt Value }) so that we could return a null if one of the tokens requested didn’t exist? Otherwise, we’ll have to trap if one of the values provided doesn’t exist. Or maybe we decided to trap? If so we should update the description of that function to direct the expected behavior.

1 Like
55

I’m messing with trying to do an actual implementation and I’m really regretting not having Bool in here:

// Generic value in accordance with ICRC-3
type Value = variant { 
    Blob : blob; 
    Text : text; 
    Nat : nat;
    Int : int;
    Array : vec Value; 
    Map : vec record { text; Value }; 
};

Should we say something about the best practice for returning boolean values? Think of something like isTradeable?

#Text(“true”);
#Blob(Blob.fromArray([1]));
#Nat(1); //I think VB used to have bool false as -1…confusing

1 Like
56

cc @mariop Do you remember why didn’t we have bool in ICRC3 Value?

1 Like
57

This line is a bit unclear if this is supposed to be a record with a property of balance or if it just returns a Nat:

icrc7_balance_of
Returns the balance of the account provided as an argument, i.e., the number of tokens held by the account. For a non-existing account, the value 0 is returned.

icrc7_balance_of : (account : Account) -> (balance : nat) query;

This is pretty consistent through the doc as most of the responses with just one value are put in () with a key name, but not in a record. I assume this is to convey the interpretation of the return and that returning just the type of the value is sufficient, but it is a bit confusing. Please correct me if I’m wrong.

Over all it may be a good idea to put a canonical did file at the end of the description so people can check.

58

Returning a null is the cleaner option and the draft is changed now.

59

Hi Working Group members!

As discussed in the recent meeting, we wanted to do another iteration over the text. This has been done now and it has improved further.

@cryptoschindler, @skilesare, @benji, @sea-snake, all

1 Like
60

Implementation note:

The spender on an Approval is an account and you can specify something with a non-null sub-account. This has no functional effect on other things as outlined in the spec and a spender calling transfer on behalf of another user would be calling from a principal and there is no functional distinction for the idea of a subaccount in the msg.caller. It is just a principal.

That being said, it is likely good to keep it an Account type as some future implementation may want to reward a spender and this sub-account would allow that ledger to send rewards to a specific sub-account. This may be a confusing explanation, but it may be worth adding the intention and clarifying for an implementer trying to figure out how to revoke on the subaccount of the caller.

61

We don’t explicitly say what the Ok return on approvals is. (I assume it is the index of the ledger entry created, but we should state it explicitly). Some ledgers use something like CAP for their leadger(which I think is a bad idea) and I’m not sure if they have Nat IDs or not.

We didn’t explicitly say that we assume an icrc7 canister is keeping a ledger, but maybe we should as this response seems to assume that it is present.

icrc7_approve_collection : (ApprovalInfo)
    -> (approval_result : variant { Ok : nat; Err : ApproveCollectionError });