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

Remove hooks from ERC20 #3838

Merged

Conversation

JulissaDantes
Copy link
Contributor

@JulissaDantes JulissaDantes commented Nov 28, 2022

Fixes #3535

The current way to customize token transfers(mint, burn, transfer) is by overriding the hooks _beforeTokenTransfer and _afterTokenTransfer. This refactor removes both hooks from the ERC20 contract, and implements the logic inside the transfer function, so future customizations only rely on overriding a single function.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio frangio changed the title Remove beforeTokenTransfer and afterTokenTransfer hooks from ERC20 Remove hooks from ERC20 Dec 2, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
contracts/mocks/ERC20SnapshotMock.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/ERC20.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/ERC20.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/ERC20.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/extensions/ERC20Capped.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/extensions/ERC20Capped.sol Outdated Show resolved Hide resolved
test/token/ERC20/ERC20.behavior.js Outdated Show resolved Hide resolved
test/token/ERC20/ERC20.test.js Outdated Show resolved Hide resolved
@@ -240,14 +225,6 @@ function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer
});
});
});

describe('when the recipient is the zero address', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since everything goes through the transfer, it cannot know if its coming from mint or burn, so transfer has a check to ensure that from and to cannot be the zero address, but if a user sends a mint to address zero it would received both from and to as the zero address. There is a test for that case on ln 206

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 206 is testing that you cannot mint to address 0. That is a valid test, but its separate from this. This tests that you cannot call transfer to the address 0.

Tests are checking for behavior correctness. Its ok (and I think its needed) to have multiple tests that are resolved by the same require in different context.

I think this test should have been kept.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that is the cause of the coverage decrease.

JulissaDantes and others added 2 commits December 2, 2022 11:10
Co-authored-by: Francisco <frangio.1@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
@frangio
Copy link
Contributor

frangio commented Dec 2, 2022

There is a separate point we need to decide, which is whether the external transfer and transferFrom functions should accept to = address(0).

I think this PR should keep the current behavior, and so we need to add in those external functions the necessary require statements.

This was referenced Sep 10, 2024
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.

3 participants