Update contracts to support Solidity 0.8.x #2442

andrekorol · 2020-12-17T01:01:42Z

Contracts' solidity version pragmas were updated to support Solidity 0.8.x.

Changes include:

Updated explicit type conversions that conform with the new restrictions.
Functions that rely on chainid in inline assembly are now view instead of pure.

Important Note: Some tests are still not passing because since 0.8.0 failing assertions and other internal checks like division by zero or arithmetic overflow do not use the invalid opcode, but instead the revert opcode. And I believe that this is something worth discussing how to handle before making any changes to the current test cases.

contracts/access/TimelockController.sol

contracts/GSN/GSNRecipientERC20Fee.sol

frangio · 2020-12-18T01:59:21Z

Thanks @andrekorol! We in fact want to remove all uses of SafeMath from the library now that checked arithmetic is built into Solidity.

We have to discuss whether we want to remove SafeMath entirely, though. It may make sense to deprecate it but still keep the interface for a while for backwards compatibility, and removing the manual checks instead just relying on Solidity's checks. There are some subtleties like the custom error messages that are produced by some of the operations, but that has been reported recently to increase gas cost unnecessarily so I think we will want to remove it.

These changes will mean that Solidity 0.8 support will happen in a new major version of the library. We plan to release a final version of the current major before that happens, so we will merge the migration to Solidity 0.8 into a separate branch.

So I will ask, if you can, to change pragmas to pragma solidity ^0.8.0;, remove uses of SafeMath, and also to resolve conflicts because I merged a related PR that we had pending since before this one.

andrekorol · 2020-12-18T02:14:06Z

@frangio I can make and commit those changes, no problem! I agree with Solidity 0.8 support coming in a new major version of the library, I don't think it makes much sense to have all the changes you mentioned in a new minor version.

… of pure

andrekorol · 2020-12-18T05:51:43Z

@frangio I've made the necessary changes. Some tests are still not passing because they expect custom error messages instead of just VM Exception while processing transaction: revert.

I still didn't make any change to the tests though, I wanted to confirm if the correct approach would be to change the expected custom error messages to the default VM Exception while processing transaction: revert.

Also, the Strings utility library seems to be broken now, and I'm still not sure why, the only thing changed was the version pragma. I suppose it's something to do with the new restrictions on type conversions...

andrekorol · 2020-12-18T08:23:18Z

So, apparently the Strings library wasn't working since solc-0.7 because it was not possible to explicitly convert from uint256 to bytes1.

But with 346fbe2, it's working again.

contracts/mocks/EIP712External.sol

lephleg · 2020-12-21T19:54:40Z

Great work, @andrekorol! Thanks for your efforts on this.
Please let me know if you require a spare hand implementing or reviewing anything particular.

frangio · 2020-12-21T20:55:22Z

I still didn't make any change to the tests though, I wanted to confirm if the correct approach would be to change the expected custom error messages to the default VM Exception while processing transaction: revert.

The test helpers have an expectRevert.unspecified function that you can use.

frangio

Thanks @andrekorol. Most of the changes look good.

I'm now having doubts about what we should do regarding the custom revert reasons on subtraction overflow. Maybe we should keep them. What do other people think about this? Should we keep custom revert reasons such as "insufficient balance"?

Can you also change SafeMath to remove our manual checks and rely only on the built in checked arithmetic?

frangio · 2020-12-21T20:56:31Z

