-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Initial migration to Solidity 0.6.x - v3.0 first steps #2063
Conversation
For reference, this is how creating an pragma solidity ^0.6.0;
import "../token/ERC20/ERC20Mintable.sol";
import "../token/ERC20/ERC20Pausable.sol";
contract MyERC20 is ERC20Mintable, ERC20Pausable {
function _afterTokensMoved(address from, address to, uint256 amount) internal override(ERC20, ERC20Pausable) {
super._afterTokensMoved(from, to, amount);
}
function _afterTokensApproved(address from, address to, uint256 amount) internal override(ERC20, ERC20Pausable) {
super._afterTokensApproved(from, to, amount);
}
} Note that the
|
I like the decision of including the hooks, however there is one downside I see: most overrides actually add preconditions rather than side-effects. By adding the precondition check after the action occurs, the gas expenditure of a failed tx is made much higher. I'd consider adding both After this, I think the next step would be to decide which off-the-shelf combinations we want to ship for ERC20, ERC721, and ERC777. I'd consider the following to guide this decision:
|
For simplicity's sake I wanted to add a single hook (either 'before' or 'after'), and 'after' has the nice property of all values having been already validated (we could also use 'before' and keep this property, though it'd require more careful code - I'd rather we didn't). Having both 'before' and 'after' could work: after all, once the user gets to our docs and figures out how to fix the issue, whether they need to fix one or four overrides will not be very important. There is one more reason to prefer less hooks however: the error messages are rather large, and four of them (two for each hook) will fill up a screen. C++ taught us this is less than ideal. Do you know how many transactions actually revert on mainnet though? I often feel optimizing the gas cost of reverts is not very useful due to these transactions never being sent in the first place. |
Hmm if I cannot convince you with the gas argument, let me pull out the big guns: forcing the dev to add the preconditions at the end breaks the check-effect-interaction security pattern. I wouldn't go against this, even if it implies larger compilation error msgs. |
Hm, I can get behind that. ERC20 doesn't really have an 'interaction' part, but it still makes sense to promote that pattern. Alright, what about
We'd extend the same general guiding principles to approvals. |
Works for me! Do we want the |
No, I wouldn't allow for that. The purpose behind hooks is to add more functionality (such as validations or extensions), not to alter the original one. Your comment made me realize however that anyone who overrides |
Looks like Solhint cannot handle 0.6.x code, so we won't be able to lint Solidity code until the new version comes out. |
It looks like solidity-docgen doesn't support 0.6 syntax yet, so the API reference module crashes: we won't get previews for that until we fix docgen: OpenZeppelin/solidity-docgen#133 |
This should now be ready for an initial alpha release: the library is in a state similar to v2.4, albeit with Solidity v0.6 keywords (so explicit The final v3.0 release won't happen for at least a few weeks: we still need to iron out the details of the migration and work on new documentation. The main purpose behind this alpha release would be to enable early adopters that want to switch to v0.6 as soon as possible, hopefully gathering feedback during the stabilization process. Regarding the hooks described above, I went ahead with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! It seems the before/after hooks distinction was indeed a good idea.
@@ -63,6 +63,8 @@ contract GSNRecipientERC20Fee is GSNRecipient { | |||
) | |||
external | |||
view | |||
virtual | |||
override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope the linter decides to put these 4 words on a single line
863036e
to
70cddaa
Compare
I rebased on top of |
* Initial migration to Solidity 0.6.x - v3.0 first steps (#2063) * Initial migration, missing GSN, 721, 777 and Crowdsales. * Add _beforeTokenOperation and _afterTokenOperation. * Add documentation for hooks. * Add hooks doc * Add missing drafts * Add back ERC721 with hooks * Bring back ERC777 * Notes on hooks * Bring back GSN * Make functions virtual * Make GSN overrides explicit * Fix ERC20Pausable tests * Remove virtual from some view functions * Update linter * Delete examples * Remove unnecessary virtual * Remove roles from Pausable * Remove roles * Remove users of roles * Adapt ERC20 tests * Fix ERC721 tests * Add all ERC721 hooks * Add ERC777 hooks * Fix remaining tests * Bump compiler version * Move 721BurnableMock into mocks directory * Remove _before hooks * Fix tests * Upgrade linter * Put modifiers last * Remove _beforeTokenApproval and _beforeOperatorApproval hooks
This is a work in progress for a migration of the library to 0.6.x.
This migration is particularly problematic because of the addition of
override
. We rely on multiple-inheritance and the linearization of the inheritance graph to optionally extend behavior by inheriting from multiple variants (e.g.MyToken is ERC20Mintable, ERC20Pausable
will combine both) that each override some base functions, often calling the parent version as well (viasuper
).In 0.6.x, this must be made explicit on the derived contract whenever there is diamond inheritance. Otherwise, an error message similar to the following will be displayed:
This will be a new burden placed on users of the library, and is sadly unavoidable (unless we were to provide every possible combination of variants, which wouldn't scale). To alleviate the situation, we decided to:
super.foo()
_transfer
,_mint
and_burn
, there'll be a single_afterTokensMoved
function. This also came up as a need recently in Made private methods internal to allow for overriding #2027 and Made ERC777 registry and private functions internal. #2048.What is currently implemented in this PR:
virtual
_afterTokensMoved
and_afterTokensApproved
ERC20 extension points, use those to implement variants (the same pattern would be repeated for 721 and 777)