@skilesare glad to see your inputs in the thread. Could you tell us when we can expect this to go live? Or has it already? We don’t follow DFINITY on Twitter since it is full of noise.
Appreciate any response and progress update.
In regards to deduplication…did we figure out a reason that we need it? The nature of a non-fungible ledger seems to imply that dups are ineffective.
If I’ve transferred token 8 and try to a second time it will fail.
I can actually see it maybe being necessary in a flash defi sense on approvals:
I approve an entire collection,
A sale is made,
I revoke the collection approval
Collection approval is replayed.
I guess this could be done for tokes as well:
I transfer a token
I buy it back
Transaction is replayed.
I guess I’m talking myself into it, but we may need add to the spec that it needs to be done for approvals as well as transfers.
I’m correcting my own understanding on this one as there is a spender subaccount in the transfer args that must match.
We didn’t really document what a canister should do on transfer if a “top level” error occurs.
If there are more than the acceptable number of items in a batch or if to == from…should we trap? If we want to return an error then we probably don’t want to return an item in the vec for each item, especially in the to-large-batch scenario.
If the to == the from, as well, you only need to return one item…maybe it is best to mention one should trap in these instances.
Just making a note for us to discuss if we need to de-duplicate the revoke args. Could a malicious node replay to disrupt service?
Revoke approvals is kind of a mess:
icrc7_revoke_token_approvals: (RevokeTokensArgs)
-> (vec record { token_id : nat; spender : Account; revoke_result : variant { Ok : nat; Err : RevokeTokensError } });
Since spender isn’t null(I want to revoke all spenders) and there are errors that can exist that keep you from being able to identify a spender(Token Doesn’t exist, Unauthorized), there isn’t really a way to report that it applied to a spender. I’d propose that the spender be changed to null in the response so that we can report a null spender.
In addition, and similarly, is it the expected behavior that we’ll have multiple records reported back for a request to revoke all for a token ID?
example:
icrc7_revoke_token_approval({
token_ids = [1,2];
from_subaccount = null;
spender = null})
result: [
{token_id = 1;
spender = {owner= principal "a"; subaccount = null};
revoke_result = #Ok(6);
},
{token_id = 1;
spender = {owner= principal "b"; subaccount = null};
revoke_result = #Ok(7);
},
{token_id = 2;
spender = {owner= principal "a"; subaccount = null};
revoke_result = #Ok(8);
},
{token_id = 2;
spender = {owner= principal "b"; subaccount = null};
revoke_result = #Ok(9);
},
{token_id = 2;
spender = {owner= principal "c"; subaccount = null};
revoke_result = #Ok(10);
},
Of note here is that each spender identified gets a revoked record in the transaction log. This is so the state could be theoretically reproduced from the ledger of the NFT. This could easily bomb a ledger and cause it to grow quickly.
type TokenApproval = record {
token_id : nat;
approvalInfo : ApprovalInfo;
};
Should approvalInfo be a different case for consistency?
2 Likes
@skilesare
If you mean using approval_info, yes, we should use that for consistency.
Fixed it just now.
Nice catch!
1 Like
Another issue that was pointed out by someone in the community is the following: The standard defines the method icrc7_logo to obtain the logo of the ledger. The concern pointed out is that the logo, if improperly used / displayed in a Web app, can allow for XSS attacks. Particularly SVG graphics are concerned as they, if improperly embedded, i.e., not within an image tag, allow for XSS. Would people agree to add a warning about this?
Proposed text to add:
“Security note: We strongly advise developers who display the logo of a token in a Web application to follow Web application security best practices to avoid attacks such as XSS and CSRF resulting from malicious image content provided by a ledger. As one particular example, images in the SVG format provide potential for attacks if used improperly.”
In the call later today we would like to finalize ICRC-7 and discuss the remaining issues you have discovered, if any.
Once we agree to move forward, voting for the standard can be opened.
Actually, we do. There is a general paragraph further above that specifies that for all calls that create history, they return the transaction index in the success case.
Methods that modify the state of the ledger have responses that comprise transaction indices as part of the response in the success case. Such a transaction index is an index into the chain of blocks containing the transaction history of this ledger. Access to the transaction history is not part of the ICRC-7 standard, but will be published as a separate standard, similar to how ICRC-3 specifies access to ICRC-1 and ICRC-2 historic blocks.
As you missed it, it might not be prominent enough and maybe should be added explicitly with the calls to which it applies.
1 Like
You are of course right with your observation that the deduping is not required for rendering duplicate tx submissions ineffective. However, the other important aspect of deduping is to allow a mobile app that lost connection for a short duration to resubmit the tx and obtain the response, i.e., the transaction index for the created history block.
Interesting point you raise w.r.t. deduping for approvals, let’s discuss in the meeting later.
1 Like
The generic spec is again in the “Conventions” section in the beginning. But you are right, we might want to make this explicit with the methods and also make explicit the cases you mention above. The generic text is a catch-all that has this information implicit.
All update methods have error variants defined for their responses that cover the error cases for the respective call. For query methods, error variants are only defined for methods for which specific errors need to be handled. Other query calls do not have error variants defined for their responses to keep the API simple. Both query and update calls can trap in specific error circumstances instead of returning an error.
1 Like
Subtle and excellent point you make here. Would making spender in the response and adding the according text be enough to address this?
What would be a viable alternative to keeping all approvals and approval revocations in the transaction log? If only approvals, but not their revocations, were stored in the log, or neither of them, then the ledger would not be auditable. This would not be acceptable from a security perspective.
Note that an implementation could batch multiple revocations into a single block, reducing storage overhead a bit. This is supported by the API here as the responses of multiple revocations could contain the same transaction index.
Rate limiting could solve the DoS issue you sketch:
- A canister should, according to the standard, limit the number of approvals per token and collection-level approvals per principal.
- If an implementation adds further rate limiting to how often approvals may be created / revoked for a token or an identity, the issue would be better addressed. An attacker with many principals could, of course, still create and revoke many collection-level approvals. Maybe an implementation can limit such operations to entities that own one or more NFTs of this collection.
Would the above solve the denial of service issue sufficiently?
I think it all needs to be in the ledger to ‘recreate the state’ via the ledger. The one caveat to this is that once we get to uploading files, we probably don’t want those in the ledger, but can instead use a hash of the file or data change(this leads to a data availability problem, but is likely less of an issue on the IC since we can store the files “forever”).
As you mentioned, we can have the record be a batch record to save on some space, but it can still get big. I like having individual records so I can easily build an index with all the records I need to retrieve when we add an icrcX_token_history endpoint. We may want to leave this up to an implementation detail and state as much. It becomes less of an implementation detail when we do the ICRCX discussion that defines the transaction log entries for NFTs in the same way that ICRC3 does for the ICRC1/2 transactions. It may be fine to say we’ll define it in the future.
While its in my head, when we do get tho that we may want a btapprove and xbtapprvoe, and an approve and xapprove op types that give intent as to if the record is batch or not. A ledger could elect to never use batch if it wants to(or maybe never use the singular as the bt would have an array as one of the items and that array could have one item in it.
1 Like
I think file e.g. file hashes could be stored in token metadata. Updating existing token metadata is outside of the standard scope at the moment. If a extension standard does offer this, then these token metadata mutations could be part of the log as defined in that standard. Minting tokens and their metadata is not even in scope to begin with 
But considering that NFTs could be more than pictures/videos/other media but also e.g. domain names, I think concepts like files and metadata structure should stay out of scope of this base standard.
2 Likes
Yep…not trying to drag it in now…just recording for posterity as we’ll eventually have to deal with it. I’ve actually have been keeping some notes for a content ICRC that would be comparable. I’ll try to write it up when I get a chance.
1 Like
Here is a Motoko implementation of the “old” standard
1 Like
hi i found an inconsistencies here
the function name differs
vs.
which name should we use? i prefer with the word _tokens since it’s clearer and we have _collection too.
1 Like
The implementation is based on an earlier draft on the standard, hence the deviation. The working group will vote on the standard soon, then we will have some reference implementations to see if some minor things need to be changed and then we plan to submit the standard to the NNS for voting.
2 Likes