-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(forge): introduce network custom features, sunset Odyssey #11675
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
Conversation
29f235c
to
46cb1d9
Compare
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.
very nice, just some comments/suggestions but we should proceed with this
crates/evm/core/src/evm.rs
Outdated
NetworkPrecompiles::default() | ||
.odyssey(evm.inspector().is_odyssey()) | ||
.celo(evm.inspector().is_celo()) | ||
.inject(evm.precompiles_mut()); |
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.
very nice
precompiles_map | ||
.extend(NetworkPrecompiles::default().odyssey(self.odyssey).celo(self.is_celo()).get()); |
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.
so much better
crates/evm/precompiles/src/lib.rs
Outdated
/// Returns precompiles for configured networks. | ||
pub fn get(self) -> BTreeMap<String, Address> { | ||
let mut precompiles = BTreeMap::new(); | ||
if self.odyssey { | ||
precompiles.insert( | ||
PrecompileId::P256Verify.name().to_string(), | ||
u64_to_address(P256VERIFY_ADDRESS), | ||
); | ||
} | ||
|
||
if self.celo { | ||
precompiles | ||
.insert(PRECOMPILE_ID_CELO_TRANSFER.name().to_string(), CELO_TRANSFER_ADDRESS); | ||
} | ||
precompiles | ||
} |
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.
can we rename this to get_ids or something?
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.
yeah, I actually repurposed the crate foundry-evm-precompiles
-> foundry-evm-networks
because networks could come with more than custom precompiles, so we have now inject_precompiles
and precompiles
and we could add fns for custom txes or other stuff
crates/forge/src/multi_runner.rs
Outdated
/// Whether to enable Odyssey features. | ||
pub odyssey: bool, | ||
/// Whether to enable Celo precompiles. | ||
pub celo: bool, |
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.
does it make sense to group this to
networks: NetworkPrecompiles
or similar?
less defaults, fields
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.
done, looks much better
896ceb5
to
f7a064b
Compare
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.
lgtm!
Some(celo_precompile::precompile()) | ||
}); | ||
} | ||
self.networks.inject_precompiles(evm.precompiles_mut()); |
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.
nicely consolidates precompile injection
/// Cumulative blob gas used by all executed transactions | ||
pub blob_gas_used: u64, | ||
pub enable_steps_tracing: bool, | ||
pub odyssey: bool, |
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.
@grandizzy FYI, there will be conflicts with #11688
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.
yep, going to include odyssey removal and superseded the other PR
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.
In that case, relaying from #11688 (comment)
Can we also remove the WalletGetCapabilities and AnvilAddCapability endpoints? Now that we have the relay open source, along with the docker container that runs Anvil and relay together for local testing?
We don't need these in anvil anymore
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.
not sure, does this mean that users will have to run relay for those? @onbjerg could you pls chime in?
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.
They can run anvil + relay using the provided docker container: https://porto.sh/relay#local-development
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.
kk, so not odyssey related endpoint?
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.
will follow up in subsequent PR
6daa409
to
d8e0a79
Compare
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.
Nice!
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.
lgtm
do we want a separate config key for this? like networks.x
should we infer this from chain id if set?
pub struct NetworkConfigs { | ||
/// Enable Optimism network features. | ||
#[arg(help_heading = "Networks", long, visible_alias = "optimism")] | ||
#[serde(skip)] |
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.
why is this serde skipped?
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.
this ends up in forge config and we don't have (yet) anything to add to forge if optimism set to true, hence decided not to include
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.
please leave a comment
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.
yep, added in cf1499e
going to open up a gh issues with follow ups to improve the impl by:
|
Motivation
closes Incompatible with the celo network,
balanceOf()
,transfer()
,deal()
and others do not work #11622moves networks specific features (precompiles for now) in networks crate, reused in forge - enables Celo transfer precompile when running
forge test
with--celo
follow-up: apply to cast as well, so network features (precompiles, custom tx types) can be applied on all foundry components
superseeds refactor!: remove
--odyssey
#11688Removes the
--odyssey
flag since Odyssey is being sunset, and we have equivalent flags that achieve the same behavior.If you just need EIP7701, you can use
--evm-version prague
.If you additionally need the P256 precompiles, you can use
--evm-version osaka
.Test
osaka_can_run_p256_precompile
updated as some additional gas spent compared with odyssey.Closes Sunset
--odyssey
flag #11681Solution
PR Checklist