Extract common utils, serialization to shared and serde-ext #4142
Conversation
The module exports and provides only Web3 types wrapping ethrpc crate, thus the new name is more fitting and avoids the need to rename imports if importing ethrpc and shared::ethrpc together.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a large refactoring that extracts common utilities and serialization logic into new shared crates, shared and serde-ext. The changes are mostly moving code, renaming modules from ethrpc to web3, and replacing util::serialize with serde_ext. The refactoring is well-executed and improves code organization. The suggestion for the new serde-ext crate to further improve it by removing some redundancy remains valid.
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
LGTM but I'm confused about the ethrpc -> web3 rename, was it necessary?
I have renamed it since the file provides wrappers for creating Web3 instances from the I have figured that it makes sense to rename the module to What do You think? |
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
Before merging please evaluate how much effort it would be to replace our handrolled bytes type with the bytes crate widely used in the ecosystem.
| target: encoded.0, | ||
| value: encoded.1.into(), | ||
| call_data: crate::util::Bytes(encoded.2.0.to_vec()), | ||
| call_data: shared::bytes::Bytes(encoded.2.0.to_vec()), |
There was a problem hiding this comment.
Rather than continue to use our handrolled Bytes type did you consider using the bytes crate instead? It's already used in alloy and tokio and is optimized for cheap cloning since it's already internally reference counted.
There was a problem hiding this comment.
I'll check it out and report back.
There was a problem hiding this comment.
Probably makes sense to move this into serde-ext as an intermediate step but in the long term we should probably have a shared crate with the primitive types we currently have duplicated in the domain of multiple services (e.g. TokenAddress, TokenAmount, etc.).
This types should then no longer need the serializeAs logic of serde-ext since their regular Serialize implementation could just do the right thing.
There was a problem hiding this comment.
That's a good idea, I'll save it for a follow-up PR.
3 participants
Description
The simulation refactoring PR (#4140) is large due to some common types being moved out to
shared. This PR covers these aspects and moves commondriverutilities toshared. Alongside these common (de)serialization logic is moved to new crateserde-extChanges
sharedautopilotethrpccrateserde-extand use it throughoutdriverandsolvers-dtoHow to test