Skip to content

There is no burn event though this comment suggests otherwise #1371

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
1 task done
mswezey23 opened this issue Oct 2, 2018 · 2 comments · Fixed by #1373
Closed
1 task done

There is no burn event though this comment suggests otherwise #1371

mswezey23 opened this issue Oct 2, 2018 · 2 comments · Fixed by #1373
Labels
contracts Smart contract code.
Milestone

Comments

@mswezey23
Copy link
Contributor

mswezey23 commented Oct 2, 2018

🎉 Description

  • 🐛 This is a bug report.

📝 Details

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/f4d6f404424669e0eef7faa05fd89ab9945f265b/contracts/token/ERC20/ERC20Burnable.sol#L29

/**
   * @dev Overrides ERC20._burn in order for burn and burnFrom to emit
   * an additional Burn event.
   */
  function _burn(address who, uint256 value) internal {
    super._burn(who, value);
  }
@frangio
Copy link
Contributor

frangio commented Oct 2, 2018

Good catch @mswezey23! The whole _burn override can actually be removed. It is left over from when we had a Burn event, before removing it in #1305 (it had been renamed TokensBurned shortly before).

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/f4d6f404424669e0eef7faa05fd89ab9945f265b/contracts/token/ERC20/ERC20Burnable.sol#L28-L34

Are you interested in sending a pull request with these changes?

@frangio frangio added kind:improvement contracts Smart contract code. labels Oct 2, 2018
Aniket-Engg added a commit to Aniket-Engg/zeppelin-solidity that referenced this issue Oct 3, 2018
nventuro pushed a commit that referenced this issue Oct 3, 2018
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1371

* Removed extra whitespace
@mswezey23
Copy link
Contributor Author

mswezey23 commented Oct 3, 2018

Next time I'll issue a PR for the changes. Looks like @Aniket-Engg beat me to it ;)

I assume the same/similar occurred for the Mint event?

Edit: Nevermind - read the #1305

nventuro pushed a commit that referenced this issue Oct 4, 2018
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1371

* Removed extra whitespace

(cherry picked from commit f3888bb)
@come-maiz come-maiz added this to the v2.0 milestone Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants