Open benjizhai opened 1 year ago
Just went in depth through the the standard and have a few comments.
icrc7_collection_metadata
Returns all the collection-level metadata of the NFT collection in a single query.
icrc7_collection_metadata : () -> record { icrc7_name : text; icrc7_symbol : text; icrc7_royalties : opt nat16; icrc7_royalty_recipient : opt Account; icrc7_description : opt text; icrc7_image : opt text; // The URL of the token logo. The value can contain the actual image if it's a Data URL. icrc7_total_supply : nat; icrc7_supply_cap : opt nat; } query;
Not sure if royalties should be part of a minimal standard. Also I've seen royalty fee implemented as "(sale price) -> royalty fee" to give more flexibility and support for dynamic logic.
icrc7_tokens_of
Returns the list of tokenIds of the account given as an argument.
icrc7_tokens_of : (Account) -> (vec nat) query;
There's a two MB limit on messages, in theory someone might want to use the standard for domain names, game ownership mechanics etc that will go beyond the limit unless there's optional pagination.
icrc7_transfer
Transfers one or more tokens from account to the to account.
type TransferArgs = record { spender_subaccount: opt blob; // the subaccount of the caller (used to identify the spender) from : Account; to : Account; token_ids : vec nat; // type: leave open for now memo : opt blob; created_at_time : opt nat64; is_atomic : opt bool; };
Not sure why the icrc-1 transfer and transfer_from are combined into a single method here, it seems confusing to me. Also since there won't be a differentiation, a user of a wallet would not know if he's transferring from his own wallet or someone else's unless the wallet dapp inspects the transaction cbor and clearifies this in the UI.
Additionally the batch part and atomic part seems to make the standard more complicated, wouldn't it make more sense to make this it's own extension?
Some more things:
icrc7_tokens
.Account
, there's no way to limit access to specific subaccount of a principal, giving approval to a principal would make more sense here.logo
metadata entry for the logo, why is it called image
in icrc-7?Not sure if royalties should be part of a minimal standard. Also I've seen royalty fee implemented as "(sale price) -> royalty fee" to give more flexibility and support for dynamic logic.
I think Royalties mechanisms can be very complex and sometimes context-oriented. The logic can be really complicated and I don't think they should be in the minimal standard. Anyway, to put royalties in the standard can also be a starting point to facilitate adoption. I'd like to have more feedback from the community on this point, I don't have a clear idea tbh.
There's a two MB limit on messages, in theory someone might want to use the standard for domain names, game ownership mechanics etc that will go beyond the limit unless there's optional pagination.
icrc7_tokens_of
I don't know about this 2 MB limit on messages @sea-snake , do you have any documentation about that?
Anyway, another personal consideration is that updates on the list of tokens owned by each account can be really cycles expensive. That's why in Ethereum ERC721 doesn't support this method. I'm working on a Motoko implementation of the standard and I think this method could lead to cycles usage issues and I don't have found/seen a really scalable implementation of this method. Any idea about that?
Not sure why the icrc-1 transfer and transfer_from are combined into a single method here, it seems confusing to me. Also since there won't be a differentiation, a user of a wallet would not know if he's transferring from his own wallet or someone else's unless the wallet dapp inspects the transaction cbor and clearifies this in the UI.
I don't think a differentiation between transfer
and transfer_from
is needed. They can be the same method, as it happens in ERC721. Why would you differentiate the method if you are transferring from your wallet or another? But maybe I didn't understood your consideration.
Is transaction deduplication needed? Compared to a fungible token, a non fungible token can only be sent once as far as I understand.
Deduplication is not needed in my opinion. If the token id has already been transferred, the method would throw an error. But maybe I think it's a "standardization" of token behaviors, and it can be user-friendly to receive a massage like "Hey, your token has already been sent!" ? I'd like to have more feedback about that too.
There seems no way to get a list of all tokens, since the spec does not specify that tokens have to be sequentially starting from zero, there's no way to know which tokens exist unless there's a method like e.g. icrc7_tokens.
I agree. A method icrc7_tokens
is needed. Maybe paginated too, as for icrc7_tokens_of
.
How to revoke approval? How to get list of approvals?
No way. Two options in my opinion: change params in current methods (adding a boolean to revoke the approval) or add a new method to revoke approval.
Does the spec allow approval for the same token id to multiple principals? (So users can e.g. list their token on multiple market places)
In ERC721 there can only be one approved address per token at a given time. Can only be called by the token owner or an approved operator. I think this specification in the ERC721 standard has been decided to overcome the problem to have high costly calls in terms of gas (imagine to operate with arrays in solidity, should be extremely high consuming), but in ICP I think we can “relax” this constraint.
Icrc7 Implementation Notes
Last week we implemented the current draft of the ICRC7 standard into the ORIGYN NFT.
While we have been working with the working group to help develop ICRC7, ORIGYN foundation has taken a stance since the beginning that we think that the general principles behind ICRC7 exacerbate the inherent issues with NFTs that arose out of the limitations of the Ethereum architecture and that the group should instead focus on an NFT standard that utilizes the full potential of the IC ecosystem.
The group is now discussing that with ICRC8 and we’re progressing well. Since the group wanted to do a more eth-like standard in ICRC7, we participated and contributed to its development as best as we felt the standard could be developed. We have every intention of supporting whatever standards are approved, just as we already support the EXT standard and the DIP721 standard. In that spirit, we undertook to implement the ICRC7 standard into the existing implementation of the origyn_nft standard. As is usually the case, some issues with a system won’t be seen until you actually try to implement it. In that spirit, the following is our implementation report of ICRC7 which we hope will be helpful in finalizing the standard.
icrc7_owner_of query - It seems odd that this does not have a result response. Since it is possible to request the owner of a token_id that does not exist, the best we could do was trap if the token doesn’t exist. It may make more sense for this function(and perhaps the other query functions that let you request a response for a token id that may not exist to have a return type of
{
message: Text; error_code: Nat}) }; }
Metadata Limitations - We’ve proposed elsewhere the ICRC16 standard for recursive, candid like, extensible metadata. We’d repropose it be considered here. We recently updated it so that can be a supertype of the ICRC3 transaction event type metadata and it makes sense to do the same here. Because we didn’t really have the control we wanted here(for example, we have a manager array and with no way to return a manager we were faced with having to have list of “com.origyn.manager.1”,“com.origyn.manager.2”,“com.origyn.manager.X…”. There is also not a Principal type which is odd considering how much principals are used in the IC(you can convert to Blob, but it is going to be really messy to read on transaction logs). Text is another option but takes more space). We chickened out and have one “metadata” field that is the json text representation of our nested, recursive metadata.
Versions evolve - The icrc7_supported_standards has a return of vec [(Name, URL)]. It may be that some standards evolve and thus end up with a version number. (we are already on origyn_nft v0.1.5 which does have some differences). We think this schema can support that with different URLs, but a version field might be worth a second consideration.
No way to get all token IDs - the collection metadata query does not return the token ids and there is no other query function to do so. We’d recommend an ircr7_token_ids( opt {skip: Nat; take: Nat}) to allow paginating IDs. I think there is an assumption that tokenids will be sequential and from 0 to total_supply-1, but this is not how we implement it. Origyn_nft has text based token_ids for readability and human cognition and recognisability. We convert these strings to a Nat when we implement ICRC7 or DIP721 and they are not sequential.
Is_atomic in transfer args - We do a number of potentially async functions when transferring around NFTs, including KYC, paying royalties, recognizing deposits, etc. Atomicity is not something we can support, and we’d argue that any sufficiently interesting or complex system with real-world utility is going to have the same problem on the IC. Encouraging that atomicity is possible/reliable is creating a very hard-to-untie knot for developers. This can become especially troublesome for generic 3rd party services that may assume you have atomicity when you actually don’t. We chose to reject any request for transfer where atomicity is requested.
Multiple token transfers but singular response - We keep separate ledgers for each token id. We could support batch transfer requests(and in fact do support this in the origyn_nft standard), but each transfer will have its own response. The fact that the standard assumes a singular response from this function is odd. As a result, we are currently rejecting any request for more than one token_id at a time.
Approve - Origyn NFTs see an existing escrow of tokens as the approval of a transfer as a market transaction. As a result, this function doesn’t have enough information in the event args to make sense to an origyn_nft and we trap automatically.
Metadata unification - The Collection metadata is hard coded to an object type, but the NFT metadata is extensible. Perhaps the Collection metadata should use the same format as NFT metadata?
Royalties - royalties were awkward to implement. We support a menu of royalty recipients and distinguish between primary and secondary markets. ICRC7 lets you pick one pay-to-account and one amount. We chose to sum up the secondary royalties and created a sub-account on the canister to direct these two. We can, in the future create a system for distributing anything sent to this account, but given that all transfers happen through our in-nft market mechanism the royalties would be auto-distributed. As a result, anything sent here was probably done by mistake. We’ll propose a better way to report this info in ICRC8, and there may not be a solution here because the demand is for simplicity. One solution might be to just move this to collection metadata if it is adjusted to be extensible considering this standard is just asking marketplace to honor this and there is no actual assumed enforcement mechanism.
Supply Cap - We don’t really have this concept(supply is controlled by the minter, so if you built some kind of capped supply you’d need to program that in the minting contract). We return null.
The group is now discussing that with ICRC8 and we’re progressing well.
Don't see anything regarding this in the ICRC-8 issue, am I missing something?
Found it: https://github.com/skilesare/ICRC/blob/patch-1/ICRCs/ICRC-8/icrc8.mo
Personally I think we should be able to get to a reasonable minimal standard consensus for a single canister NFT standard without creating multiple standards 😅
Details that aren't needed for a minimal standard should be left out, these topics are quite broad and could vary a lot per use case, I'd rather see these discussed individually into more detail than pin that down into the base NFT standard.
Though to make an NFT contract useful within e.g. exchanges, this still means that these topics are a high prio. But not all NFTs are used withing this context and need these functionality for bare minimum functionality.
Also this would allow a further iteration of e.g. the approval standard to be implemented and older approval standard to be deprecated. For example if icrc2 fungible token approval standard is deemed problematic and icrc43 is proposed, new contracts only need to implement icrc1 and icrc43. This is why I'd rather see icrc standards as small purpose oriented standards over standards that try to do everything at once.
As for metadata I think there's two sorts of metadata:
The low level metadata should be the bare minimum to make things work while the high level metadata is for clients with more extensive implementations and functionality beyond listing, sending and receiving NFTs.
I do agree that the missing of Principal is unfortunate but adding it now while the icrc1 standard doesn't have it in it's metadata would seem inconsistent. I would opt to make all metadata aligned with the format used in icrc1.
As for storing principals within these limited metadata field types, either bytes or text is fine. Personally I would opt for text in the textual account representation to make it readable in history and also make clear what subaccount it is (if there is one).
In theory the low level metadata could also refer to the high level metadata, allowing for external managed data on another e.g. game canister. If the high level metadata is on the same canister as the NFTs, it could be part of the same history chain. This topic is also quite diverse since concepts like "wearable" NFTs could also be covered here, where metadata is changed based on e.g. user interaction.
Regarding the version evolve, this would conflict with what's already in icrc1, also this would add complexity for the clients where they have to not only look at the name as unique indicator of the token standard but also at the version number and implement major/minor/patch version logic. From this perspective it might make more sense to include the version number in both the name and url so there's no overlap between the data of the returned supported standards.
Though this topic is a broader topic that applies to all token standards on the IC, due to the nature of contracts being able to be upgraded. I would maybe consider making a standard for version numbering in either the metadata or a method it's own standard so it can be also used on icrc-1.
Personal thoughts:
Royalties: icrc7_royalties
and icrc7_royalty_recipient
are too complex and case-specific concepts to be included in a minimal standard for NFT. They should be part of some other ICRC, not this one. I'd remove them both from the standard.
Supply Cap: Supply cap is something related to minting logic. As for icrc7_royalties
and icrc7_royalty_recipient
, I'd remove icrc7_supply_cap
from the ICRC-7 standard too. This information is unnecessary for meeting the minimum standard.
Token IDs List: I think that adding a method that returns all the token IDs of the NFT collection is necessary. @sea-snake suggested icrc7_tokens
, @skilesare icrc7_token_ids( opt {skip: Nat; take: Nat})
. I like them both, and pagination is necessary. @skilesare do you think pagination should be added to the icrc7_tokens_of
method too?
Is_atomic & multiple token ids transfers: At the moment, I'm not clear if the response of token transfer with is_atomic = false
and token_ids.size() > 1
with at least 1 token id failing the transfer is #Ok
or #Err
. In the implementation I (and my Team) are working on, we decided to try to support atomic and multiple token ids transfer but we are not sure if it's the correct way and could lead to drawbacks. Removing atomicity could be a solution but the response has to be reviewed, at the moment it's not compliant with multiple tokens.
100% agree with @skilesare :
Multiple token transfers but singular response - We keep separate ledgers for each token id. We could support batch transfer requests [...], but each transfer will have its own response. The fact that the standard assumes a singular response from this function is odd.
Metadata unification: I think that some information should be mandatory for the collection, such as name and description. I don't know if unification is the way, but each collection that follows the ICRC-7 should return this mandatory information.
WG voting on ICRC-7
This comment hosts the official vote of the Ledger & Tokenization Working Group on ICRC-7 as specified through https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-7/ICRC-7.md.
How to vote:
React to this comment with 👍 if you agree with the ICRC-7 proposal and to bring it to an NNS vote. Leave comments regarding things you would like to still be fixed before bringing it to an NNS vote. There may still be some small things that need to be adapted after the recent refactoring.
React with 👎 and leave a comment with your objection if you oppose.
The Working Group will accept only the votes of the core members, i.e., the members regularly contributing to the WG. However, if you aren't a core working group member but have technical objections or comments to this proposal, please leave a comment. We'll try to address it according to the rough consensus model.
Found a few small issues and made an PR to fix these: https://github.com/dfinity/ICRC/pull/53
As discussed in last WG meeting, here's the forum thread to discuss the issue between the current mint block schema and token metadata: https://forum.dfinity.org/t/align-token-metadata-in-icrc-7-with-icrc-3-history-blocks/28243
ICRC-7: Minimal Non-Fungible Token (NFT) Standard
Following discussions during ICRC NFT Technical Working Group Meetings