contracts/GSN/Context.sol

    function _msgSender() internal view virtual returns (address payable) {
-        return msg.sender;
+        return payable(msg.sender);


Since Solidity changed the type of msg.sender we should change the return type of our _msgSender function accordingly, instead of casting it here.

frangio · 2020-12-21T21:11:29Z

contracts/drafts/EIP712.sol

+    function _getChainId() private view returns (uint256 chainId) {
+        chainId = block.chainid;
    }


We can remove this function now and just use the built in.

frangio · 2020-12-21T21:12:11Z

contracts/mocks/EIP712External.sol

+    function getChainId() external view returns (uint256 chainId) {
+        chainId = block.chainid;
    }


frangio · 2020-12-21T21:15:49Z

contracts/mocks/ERC20PermitMock.sol

+    function getChainId() external view returns (uint256 chainId) {
+        chainId = block.chainid;
    }


thegostep · 2020-12-21T23:14:44Z

Should we keep custom revert reasons such as "insufficient balance"?

I generally prefer explicit revert messages now that it is convenient to test for them with waffle.

aaronovz1 · 2020-12-21T23:26:12Z

Should we keep custom revert reasons such as "insufficient balance"?

I generally prefer explicit revert messages now that it is convenient to test for them with waffle.

Would custom messages undo the gas savings from using built in arithmetic checker? This is the best part of 0.8 IMO - we wouldn't want to lose that benefit.

frangio · 2020-12-21T23:28:32Z

We have to run some tests and measure the impact. I don't know if there is any special magic in the built in arithmetic checker that would make it more efficient than manual checks.

msg.sender have the type address instead of address payable since Solidity 0.8.0

andrekorol · 2020-12-24T00:48:35Z

Great work, @andrekorol! Thanks for your efforts on this.
Please let me know if you require a spare hand implementing or reviewing anything particular.

@lephleg Help would be appreciated in fixing tests that are failing under coverage (solidity-coverage/ganache) 😄

andrekorol · 2020-12-24T01:34:37Z

Should we keep custom revert reasons such as "insufficient balance"?

One way to retain custom revert reasons would be to keep functions with different signatures like sub(uint256 a, uint256 b) and sub(uint256 a, uint256 b, string memory errorMessage), where in the one with the errorMessage, manual checks are performed as before, and the actual operation is performed inside an unchecked block.

lephleg · 2020-12-24T02:30:05Z

@lephleg Help would be appreciated in fixing tests that are failing under coverage (solidity-coverage/ganache) 😄

At first glance it seems there's a memory leak introduced in node. I wouldn't allocate more before ensuring there's nothing sketchy in the changes. Tricky one, I'll give it a shot with clear head tomorrow.

axic · 2020-12-24T12:37:32Z

Can you also change SafeMath to remove our manual checks and rely only on the built in checked arithmetic?

@frangio does it actually make sense to keep SafeMath as a weird wrapper? It adds no benefit, and the "special error message" versions are removed in this PR.

Perhaps it is better biting the bullet by removing it, and that could trigger users to review the new semantics and use them properly (checked vs. unchecked). And also be aware of the gas implications.

andrekorol · 2020-12-25T19:00:23Z

Can you also change SafeMath to remove our manual checks and rely only on the built in checked arithmetic?

@frangio does it actually make sense to keep SafeMath as a weird wrapper? It adds no benefit, and the "special error message" versions are removed in this PR.

Perhaps it is better biting the bullet by removing it, and that could trigger users to review the new semantics and use them properly (checked vs. unchecked). And also be aware of the gas implications.

@axic, as I've said in the comment above, it's possible to revert those changes that removed the "special error messages" and have SafeMath overload functions with two different signatures. One would be just that weird wrapper, and the other would perform the manual checks with custom error messages. The actual arithmetic operation would be wrapped in an unchecked block to avoid wasting gas with double-checking.

hantuzun · 2020-12-28T12:07:22Z

Removing SafeMath is tedious but a straightforward refactoring, but its benefit to the ecosystem would worth the effort. I'm against keeping it as a wrapper.

Edit: Happy holidays and thank you for the work going on in this PR ✨

axic · 2020-12-29T21:24:01Z

test/token/ERC1155/ERC1155.behavior.js

@@ -208,7 +208,7 @@ function shouldBehaveLikeERC1155 ([minter, firstTokenHolder, secondTokenHolder,
      });

      it('reverts when transferring more than balance', async function () {
-        await expectRevert(
+        await expectRevert.unspecified(


Should OpenZeppelin/openzeppelin-test-helpers#149 be implemented, tests like these would not lose precision as much as in this current PR.

andrekorol · 2020-12-30T01:15:02Z

@hantuzun Thanks, happy holidays to you as well!!

Should I remove SafeMath and the related tests, including cases like the one @axic mentioned on his last review?

axic · 2020-12-30T12:15:09Z

Just to be clear, I am not part of OpenZeppelin, so my suggestions are only suggestions, not an official review 😃

andrekorol · 2020-12-30T14:33:51Z

No problem, thanks for the review anyway!

andrekorol · 2020-12-30T14:35:16Z

@frangio is there a consensus on whether SafeMath should be removed?

Do you think setting up a poll before making any further changes is a good idea?

Amxx

Everything looks good to me.

I was expecting a change of the proxies, using the new fallback (bytes calldata _input) external [payable] returns (bytes memory _output) signature, but considering the assembly part is in the _delegate internal function, It might do more harm then good to embrace this change.

Amxx · 2021-01-05T10:30:51Z

My grain of salt:
I think the current version of SafeMath is usefull, and we should be kept in the codebase. Its just a wrapper around the solidity built-in op, so it should be optimized trivially ... and it makes old code more easily portable to 0.8.

Also, it can be used to force/enable safemath inside unchecked {} blocks:

contract Test {
    uint256 constant internal x = 0;
    uint256 constant internal y = 1;
    function a() public pure returns(uint256) { return x - y; } // revert
    function b() public pure returns(uint256) { unchecked { return x - y; } } // overflow
    function c() public pure returns(uint256) { return SafeMath.sub(x, y); } // revert
    function d() public pure returns(uint256) { unchecked { return SafeMath.sub(x, y); } } // revert
}
```

Amxx · 2021-01-05T10:36:47Z

Note for the future: Some "low level" functions, such as Array::findUpperBound or String::toString could probably benefit from using unchecked to reduce gas cost.

frangio · 2021-01-08T19:20:05Z

Let's keep SafeMath in this PR and discuss its removal separately. I've created an issue dedicated to it, please participate! Go to #2465.

We have to take a look at what's causing the coverage job to fail. Otherwise the PR seems good to go. Thanks for working on this @andrekorol!

andrekorol · 2021-01-08T20:52:20Z

It was my pleasure!

I think the coverage job failing has something to do with trufflesuite/ganache/issues/515.

It should be fixed in ganache@3.0.0-internal.0, but I'll take a closer look at it to see if that's really the case.

andrekorol · 2021-01-09T04:00:44Z

Initially, coverage was failing due to an out of memory error, which could be solved by either updating Node's version here to any version >= 14:

openzeppelin-contracts/.github/workflows/test.yml

Line 35 in 318c4b4

node-version: 10.x

or by increasing the heap memory as I did in 6a30dcc.

Now the coverage job is failing because of six different test cases that are not passing. Two of them (domain separator) I assume to be related to that chainId issue.

frangio · 2021-01-12T23:04:16Z

I think you have debugged it correctly, it's likely to be the chainId issue.

Something else we will have to do afterwards is review all of the expectRevert.unspecified in tests and add back the revert reasons. We will be able to bring them back with the functionality we're adding in #2462.

Thank you everyone! Please remember this is going in the solc-0.8 branch, and we will do our best to release it as soon as possible as version 4.0 of the library. We are now preparing 3.4 as the last release of the 3.x line.

frangio · 2021-01-12T23:21:30Z

A correction: while doing a last review of the changes in this PR I found that the chain id issue in Ganache already had a workaround and it was being removed here. It fixed the coverage for me locally and hopefully the CI will agree.

andrekorol mentioned this pull request Dec 17, 2020

Support for Solidity 0.8.x #2441

Closed

thegostep reviewed Dec 17, 2020

View reviewed changes

contracts/access/TimelockController.sol Outdated Show resolved Hide resolved

aaronovz1 reviewed Dec 17, 2020

View reviewed changes

contracts/GSN/GSNRecipientERC20Fee.sol Outdated Show resolved Hide resolved

kamiebisu force-pushed the fix/solc-0.8-#2441 branch from b72352c to 5079cd1 Compare December 18, 2020 03:20

andrekorol force-pushed the fix/solc-0.8-#2441 branch from 4e171e8 to 5079cd1 Compare December 18, 2020 03:28

andrekorol added 3 commits December 18, 2020 00:28

Update contract pragmas to include 0.8.x

13fdcf3

Update the solc version on Buidler's config

52c9ee9

The chainid builtin in inline assembly is now considered view instead…

4ccca4e

… of pure

andrekorol force-pushed the fix/solc-0.8-#2441 branch from 5079cd1 to 4ccca4e Compare December 18, 2020 03:29

andrekorol added 4 commits December 18, 2020 00:45

Update version pragmas to ^0.8.0

3a230e2

Remove uses of SafeMath

a124c43

Remove ignored constructor visibility specifiers

587e067

Remove redundant ABI decoder pragma, v2 is now the default

f9173c7

andrekorol force-pushed the fix/solc-0.8-#2441 branch 2 times, most recently from f82ef07 to d2ca110 Compare December 18, 2020 09:05

axic reviewed Dec 18, 2020

View reviewed changes

contracts/mocks/EIP712External.sol Outdated Show resolved Hide resolved

Fix conversion from uint256 to string

346fbe2

andrekorol force-pushed the fix/solc-0.8-#2441 branch from d2ca110 to 346fbe2 Compare December 18, 2020 18:57

Use Solidity's block.chainid instead of relying on inline assembly

768d92c

frangio changed the base branch from master to solc-0.8 December 21, 2020 21:13

frangio reviewed Dec 21, 2020

View reviewed changes

andrekorol added 4 commits December 21, 2020 22:03

Update the type of msg.sender

c25abfd

msg.sender have the type address instead of address payable since Solidity 0.8.0

Remove manual checks in favor of the built-in checked arithmetic

31821b7

Remove custom revert reasons from tests that revert without one

28845d3

Update version pragma and remove constructor visibility specifier

38aadc5

andrekorol force-pushed the fix/solc-0.8-#2441 branch from cd0dc80 to 38aadc5 Compare December 24, 2020 00:34

axic reviewed Dec 29, 2020

View reviewed changes

Amxx reviewed Jan 5, 2021

View reviewed changes

frangio mentioned this pull request Jan 8, 2021

Discussion: Should we remove the SafeMath library for Solidity 0.8? #2465

Closed

Increase Node's heap memory on the coverage job

6a30dcc

Fix coverage failing due to chain id

8627479

increase gas for Address test

46c1783

frangio merged commit 974c534 into OpenZeppelin:solc-0.8 Jan 14, 2021

neutiyoo mentioned this pull request May 8, 2022

Consider using Solidity v0.8 alt-research/altbridge-solidity#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update contracts to support Solidity 0.8.x #2442

Update contracts to support Solidity 0.8.x #2442

andrekorol commented Dec 17, 2020

frangio commented Dec 18, 2020

andrekorol commented Dec 18, 2020

andrekorol commented Dec 18, 2020

andrekorol commented Dec 18, 2020 •

edited

Loading

lephleg commented Dec 21, 2020

frangio commented Dec 21, 2020

frangio left a comment •

edited

Loading

frangio Dec 21, 2020

frangio Dec 21, 2020

frangio Dec 21, 2020

frangio Dec 21, 2020

thegostep commented Dec 21, 2020

aaronovz1 commented Dec 21, 2020

frangio commented Dec 21, 2020

andrekorol commented Dec 24, 2020

andrekorol commented Dec 24, 2020 •

edited

Loading

lephleg commented Dec 24, 2020 •

edited

Loading

axic commented Dec 24, 2020

andrekorol commented Dec 25, 2020 •

edited

Loading

hantuzun commented Dec 28, 2020 •

edited

Loading

axic Dec 29, 2020

andrekorol commented Dec 30, 2020

axic commented Dec 30, 2020

andrekorol commented Dec 30, 2020

andrekorol commented Dec 30, 2020

Amxx left a comment

Amxx commented Jan 5, 2021 •

edited

Loading

Amxx commented Jan 5, 2021

frangio commented Jan 8, 2021

andrekorol commented Jan 8, 2021

andrekorol commented Jan 9, 2021 •

edited

Loading

frangio commented Jan 12, 2021 •

edited

Loading

frangio commented Jan 12, 2021

Update contracts to support Solidity 0.8.x #2442

Update contracts to support Solidity 0.8.x #2442

Conversation

andrekorol commented Dec 17, 2020

frangio commented Dec 18, 2020

andrekorol commented Dec 18, 2020

andrekorol commented Dec 18, 2020

andrekorol commented Dec 18, 2020 • edited Loading

lephleg commented Dec 21, 2020

frangio commented Dec 21, 2020

frangio left a comment • edited Loading

Choose a reason for hiding this comment

frangio Dec 21, 2020

Choose a reason for hiding this comment

frangio Dec 21, 2020

Choose a reason for hiding this comment

frangio Dec 21, 2020

Choose a reason for hiding this comment

frangio Dec 21, 2020

Choose a reason for hiding this comment

thegostep commented Dec 21, 2020

aaronovz1 commented Dec 21, 2020

frangio commented Dec 21, 2020

andrekorol commented Dec 24, 2020

andrekorol commented Dec 24, 2020 • edited Loading

lephleg commented Dec 24, 2020 • edited Loading

axic commented Dec 24, 2020

andrekorol commented Dec 25, 2020 • edited Loading

hantuzun commented Dec 28, 2020 • edited Loading

axic Dec 29, 2020

Choose a reason for hiding this comment

andrekorol commented Dec 30, 2020

axic commented Dec 30, 2020

andrekorol commented Dec 30, 2020

andrekorol commented Dec 30, 2020

Amxx left a comment

Choose a reason for hiding this comment

Amxx commented Jan 5, 2021 • edited Loading

Amxx commented Jan 5, 2021

frangio commented Jan 8, 2021

andrekorol commented Jan 8, 2021

andrekorol commented Jan 9, 2021 • edited Loading

frangio commented Jan 12, 2021 • edited Loading

frangio commented Jan 12, 2021

andrekorol commented Dec 18, 2020 •

edited

Loading

frangio left a comment •

edited

Loading

andrekorol commented Dec 24, 2020 •

edited

Loading

lephleg commented Dec 24, 2020 •

edited

Loading

andrekorol commented Dec 25, 2020 •

edited

Loading

hantuzun commented Dec 28, 2020 •

edited

Loading

Amxx commented Jan 5, 2021 •

edited

Loading

andrekorol commented Jan 9, 2021 •

edited

Loading

frangio commented Jan 12, 2021 •

edited

Loading