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

list approval ICRC #103

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

list approval ICRC #103

wants to merge 16 commits into from

Conversation

bogwar
Copy link

@bogwar bogwar commented Sep 3, 2024

This is a draft for a standard to list existing allowances

Copy link
Contributor

@sea-snake sea-snake left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments, sorry for the late review 😅

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

One last comment, not sure what the correct method name should be.

On ICRC-37 we have icrc37_get_token_approvals and icrc37_get_collection_approvals (since we have approvals either per token or the whole collection). That would mean that the method name in ICRC-103 along those lines would be icrc103_get_approvals.

Where there's two differences:

  • "approvals" instead of "allowances", which makes sense since the ICRC-2 allowance method returns the allowance (nat) while this ICRC-103 returns (a list of) the whole approval that includes the allowance among other details like from/to and expiry.
  • "get" instead of "list", this is a nitpick, no idea which one of the two would be most consistent across existing ICRC standards, probably requires checking existing ICRC standards in depth to pick one or the other.

@bogwar
Copy link
Author

bogwar commented Sep 24, 2024

One last comment, not sure what the correct method name should be.

On ICRC-37 we have icrc37_get_token_approvals and icrc37_get_collection_approvals (since we have approvals either per token or the whole collection). That would mean that the method name in ICRC-103 along those lines would be icrc103_get_approvals.

Where there's two differences:

  • "approvals" instead of "allowances", which makes sense since the ICRC-2 allowance method returns the allowance (nat) while this ICRC-103 returns (a list of) the whole approval that includes the allowance among other details like from/to and expiry.
  • "get" instead of "list", this is a nitpick, no idea which one of the two would be most consistent across existing ICRC standards, probably requires checking existing ICRC standards in depth to pick one or the other.

For NFTs, approvals is appropriate because there is no partial transfer so you list outstanding approvals. For fungible tokens allowance seems more suitable because you don't list the original approvals but rather whatever is left to be transferred because some partial transfer_from may have occurred... so that's more of an allowance.
I don't have strong feelings about get or list so I'm happy to go with get for consistency. WDYT?

@sea-snake
Copy link
Contributor

sea-snake commented Sep 24, 2024

For NFTs, approvals is appropriate because there is no partial transfer so you list outstanding approvals. For fungible tokens allowance seems more suitable because you don't list the original approvals but rather whatever is left to be transferred because some partial transfer_from may have occurred... so that's more of an allowance. I don't have strong feelings about get or list so I'm happy to go with get for consistency. WDYT?

Makes completely sense, also in ICRC-2 there's the allowance method while in ICRC-37 there's the is approved method. So to keep it consistent along those lines, allowances is indeed the better option.

Just double checked most ICRC standards, can't find any method at first glance that use "list" in the method name, so let's go with "get".

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.

2 participants