Update pragma to 0.8.20 #4489

Amxx · 2023-07-31T07:41:30Z

We need 0.8.20 for better ERC-7201 compatibility.

changeset-bot · 2023-07-31T07:41:33Z

⚠️ No Changeset found

Latest commit: 81aaffa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Major

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

ernestognw

LGTM 🚢

Amxx · 2023-07-31T15:49:26Z

contracts/utils/Arrays.sol

@@ -57,7 +57,7 @@ library Arrays {
    function unsafeAccess(address[] storage arr, uint256 pos) internal pure returns (StorageSlot.AddressSlot storage) {
        bytes32 slot;
        // We use assembly to calculate the storage slot of the element at index `pos` of the dynamic array `arr`
-        // following https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#mappings-and-dynamic-arrays.
+        // following https://docs.soliditylang.org/en/latest/internals/layout_in_storage.html#mappings-and-dynamic-arrays.


I think we don't use latest here in case the docs ever change in a way that would make the link invalid. For example the tag could change. I'd be ok with changing that to v0.8.20.

Yeah I prefer using a fixed version.

We have a few others using latest. I think we had this conversation in another PR and we decided to do latest iirc. No problem for now, just flagging in case it's worth discussing a better strategy.

I would vote for using a fixed version in all links to Solidity docs.

I'd vote for consistency, and I'd push for using 0.8.x or something that doesn't fixes the docs to an specific patch.

EDIT: Just tried with 0.8.x and 0.8 without success. So I guess we're fine with a fixed version then

pcaversaccio · 2023-08-02T13:05:32Z

Hey guys, could you quickly clarify the need for 0.8.20? My recommendation I always give is to only use Solidity versions >=0.8.20 for EVM networks that support the opcode PUSH0 since solc defaults to shanghai for these versions. And the OZ library can't enforce the paris EVM version anyways, so I'm afraid that people will use it and deploy code on non-PUSH0 supporting EVM chains that either will fail at deployment time already or later at runtime.

Amxx · 2023-08-02T14:22:19Z

We want to benefit from some of the 0.8.20 (and beyond) features. In particular, better support for natspec comments.

Release of our code, in particular compiled bytecode for the upgrade plugin is an issue with PUSH0. IMO we should stick with paris for the artefacts we provide (at least for a while).

If the issue is about devs depending on our lib, using a recent version of the compiler, comilling with default settings, and deploying to chains that don't support PUSH0 ... I'd say its on them for not testing in a proper test network (all chains and L2 have a test duplicate that you should use before you go live). This is going to happen anyway, regardless of the pragma we use.

pcaversaccio · 2023-08-02T14:51:48Z

This is going to happen anyway, regardless of the pragma we use.

Not if you use version 0.8.19. But in any case, please add warnings in the docs as well as in the OpenZeppelin wizard about the PUSH0.

Amxx · 2023-08-02T14:59:16Z

Right, are you proposing that we prevent ourself and our knowledgable users from using new, more efficient, version of the compiler, in order to protect the less knowledgable users?

pcaversaccio · 2023-08-02T15:13:00Z

Right, are you proposing that we prevent ourself and our knowledgable users from using new, more efficient, version of the compiler, in order to protect the less knowledgable users?

No, I only propose to make the default compilation of the OpenZeppelin library "noob-safe" until shanghai is mainstream. Optimsim e.g. doesn't even support PUSH0 yet. Do you really believe that the majority of devs using OZ contracts are the knowledgeable ones? I understand that this is not a released version yet, I'm just raising potential issues if that version is released soon.

JorgeAtPaladin · 2023-08-02T15:23:11Z

Documenting this sounds like a good middle ground

Amxx · 2023-08-02T15:35:14Z

Do you really believe that the majority of devs using OZ contracts are the knowledgeable ones?

That is something that causes many internal dissagrements 😛

I'm personally expecting people that target polygon, to test their entier app (contracts + indexer + front +...) on mumbai first. Is that too much to expect?

I'm just raising potential issues if that version is released soon.

And we thank you for doing so, that is really really valuable to us !

pcaversaccio · 2023-08-02T15:56:27Z

I'm personally expecting people that target polygon, to test their entier app (contracts + indexer + front +...) on mumbai first. Is that too much to expect?

Well, yeah that's how it should work, but expectations usually don't meet reality. I just want to remind you that there was just recently an issue on zkSync where a project deployed a contract that uses transfer instead of call to transfer native tokens out, but since zkSync uses AA with additional logic for which you need gas, every withdraw call reverted with OOG. They unit tested their code, they even fork-tested on mainnet, but didn't fork test on zkSync.

Anyways, I personally think we should highlight this potentially breaking issue everywhere where devs interact in some sense with the OZ library, e.g. GitHub, Docs, NPM side, and the OZ Wizard.

frangio · 2023-08-02T18:36:57Z

Do we need to document this issue if the pragma remains ^0.8.19?

I'm interested in helping prevent these issues but I'm not even confident that people will see these warnings. And the issue is much larger than OpenZeppelin unfortunately.

Amxx · 2023-08-02T18:41:29Z

but didn't fork test on zkSync

When you fork test, do you get the behavior of the forked chain, or just the state? Like if polygon doesn't have PUSH0, and I fork polygon using foundry or hardhat, does my local fork detect that the forked chain doesn't have PUSH0, or does it run a Shanghai node with the state of the remote chain?

pcaversaccio · 2023-08-02T19:08:35Z

Do we need to document this issue if the pragma remains ^0.8.19?

In any case, I would document it properly. Ultimately, it's all about education.

I'm interested in helping prevent these issues but I'm not even confident that people will see these warnings. And the issue is much larger than OpenZeppelin unfortunately.

Agreed, and I don't know how to solve it really.

When you fork test, do you get the behavior of the forked chain, or just the state? Like if polygon doesn't have PUSH0, and I fork polygon using foundry or hardhat, does my local fork detect that the forked chain doesn't have PUSH0, or does it run a Shanghai node with the state of the remote chain?

My understanding (and I can be wrong) is that fork tests are local simulations with accounts & states and storage slot information fetched from the RPC. I.e. you don't get the full chain behaviour and thus can't detect that specific issue.

I think the desired behaviour would look like the following:

Default compilation of any OZ contract defaults to the evm version paris
If dev wants to change it, he needs to adjust the config

E.g. Foundry does default to paris and not shanghai.

The question is whether OZ fully delegates that responsibility to the tooling side at the end of the day.

Amxx · 2023-08-02T19:16:13Z

The question is whether OZ fully delegates that responsibility to the tooling side at the end of the day.

We, and other libraries like PRB math, already do that. We don't control the users development environment. We don't control the users compiler settings. If we were providing a hardat.config.js for reuse we would indeed have a responsability, but that is not the case.

We do provide compiled artefacts in the npm package, which are used by our upgrades plugin. That part we control, and IMO it should be targetting paris (see #4503).

pcaversaccio · 2023-08-02T19:21:00Z

We do provide compiled artefacts in the npm package, which are used by our upgrades plugin. That part we control, and IMO it should be targetting paris (see #4503).

That's great. But you don't think any further documentation is necessary at all?

frangio · 2023-08-02T20:08:33Z

I've opened an issue to set the Hardhat default to paris:

Set default evmVersion to paris NomicFoundation/hardhat#4232

With this change I would feel comfortable keeping the pragma at 0.8.20 and I think there would be less of a need for documentation. It's always nice to help towards education, but having to put warnings in every possible location is an indication that something else is wrong.

pcaversaccio · 2023-08-02T21:55:16Z

I feel we got a good compromise.

Amxx · 2023-08-03T11:50:28Z

But you don't think any further documentation is necessary at all?

We could for sure use more documentation. I'm not sure where we would put that though.

#4503 is a clear actionable. For the documentation part, its not that clear to me what the actionnable is.

pcaversaccio · 2023-08-03T12:06:56Z

In the UI of the OpenZeppelin Wizard and maybe in the Security Center somewhere?

Fixes hats-finance#17 This PR changes the pragmas in the passkeys contract to be: 1. More consistent: we use `^0.8.20` everywhere for "non-top-level" contracts. The version was chosen as this it the first version that supports all of the compiler features that we make use of. Notably, we have `@custom:` NatSpec items which are only officially supported as of v0.8.20 (incidentally, this is the same version used by OpenZeppelin contracts for similar reason: [source](OpenZeppelin/openzeppelin-contracts#4489)). The choice to use floating pragmas here is to make using these support contracts easier across other projects (namely, the `WebAuthn` and `P256` libraries are useful outside of this project). Furthermore, we use `^` versions so that breaking changes introduced in a potential Solidity v0.9 would not affect the security and integrity of the contract code. 2. Use fixed pragmas for "top-level" contracts that we deploy: - `SafeWebAuthnProxyFactory` - `SafeWebAuthnSharedSigner` - `FCLP256Verifier` I am aware that Solidity v0.8.20 doesn't play nice with chains like BNB by default, however this can be worked around by explicitly by setting the EVM version target (as we do in this project) and do not believe this is an issue.

update pragma to 0.8.20

632b9af

Amxx requested review from ernestognw and frangio July 31, 2023 12:14

frangio and others added 5 commits July 31, 2023 12:20

update more pragmas to 0.8.20

91e4127

update changeset

29bf290

update SOLC_VERSION

2e7f5ed

Update hardhat.config.js default solc comment

e7ed4c2

Reference latest docs in Arrays.sol

19cd6e6

ernestognw previously approved these changes Jul 31, 2023

View reviewed changes

Amxx commented Jul 31, 2023

View reviewed changes

Update Arrays.sol

81aaffa

Amxx dismissed ernestognw’s stale review via 81aaffa July 31, 2023 19:50

frangio approved these changes Jul 31, 2023

View reviewed changes

frangio merged commit 00cbf5a into OpenZeppelin:master Jul 31, 2023

Amxx deleted the pragma-0-8-20 branch August 1, 2023 12:31

Amxx mentioned this pull request Aug 2, 2023

Set EVM to Paris for the release process #4503

Open

This was referenced Aug 5, 2023

default evm version to paris [APE-1268] ApeWorX/ape-vyper#91

Open

default evm version to paris [APE-1269] ApeWorX/ape-solidity#118

Open

d10r mentioned this pull request Aug 7, 2023

[SPIKE] EVM bytecode compatibility across SF networks superfluid-finance/protocol-monorepo#1533

Closed

codehawks-bot mentioned this pull request Aug 29, 2023

Importing contacts from Openzeppelin with v0.8.20 can cause problems while deploying on Polygon and other L2 Cyfrin/2023-08-sparkn#470

Closed

Amxx mentioned this pull request Oct 26, 2023

Fixed Misleading Typo in CHANGELOG.md related to false solidity version #4697

Merged

3 tasks

nlordell mentioned this pull request Jun 21, 2024

Fixed and More Consistent Pragmas hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac#24

Closed

nlordell mentioned this pull request Jul 4, 2024

Fixed and More Consistent Pragmas safe-global/safe-modules#458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pragma to 0.8.20 #4489

Update pragma to 0.8.20 #4489

Amxx commented Jul 31, 2023 •

edited by frangio

Loading

changeset-bot bot commented Jul 31, 2023 •

edited

Loading

ernestognw left a comment

Amxx Jul 31, 2023

frangio Jul 31, 2023

ernestognw Jul 31, 2023

frangio Jul 31, 2023

ernestognw Jul 31, 2023 •

edited

Loading

pcaversaccio commented Aug 2, 2023 •

edited

Loading

Amxx commented Aug 2, 2023

pcaversaccio commented Aug 2, 2023

Amxx commented Aug 2, 2023

pcaversaccio commented Aug 2, 2023

JorgeAtPaladin commented Aug 2, 2023

Amxx commented Aug 2, 2023 •

edited

Loading

pcaversaccio commented Aug 2, 2023

frangio commented Aug 2, 2023

Amxx commented Aug 2, 2023 •

edited

Loading

pcaversaccio commented Aug 2, 2023 •

edited

Loading

Amxx commented Aug 2, 2023

pcaversaccio commented Aug 2, 2023

frangio commented Aug 2, 2023 •

edited

Loading

pcaversaccio commented Aug 2, 2023

Amxx commented Aug 3, 2023 •

edited

Loading

pcaversaccio commented Aug 3, 2023

Update pragma to 0.8.20 #4489

Update pragma to 0.8.20 #4489

Conversation

Amxx commented Jul 31, 2023 • edited by frangio Loading

changeset-bot bot commented Jul 31, 2023 • edited Loading

⚠️ No Changeset found

ernestognw left a comment

Choose a reason for hiding this comment

Amxx Jul 31, 2023

Choose a reason for hiding this comment

frangio Jul 31, 2023

Choose a reason for hiding this comment

ernestognw Jul 31, 2023

Choose a reason for hiding this comment

frangio Jul 31, 2023

Choose a reason for hiding this comment

ernestognw Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

pcaversaccio commented Aug 2, 2023 • edited Loading

Amxx commented Aug 2, 2023

pcaversaccio commented Aug 2, 2023

Amxx commented Aug 2, 2023

pcaversaccio commented Aug 2, 2023

JorgeAtPaladin commented Aug 2, 2023

Amxx commented Aug 2, 2023 • edited Loading

pcaversaccio commented Aug 2, 2023

frangio commented Aug 2, 2023

Amxx commented Aug 2, 2023 • edited Loading

pcaversaccio commented Aug 2, 2023 • edited Loading

Amxx commented Aug 2, 2023

pcaversaccio commented Aug 2, 2023

frangio commented Aug 2, 2023 • edited Loading

pcaversaccio commented Aug 2, 2023

Amxx commented Aug 3, 2023 • edited Loading

pcaversaccio commented Aug 3, 2023

Amxx commented Jul 31, 2023 •

edited by frangio

Loading

changeset-bot bot commented Jul 31, 2023 •

edited

Loading

ernestognw Jul 31, 2023 •

edited

Loading

pcaversaccio commented Aug 2, 2023 •

edited

Loading

Amxx commented Aug 2, 2023 •

edited

Loading

Amxx commented Aug 2, 2023 •

edited

Loading

pcaversaccio commented Aug 2, 2023 •

edited

Loading

frangio commented Aug 2, 2023 •

edited

Loading

Amxx commented Aug 3, 2023 •

edited

Loading