feat(builder): only include paymaster gas if there is a postOp #457
Conversation
alex-miao
requested review from
dancoombs,
dphilipson and
0xfourzerofour
as code owners
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| op, | ||
| chain_id, | ||
| false, | ||
| Some(op_with_sim.simulation.requires_post_op), |
There was a problem hiding this comment.
Totally forgot that we run the simulation in the builder so this information is available to us after that. Won't be the case when we remove the simulation, but works for now!
| @@ -536,7 +539,7 @@ where | |||
| let mut gas_left = U256::from(self.settings.max_bundle_gas); | |||
| let mut ops_in_bundle = Vec::new(); | |||
| for op in ops { | |||
| let gas = gas::user_operation_gas_limit(&op.uo, self.settings.chain_id, false); | |||
| let gas = gas::user_operation_gas_limit(&op.uo, self.settings.chain_id, false, None); | |||
There was a problem hiding this comment.
Why is post-op None here?
There was a problem hiding this comment.
Seems like we need to account for that when we're limiting the gas in the bundle.
There was a problem hiding this comment.
This is annoying, however, since we don't have this information here since we haven't simulated yet. Ideally we limit gas in bundle before simulating...
There was a problem hiding this comment.
Yep agree we need to account for it when limiting gas in the bundle, so the way it gets accounted for is that the gas::user_operation_gas_limit function takes in an Option<bool> representing whether we have a postOp or not. The None state represents when we have not done simulation yet, and the gas::verification_gas_limit_multiplier function defaults to the worst case of 3. We pass in Some(true) and Some(false) only post-simulation when we actually know if we use postOp or not.
There was a problem hiding this comment.
Won't that mean that we're going to create bundles well below our limit if they all use postops?
There was a problem hiding this comment.
One approach we could do is to have an initial pre-sim bundle gas limiting phase like we have now. We can add UOs up until the gas limit, but for UOs that have a paymaster, we can compute an optimistic and worst-case gas usage (optimistic being that they all have no postOp, pessimistic being they all use postOp). We then sim these UOs, but also a set of UOs that have a total gas limit bounded by the difference of the pessimistic and optimistic case. We deem these UOs as "backup" in the case that we end up having more space than we expected.
There was a problem hiding this comment.
Hm, improving the gas limit we submit the bundle with doesn't really improve our ability to land more UOs on chain. We likely need to do more.
How about this?
- Limit bundle assuming that none of the UOs use post op
- Simulate
- Further limit after knowing if UOs use post op.
There was a problem hiding this comment.
Looks like we had a similar idea. It must be good then...
There was a problem hiding this comment.
Sounds good, since this represents a shift to limiting post sim, might also make sense here to take into account UOs that fail validation
alex-miao
force-pushed
the
alex/paymaster-postop-gas
branch
2 times, most recently
from
5de4c82 to
b2e2c3f
Compare
alex-miao
force-pushed
the
alex/paymaster-postop-gas
branch
from
c131f4b to
d2abe8f
Compare
#349
Proposed Changes