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

feat(forge): Paris & Shanghai support & add prevrandao cheatcode #4856

Merged
merged 36 commits into from
May 5, 2023

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Apr 30, 2023

Motivation

Right now the default spec is still London and most tooling will now make Shanghai the default spec. We should update everything related to selecting specs post london, for Paris and Shanghai support, but keep Paris as default until solidity .8.20 is out and ethers switches over.

Solution

Adds Paris and Shanghai as supported specs (with Paris as default as Ethers falls back to it), along with updating tests to use the new Prevrandao cheatcode included in this PR. Closes #4855.

This impacts a few things (will look at this after today, getting late):

  • Cheatcode tests: difficulty is used on a few I believe, and difficulty doesn't work from Paris onwards. I assume we'll want to change/add tests to use prevrandao (which by extension prob means we need a prevrandao cheatcode).
  • Ethers will be bumped to 2.0.4.

The files changed tab looks daunting, but bear with me—most of those changes are just modifying compiler output so that tests pass.

@Evalir Evalir changed the title chore: upgrade default evm version to shanghai for optimizoors General: Upgrade default EVM spec Apr 30, 2023
@Evalir Evalir changed the title General: Upgrade default EVM spec [WIP] feat(forge): Upgrade default EVM spec Apr 30, 2023
@Evalir Evalir requested review from mattsse and onbjerg and removed request for mattsse and onbjerg April 30, 2023 01:24
@mds1
Copy link
Collaborator

mds1 commented Apr 30, 2023

One thing to consider is that many chains users deploy to won’t support Shanghai/PUSH0, so during scripting we may want to check the chain ID (from the RPC url) and warn or error if it’s a chain not known to support Shanghai. Otherwise users might build/deploy contracts that work during testing but not in production

@Evalir Evalir marked this pull request as ready for review April 30, 2023 06:01
@Evalir
Copy link
Member Author

Evalir commented Apr 30, 2023

@mds1 yup that's a great observation. what do you prefer? Right now the solidity compiler does not support PUSH0 yet so just warning could be fine for now—it would only concern hufflang folks rn. cc @mattsse

