-
Notifications
You must be signed in to change notification settings - Fork 1.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
refactor: rewrite revert and precompile decoding #6222
Conversation
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.
Super cool. Some comments:
@@ -29,7 +29,7 @@ use proptest::{ | |||
test_runner::{TestCaseError, TestRunner}, | |||
}; | |||
use revm::{ | |||
primitives::{Address as rAddress, HashMap, U256 as rU256}, | |||
primitives::{Address as aAddress, HashMap, U256 as rU256}, |
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 think we can just use alloy_primitives::Address
now
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.
Anvil stuff, I don't want to get into that
_ => unreachable!(), | ||
}; | ||
|
||
// TODO: Other chain 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.
I'm a bit unsure if we want to implement precompiles for other chains—we had discussed this is a thing we probably don't want to do (albeit we could probably do Optimism's easily?) cc @mattsse
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.
node.decode_precompile(precompile_fn, &self.labels); | ||
} else if let RawOrDecodedCall::Raw(ref bytes) = node.trace.data { | ||
if bytes.len() >= 4 { | ||
// TODO: chain ID argument |
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 is to decode different precompiles per chain id?
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.
Yes that's what I was thinking, but maybe I'm thinking ahead too much. Maybe we should just try to decode whatever revm has set as 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.
Yeah I think might be best to stick to what revm
has—I think soft consensus is to not get into maintaining precompiles for other chains—they can become a pain. IIRC arbitrum, which is the biggest offender, has a repo that simulates their custom precompile that ppl can use
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.
Ref #748
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'm cool with the changes, the only contentious thing is decoding other precompiles—but as this is not implemented imho not a blocker
A thing to note is that now |
Motivation
Closes #6102
Fixes #5337
Solution