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

BurnableToken doesn't fire Transfer event #732

Closed
1 task done
rstormsf opened this issue Feb 13, 2018 · 4 comments · Fixed by #735
Closed
1 task done

BurnableToken doesn't fire Transfer event #732

rstormsf opened this issue Feb 13, 2018 · 4 comments · Fixed by #735

Comments

@rstormsf
Copy link
Contributor

rstormsf commented Feb 13, 2018

  • 📈 This is a feature request.

🎉 Description

As a user, I'd expect that BurnableToken when called burn(uint _value) method, it should fire
Transfer(msg.sender, address(0), value); event.

https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/ERC20/BurnableToken.sol#L18

the same as MintableToken does when it fires mint(to, value)
https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/ERC20/MintableToken.sol#L35

    Transfer(address(0), _to, _amount);

Thoughts?

@androolloyd
Copy link

androolloyd commented Feb 13, 2018

Is it a transfer though, it reads to me as a complete destruction of the burned token, rather than movement of it to a 0 address.

@AugustoL
Copy link
Contributor

I think it should have it! we had to change the burn method in the LifToken implementation to add the burn function.

@AugustoL
Copy link
Contributor

@rstormsf can you go ahead and create the PR? assign me as reviewer pls.

@rstormsf
Copy link
Contributor Author

@AugustoL done

@shrugs shrugs added the next label Feb 20, 2018
@shrugs shrugs removed the next label Feb 20, 2018
shrugs pushed a commit that referenced this issue Feb 20, 2018
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this issue Mar 9, 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.

4 participants