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/approval change #3

Merged
merged 5 commits into from
Mar 31, 2022
Merged

Fix/approval change #3

merged 5 commits into from
Mar 31, 2022

Conversation

zcstarr
Copy link

@zcstarr zcstarr commented Mar 30, 2022

No description provided.

zcstarr added 2 commits March 30, 2022 13:39
This change comes about because with Multi Token standard there
may be many holders of a single token id. It may be gas prohibitive
to return back all of the approval data for a token in the Token
response.

The solution was to introduce a new method called mt_token_approvals
which allows one to iterate through, the list of token approvals
for a specific token id.

This is a minimal addition that allows consumers of the data to
find all the approvals for a particular token id.
@jriemann
Copy link

Overall, these changes look pretty good. I think it makes sense to have the two new functions as you have them.
One thing we might want to change is the owner_id name, since we use the same name in other contexts in the spec (like in the Token type, where it is used to indicate singular ownership of a token). Perhaps approval_creator_id? or something else?

Adding to what's here, I think we need to update the mt_transfer* methods as well. As it stands, we only take approval_id (a number) as an argument. I think it should take a tuple of (approval_id, owner_id).

Rationale: the most natural structure to keep track of approvals is by using 3 nested maps, basically
token_id -> owner_id -> grantee_id -> (approval_id, amount). This makes the creation & deletion operations very simple to reason about.
However, when calling transfer, if the contract isn't provided with the owner_id, it will need to iterate over keys, testing various owner_id values until it finds a match for the given approval_id. I think this will be pretty messy & inefficient.

And also, mt_resolve_transfer needs to take an argument of all the approvals to be restored in case of failure. Since we have batch transfers wherein each transfer can clear multiple approvals, I think the argument should be (deep breath): approvals: (null | (amount: string, approval_id: number, owner_id: string)[])[] | null .

Thoughts?

@zcstarr
Copy link
Author

zcstarr commented Mar 31, 2022

approvals

I think these adjustments make sense pushing up some changes in a few

@zcstarr
Copy link
Author

zcstarr commented Mar 31, 2022

@jriemann PTAL we let me know what you think.

@jriemann
Copy link

LGTM. Thanks for making the changes.

zcstarr added 3 commits March 31, 2022 16:34
Prior to this fix, we had singular representation for token_ids that
may have many owners. There was no way to resolve approvals for those
types of tokens, typically ft style tokens.

This change links together the owners of the accounts to the ft. So
when an approval occurs we can resolve which account to transfer the
tokens from as well as handle resolution of transfer failures from
a singular token id with many owners.
@zcstarr zcstarr force-pushed the fix/approval-change branch from 86272b5 to 0805d5b Compare March 31, 2022 23:34
@zcstarr zcstarr merged commit 8ecf6ec into master Mar 31, 2022
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