Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICRC-7 including WG comments and other changes #33

Merged
merged 87 commits into from
Feb 1, 2024
Merged

Conversation

dietersommer
Copy link
Collaborator

@dietersommer dietersommer commented Sep 25, 2023

No description provided.

@@ -160,7 +277,7 @@ type TransferArgs = record {
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// type: leave open for now

This should probably be defined similar to ICRC-1 spec:

The memo parameter is an arbitrary blob that has no meaning to the ledger. The ledger SHOULD allow memos of at least 32 bytes in length.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this is already in the text below the data structure:

The memo parameter is an arbitrary blob that is not interpreted by the ledger.
The ledger SHOULD allow memos of at least 32 bytes in length.
The ledger SHOULD use the memo argument for transaction deduplication.

Is this what you meant?

ICRCs/ICRC-7/ICRC-7.md Outdated Show resolved Hide resolved
@sea-snake
Copy link
Contributor

sea-snake commented Oct 10, 2023

As mentioned during the meeting, a token could set a high approval limit and/or have large blob sizes in their memo fields.

Currently to get token approvals we have:

icrc7_get_approvals: (token_ids : vec nat)
    -> (vec record { token_id : nat; approvals : vec record { token_id : nat; spender : Account; expires_at : opt nat64; created_at_time : opt nat64; } });

To add support for high approval limits and/or large memo fields, skip take would need to be added.

During this discussion we mentioned that skip take per token id or a batch query call would be complicated because we would have a list of skip take lists, this isn't user friendly.

The first thing that currently comes to mind is considering the query token_ids argument as filter and returning a list of approvals that isn't grouped by token id. In combination with sorting on e.g. created_at_time, one could do a skip take where skip is the created at time. This basically would allow a user to get approvals before/after a specific time (depending on sort order).

icrc7_get_approvals: (token_ids : opt vec nat, skip: opt nat64; take: opt nat)
    -> (vec record { token_id : nat; spender : Account; expires_at : opt nat64; created_at_time : opt nat64; });

This would also allow for more filters besides token_ids so e.g. a marketplace could get all approvals is has received.

icrc7_get_approvals: (opt token_ids : opt vec nat, owner: opt Account; spender: opt Account; skip: opt nat64; take: opt nat)
    -> (vec record { token_id : nat; spender : Account; expires_at : opt nat64; created_at_time : opt nat64; });

If token_ids, owner and spender are all not set, it would return a list of all token approvals within the ledger.

@dietersommer
Copy link
Collaborator Author

dietersommer commented Oct 11, 2023

As mentioned during the meeting, a token could set a high approval limit and/or have large blob sizes in their memo fields.

Currently to get token approvals we have:

icrc7_get_approvals: (token_ids : vec nat)
    -> (vec record { token_id : nat; approvals : vec record { token_id : nat; spender : Account; expires_at : opt nat64; created_at_time : opt nat64; } });

To add support for high approval limits and/or large memo fields, skip take would need to be added.

During this discussion we mentioned that skip take per token id or a batch query call would be complicated because we would have a list of skip take lists, this isn't user friendly.

The first thing that currently comes to mind is considering the query token_ids argument as filter and returning a list of approvals that isn't grouped by token id. In combination with sorting on e.g. created_at_time, one could do a skip take where skip is the created at time. This basically would allow a user to get approvals before/after a specific time (depending on sort order).

icrc7_get_approvals: (token_ids : opt vec nat, skip: opt nat64; take: opt nat)
    -> (vec record { token_id : nat; spender : Account; expires_at : opt nat64; created_at_time : opt nat64; });

This would also allow for more filters besides token_ids so e.g. a marketplace could get all approvals is has received.

icrc7_get_approvals: (opt token_ids : opt vec nat, owner: opt Account; spender: opt Account; skip: opt nat64; take: opt nat)
    -> (vec record { token_id : nat; spender : Account; expires_at : opt nat64; created_at_time : opt nat64; });

If token_ids, owner and spender are all not set, it would return a list of all token approvals within the ledger.

@sea-snake

This is a good point indeed! As discussed in yesterday's meeting we want to reintroduce pagination for this reason.

Your interface is very generic and nice in terms of what one can do for querying approvals. I really like this API. Is this still sufficiently easy to implement? Let's see how it could be implemented reasonably.

Take the simplest case of allowing for queries for approvals for a list of token_ids where the approvals in the response are sorted by created_at_time, henceforth referred to as time also. All of this with pagination and efficient. Fulfilling those requirements requires a data structure that has indexing over token_id and created_at_time built in, or multiple data structures in orchestration.

icrc7_get_approvals: (token_ids : opt vec nat, skip: opt ApprovalInfo, take: opt nat)
-> (vec record { token_id : nat; approval : ApprovalInfo });

ApprovalInfo contains all the attributes of the approval without the token_id it applies to.

An example of such a data structure could be the following: Keep an index over sorted token_ids, each entry pointing to a tree data structure that contains the approvals for the given token_id, sorted by created_at_time.

Each call being made to retrieve take many approvals for a given list of token ids, starting with the approval following one provided as skip parameter can be implemented efficiently using this data structure. The skip parameter is the most recent approval seen in the previous call to the method. Based on its token_id, we need to obtain approvals following this token_id in a time-sorted manner through the tree data structure for this token_id, and return all the ones following the skip approval. Once all approvals have been enumerated for this token_id, iterate to the next higher token id and enumerate approvals until take many tokens can be provided in the response.

What's not yet addressed here is the case of multiple approvals issued with the same time. As the timestamps have nanosecond granularity, this is likely a non-issue and identical time stamps should not be a practical problem.

All of this is not really rocket science, but also not trivial to implement reliably and efficiently. Further filtering capabilities than in the outlined example, i.e., getting to the general API sketched above, require further indices (efficient) or iteration over possibly large data sets (easier to implement, but inefficient for practical data sets).

What do people think about the complexity of such an implementation? Is this something we can put into the base standard and require everyone to implement?

Also: Should't take be int instead of nat type?

@benjizhai
Copy link
Contributor

While I like the general idea, I would like to point out that created_at_time is an optional field.

According to what we discussed during the WG, the skip-take semantics should just use any arbitrary return item as the non-null value for skip, and the implementation may use any internal sorting order.

@sea-snake
Copy link
Contributor

Changes look good to me, approvals look simple enough for the client to fetch them with prev take while the complexity for implementers has been kept simple :D

@sea-snake
Copy link
Contributor

One small consistency detail that might need to be double checked, some methods have nat32 as take type and some have nat.

@sea-snake
Copy link
Contributor

Collection metadata is still vec value, thus has no keys, I thought the ideas was to make that key value (vec record {text; Value})?

ICRCs/ICRC-7/ICRC-7.md Outdated Show resolved Hide resolved
```

```candid "Methods" +=
icrc7_metadata : (nat) -> (vec record { text; Metadata }) query;
icrc7_token_metadata : (token_ids : vec nat)
-> (vec opt metadata: vec record { text; Value }) query;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the candid doesn't seem to match the text above, i don't see any token_id in the response 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, adapted to match our simplified positional API.

Returns the owner of a tokenId.
Returns the owner `Account` of each token in a list `token_ids` of token ids. The ordering of the response elements corresponds to that of the request elements.

Note that tokens for which an ICRC-1 account cannot be found have a `null` response. This can for example be the case for a ledger that has originally used a different token standard, e.g., based on the ICP account model, and tokens of this ledger have not been fully migrated yet to ICRC-7.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we handle the case that a token_id does not exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that they should also receive a null response. Non-existing token ids should not be a major error case: We can require that it's everyone's responsibility to query for existing token ids as all the ids used in the API need to come from some source and thus should be valid. Loosing the differentiation between non-existing and no owner listable is thus not a major issue.

@letmejustputthishere
Copy link
Member

letmejustputthishere commented Jan 29, 2024

in general, the more that i'm looking at this the more i'm wondering if we should extract batching into a separate standard as well to keep ICRC7 as simple as possible 🤔

i wasn't around for the last 2 meetings due to calendar conflicts, but would be happy to understand why we reverted to the index based response.

@dietersommer
Copy link
Collaborator Author

dietersommer commented Jan 29, 2024

in general, the more that i'm looking at this the more i'm wondering if we should extract batching into a separate standard as well to keep ICRC7 as simple as possible 🤔

We can discuss tomorrow, but as far as I remember, we have discussed already and wanted to have the basic API be a batch API.

i wasn't around for the last 2 meetings due to calendar conflicts, but would be happy to understand why we reverted to the index based response.

The new positional API is much simpler and saves space in the response value. Also, it simplifies by removing the top-level error. All of this makes the API much simpler. Also, some requirements like that of all request items must be responded to in the response has been omitted as it was complicating the implementation.

And the new API is aligned with the ICRC-4 standard on batch methods for fungible tokens. The ideas came up in this context and were then brought over to here.

@dietersommer dietersommer marked this pull request as ready for review February 1, 2024 18:37
@dietersommer dietersommer merged commit 5613ab8 into main Feb 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants