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

Revisit nft_resolve_transfer args #568

Closed
mikedotexe opened this issue Sep 15, 2021 · 3 comments
Closed

Revisit nft_resolve_transfer args #568

mikedotexe opened this issue Sep 15, 2021 · 3 comments

Comments

@mikedotexe
Copy link
Contributor

mikedotexe commented Sep 15, 2021

According to the standard:
https://nomicon.io/Standards/NonFungibleToken/Core.html#nft-interface

nft_resolve_transfer has an argument approved_account_ids that is supposed to be string[] but I see there's a HashMap here. There's also a place where that field calls approvals, which should be fixed.

It seems like we don't need to pass the entire HashMap and can stick to the standard and just send a Vec instead.

This is one of those tickets where I noticed it while I'm in the middle of something, so don't have more info other than this at the moment.

@austinabell
Copy link
Contributor

austinabell commented Sep 15, 2021

What we probably want here is a Set (probably BTreeSet) for this. A set will serialize/deserialize as an array but comes with the property that we don't have to check for duplicates manually and can still do the lookup more efficiently than iterating through.

I had a quick look and still don't understand the use of storing an integer value for each one. I'll dig into this more tomorrow. (seems like approval ids are currently required to be unique per token, not quite sure why though)

@austinabell
Copy link
Contributor

dug slightly more into this, but it's clear there needs to be discussion around this because I need more context around what the flow is intended to be, and the fact that this is quite a breaking change so if the API is changed we need to be careful.

@frol
Copy link
Collaborator

frol commented Jun 23, 2023

Resolved in #1019

@frol frol closed this as completed Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants