-
Notifications
You must be signed in to change notification settings - Fork 241
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
Conversation
There was a problem hiding this 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!
There was a problem hiding this 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
)
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm.
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 getapproved_account_ids
, it should beNone
, but if a contract does have Approval Extension, then we getapproved_account_ids
, it should be at least empty map, NEVER beNone
.