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

fix: approved account ids #1021

Merged
merged 4 commits into from May 24, 2023
Merged

Conversation

hanakannzashi
Copy link
Contributor

According to the comments and nft_token implementation, I think 'Extension Not Exists' vs 'Extension Exist But Value Not Exists' is different. i.e. If a contract doesn't have Approval Extension, when we get approved_account_ids, it should be None, but if a contract does have Approval Extension, then we get approved_account_ids, it should be at least empty map, NEVER be None.

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@hanakannzashi This is definitely a good usability improvement!

Copy link
Contributor

@miraclx miraclx left a comment

Choose a reason for hiding this comment

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

Seems the benefit here (in the case that approval extensions are supported, but the map is empty) is that the error moves from being "Unauthorized" to "Sender not approved" and if approval extensions are not supported, the panic message is "Approval extension is disabled"

Just clearing that out of the way because even though account approvals are supported, but an empty map is provided, WE WILL STILL panic, just with an arguably, better error message.

The only case that only ever successfully executes, is if account approvals are supported (AND the non-empty associative map contains the sender_id)

@frol
Copy link
Collaborator

frol commented May 24, 2023

@miraclx Yes, that is it. It is a minor yet welcome UX improvement. I am not a codeowner here, can you approve it if it looks fine to you?

P.S. I was just looking around for some good-first-issues and realized that there are a bunch of open PRs in this repo, so I started to take a look.

Copy link
Contributor

@miraclx miraclx left a comment

Choose a reason for hiding this comment

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

Lgtm.

@frol frol merged commit d8e40d6 into near:master May 24, 2023
14 checks passed
@hanakannzashi hanakannzashi deleted the fix/approved_account_ids branch August 24, 2023 03:07
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.

None yet

3 participants