@Evalir Evalir marked this pull request as draft April 30, 2023 06:32
let provider = ethers::providers::Provider::<Http>::try_from(rpc)?;
let chain_id = provider.get_chainid().await?;
if chain_id != U256::one() {
shell::println(format!("Shanghai is only supported on Ethereum Mainnet (Chain ID 1). This RPC uses chain id {} therefore contracts using PUSH0 will not work.", chain_id))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change this to a more meaningful description. The average user does not think in terms of assembly. I would instead reference the EIP-3855 and provide a link

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also shanghai is also supported on goerli and sepolia, not just mainnet

Copy link
Member Author

@Evalir Evalir Apr 30, 2023

Choose a reason for hiding this comment

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

Yeah agreed this message could be better—and probably a bit louder as well. I'm still not 100% sold on only warning actually though: I've seen some discussion around the topic and we might wanna consider instead erroring (with some mechanism to bypass this). we'll have the same story in cancun with transient storage so that's why I'm giving it some more thought

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved the warning message here: c5b29b8

Copy link
Member Author

Choose a reason for hiding this comment

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

did some improvements, here's an example of how it looks:

Screen Shot 2023-05-02 at 13 49 03

It maybe should be even bigger—lmk what you think about copy and if we should make it even louder!

@mds1
Copy link
Collaborator

mds1 commented Apr 30, 2023

Right now the solidity compiler does not support PUSH0 yet so just warning could be fine for now

It will support it and make it the default soon, though: ethereum/solidity#14158

Since hardhat and solc are making shanghai the default (which makes sense, since ethereum mainnet is still the primary target of these tools), I think it's ok for foundry to do so also, but a loud warning before deployments feels like a nice mitigation

config/src/lib.rs Outdated Show resolved Hide resolved
forge/tests/it/config.rs Outdated Show resolved Hide resolved
@Evalir Evalir changed the title [WIP] feat(forge): Upgrade default EVM spec [WIP] feat(forge): Upgrade default EVM spec & add prevrandao cheatcode May 1, 2023
@Evalir Evalir marked this pull request as ready for review May 1, 2023 23:14
@Evalir Evalir changed the title [WIP] feat(forge): Upgrade default EVM spec & add prevrandao cheatcode feat(forge): Upgrade default EVM spec & add prevrandao cheatcode May 1, 2023
@Evalir Evalir marked this pull request as draft May 1, 2023 23:19
@Evalir Evalir requested a review from 0xMelkor May 3, 2023 12:22
@Evalir Evalir force-pushed the evalir/upgrade_evm_version branch from 6c20c36 to edef79b Compare May 3, 2023 12:45
cli/src/cmd/forge/script/mod.rs Outdated Show resolved Hide resolved
cli/src/cmd/forge/script/mod.rs Outdated Show resolved Hide resolved
cli/src/cmd/forge/script/mod.rs Outdated Show resolved Hide resolved
cli/src/cmd/forge/script/mod.rs Outdated Show resolved Hide resolved
Evalir and others added 2 commits May 3, 2023 10:16
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@Evalir Evalir requested a review from DaniPopes May 3, 2023 15:39
@Evalir Evalir force-pushed the evalir/upgrade_evm_version branch from 12599da to 06e493b Compare May 3, 2023 15:46
@Evalir Evalir force-pushed the evalir/upgrade_evm_version branch from 06e493b to 87aea94 Compare May 3, 2023 16:13
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

LGTM, 1 question which can be resolved in another PR.

Comment on lines +140 to +150
/// Converts an `EvmVersion` into a `SpecId`
pub fn evm_spec(evm: &EvmVersion) -> SpecId {
match evm {
EvmVersion::Istanbul => SpecId::ISTANBUL,
EvmVersion::Berlin => SpecId::BERLIN,
EvmVersion::London => SpecId::LONDON,
EvmVersion::Paris => SpecId::MERGE,
EvmVersion::Shanghai => SpecId::SHANGHAI,
_ => panic!("Unsupported EVM version"),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I know you added just Paris and Shanghai (current is just istanbul, berlin, london), but is there any reason we didn't have more versions mapped in the first place? This panics in some cases: #4873

EvmVersion docs, SpecId docs

Introduced in #918: cc @onbjerg @mattsse

Copy link
Member Author

Choose a reason for hiding this comment

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

I am actually not sure and is a pending question I have myself—I wanna try and document how we're using the hard forks and if there's any reason not to include them all (and if we want to, ideally in another PR).

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss in separate issue - i dont recall a particular reason

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice work!

looks good, one nit re error check and message

Comment on lines 735 to 740
if !chain_ids_supported {
let msg = "\
EIP-3855 is not supported in one or more of the RPCs used.
Contracts deployed with a Solidity version equal or higher than 0.8.20 might not work properly.
For more information, please see https://eips.ethereum.org/EIPS/eip-3855";
shell::println(Paint::yellow(msg))?;
Copy link
Member

Choose a reason for hiding this comment

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

it'd be good to include the chain id that does not support it her, otherwise the user needs to identify this manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

sweet, here's how it would look:
Screen Shot 2023-05-04 at 14 09 58

Copy link
Member Author

@Evalir Evalir May 4, 2023

Choose a reason for hiding this comment

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

Ah will quickly change the message so it points out these are chain IDs. EDIT: ad8803f

@Evalir Evalir requested a review from mattsse May 4, 2023 18:05
@Evalir Evalir mentioned this pull request May 4, 2023
2 tasks
Comment on lines +739 to +742
EIP-3855 is not supported in one or more of the RPCs used.
Unsupported Chain IDs: {}.
Contracts deployed with a Solidity version equal or higher than 0.8.20 might not work properly.
For more information, please see https://eips.ethereum.org/EIPS/eip-3855"#,
Copy link
Member

Choose a reason for hiding this comment

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

beautiful

Comment on lines +140 to +150
/// Converts an `EvmVersion` into a `SpecId`
pub fn evm_spec(evm: &EvmVersion) -> SpecId {
match evm {
EvmVersion::Istanbul => SpecId::ISTANBUL,
EvmVersion::Berlin => SpecId::BERLIN,
EvmVersion::London => SpecId::LONDON,
EvmVersion::Paris => SpecId::MERGE,
EvmVersion::Shanghai => SpecId::SHANGHAI,
_ => panic!("Unsupported EVM version"),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss in separate issue - i dont recall a particular reason

Comment on lines +13 to +22
bytes memory bytecode = hex"365f5f37365ff3";
// 36 CALLDATASIZE
// 5F PUSH0
// 5F PUSH0
// 37 CALLDATACOPY -> copies calldata at mem[0..calldatasize]

// 36 CALLDATASIZE
// 5F PUSH0
// F3 RETURN -> returns mem[0..calldatasize]

Copy link
Member

Choose a reason for hiding this comment

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

great!

@gakonst gakonst merged commit e48db34 into master May 5, 2023
@gakonst gakonst deleted the evalir/upgrade_evm_version branch May 5, 2023 19:57
grandizzy added a commit to ajna-finance/ajna-grants that referenced this pull request May 8, 2023
- update pragmas to 0.8.18
- use preverando instead difficulty (need a version of foundry containing foundry-rs/foundry#4856)
grandizzy added a commit to ajna-finance/ajna-grants that referenced this pull request May 8, 2023
- update pragmas to 0.8.18
- use preverando instead difficulty (need a version of foundry containing foundry-rs/foundry#4856)
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.

Enable Paris & Shanghai support after REVM Upgrade
8 participants