-
Notifications
You must be signed in to change notification settings - Fork 50
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(simulation): v0.7 call gas estimation #663
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
|
f112a84
to
05003dd
Compare
@@ -0,0 +1,1484 @@ | |||
/// Bytecode of the deployed v0.6 entry point, retrieved from on-chain |
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.
Shouldn't you be able to get this from our compiled contracts instead of including in a file?
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.
We don't actually compile EntryPoint
at the moment, only IEntryPoint
for its ABI, although that would be easy enough to change. But also doing it this way means we get the same bytecode as the deployed entry point, whereas if we compile it ourselves the bytecode won't match because we won't have the same compiler settings, which could lead to minor differences in gas estimation.
I don't think it matters too much either way. Happy to switch to our compiled version if you prefer. That would have the advantage of reducing the Git repo size.
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 have the same imperfect match issue with v0.7 right?
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, but in 0.7 we're using a state overriden EntryPointSimulations
instead of EntryPoint
and there is no canonical bytecode for that.
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 that's sort of what I was referring to, since EntryPointSimulations
contains the EntryPoint
code and we're compiling it. Would prefer we keep this the same way across both EPs.
Although I did just notice we already have this https://github.com/alchemyplatform/rundler/blob/main/crates/types/contracts/bytecode/entrypoint/0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789.txt
So at a minimum we should dedup and use that
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.
Great work again. Awesome to see that we can reuse all of this logic between the entrypoints with pretty simple abstractions.
31cf334
to
a61a95b
Compare
a6e5da9
to
6e93036
Compare
I don't understand what is happening in this PR. The PR is showing added files like Edit: Got it fixed, not sure what was wrong. |
6e93036
to
13e81ab
Compare
a61a95b
to
73c9654
Compare
13e81ab
to
67e1d55
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.
Looks good! great work on this @dphilipson
Factors out the call gas estimation logic, which performs a binary search using the `target` and `targetData` arguments to `simulateHandleOp` along with some state override trickery, into a new trait, `CallGasEstimator`, then provides implementations for v0.6 and v0.7.
Rather than hardcoding v0.6 entry point bytecode in two places, one with the init code and one with the deployed code, use abigen to generate Rust code for the v0.6 `EntryPoint.sol` and use the bytecode that was generated. This makes v0.6 `IEntryPoint.sol` unnecessary, to remove it and convert all occurrences of `v0_6::i_entry_point::IEntryPoint` to `v0_6::entry_point::EntryPoint`.
b4cbd03
to
e4b7f39
Compare
Factors out the call gas estimation logic, which performs a binary search using the
target
andtargetData
arguments tosimulateHandleOp
along with some state override trickery, into a new trait,CallGasEstimator
, then provides implementations for v0.6 and v0.7.