Skip to content

[bug] Resetting of value to non-zero when burning last token #105

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

Closed
mg6maciej opened this issue Jun 24, 2018 · 3 comments · Fixed by #110
Closed

[bug] Resetting of value to non-zero when burning last token #105

mg6maciej opened this issue Jun 24, 2018 · 3 comments · Fixed by #110
Assignees

Comments

@mg6maciej
Copy link
Contributor

When burn is called on the last token, code in removeNFToken will not clear idToOwnerIndex for it.
Order of these statements should be changed, but even better to use check for last token mentioned in previous issue #104.

    idToOwnerIndex[_tokenId] = 0;
    idToOwnerIndex[lastToken] = tokenToRemoveIndex;
@mg6maciej mg6maciej changed the title [bug] Resetting of value no non-zero when burning last token [bug] Resetting of value to non-zero when burning last token Jun 24, 2018
@MoMannn
Copy link
Collaborator

MoMannn commented Jun 24, 2018

I'm kind of not sure which approach would be better here.

I would change the order because it is unlikely that an event of burning all the tokens happens often if at all so the check mentioned it #104 would consume more gas overall.

But on the other hand if there would be contract that constantly burns all the tokens the gas price will be much higher for it.

@mg6maciej @xpepermint @lknix Thoughts?

@lknix
Copy link
Contributor

lknix commented Jun 24, 2018

I'd go for simplicity (changing the order) unless gas difference justifies it.

@lknix
Copy link
Contributor

lknix commented Jun 24, 2018

@mg6maciej current fix: #110

@lknix lknix closed this as completed Jun 24, 2018
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 a pull request may close this issue.

3 participants