fix(oppool): use efficient pack length #371
Conversation
alex-miao
requested review from
dancoombs,
dphilipson and
0xfourzerofour
as code owners
alex-miao
force-pushed
the
alex/pack-len
branch
from
058569b to
771977b
Compare
| @@ -164,7 +164,7 @@ impl PoolOperation { | |||
| } | |||
|
|
|||
| pub fn size(&self) -> usize { | |||
| self.uo.pack().len() | |||
| USER_OPERATION_PACKED_LEN | |||
There was a problem hiding this comment.
Was kind of confused, this is just a constant no matter what the size of the user op is right? Was it not at some point?
There was a problem hiding this comment.
thats a good question, let me look into the packed size of a specific uo and make sure that constant is still correct
There was a problem hiding this comment.
the original discussion referenced in #81 seemed to imply that this value is dynamic, need to make sure we are not confusing packed vs actual size of the UO throughout the codebase
| @@ -164,7 +164,7 @@ impl PoolOperation { | |||
| } | |||
|
|
|||
| pub fn size(&self) -> usize { | |||
| self.uo.pack().len() | |||
| self.uo.mem_size() | |||
There was a problem hiding this comment.
This size function ends up only being used for managing the pool size, so using the memory size here. Should we also be including the memory overhead of the other fields of PoolOperation?
There was a problem hiding this comment.
Yeah, good thought. Makes sense to move this mem_size() function one level up to PoolOperation.
crates/types/src/user_operation.rs
Outdated
| @@ -61,7 +64,43 @@ impl UserOperation { | |||
| } | |||
|
|
|||
| /// Packs the user operation into a byte array, consistent with the entrypoint contract's expectation | |||
| pub fn pack(&self) -> Bytes { | |||
| pub fn abi_encode(&self) -> Bytes { | |||
There was a problem hiding this comment.
Does ethers not already offer a function for this?
There was a problem hiding this comment.
good point, now just using the .encode() function from the AbiEncode trait
crates/types/src/user_operation.rs
Outdated
| } | ||
|
|
||
| /// Gets the byte array representation of the user operation to be used in the signature | ||
| pub fn pack_signature_message(&self) -> Bytes { |
There was a problem hiding this comment.
I think a better name here would be pack_for_hash, since the signature gets calculated from the hash.
alex-miao
force-pushed
the
alex/pack-len
branch
2 times, most recently
from
cf3d11a to
198a16a
Compare
crates/pool/src/mempool/mod.rs
Outdated
| pub fn mem_size(&self) -> usize { | ||
| std::mem::size_of::<Self>() | ||
| + self.uo.mem_size() | ||
| + std::mem::size_of::<Vec<EntityType>>() |
There was a problem hiding this comment.
Assuming that this is for entities_needing_stake is this not accounted for in std::mem::size_of::<Self>()?
There was a problem hiding this comment.
yep! actually made a mistake about how size_of works, it actually is able to recursively get the size of everything that gets allocated to the stack, so calling self.uo.mem_size() here would be double counting.
alex-miao
force-pushed
the
alex/pack-len
branch
from
198a16a to
55e780a
Compare
Closes #81
Proposed Changes