Refactor driver's simulator into separate crate #4140
Conversation
![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 refactors the simulation logic from the driver crate into a new, reusable simulator crate, and moves shared domain and utility types into the shared crate. The changes are extensive but mostly consist of moving code and updating imports. I've identified a logic error in the error handling within the new simulator library, where non-revertible blockchain errors were incorrectly classified as transaction reverts. The provided review comment addresses this issue.
| SimulatorError::Tenderly(tenderly::Error::Revert(_)) => Some(tx), | ||
| SimulatorError::Tenderly(provider::tenderly::Error::Http(_)) => None, | ||
| SimulatorError::Tenderly(provider::tenderly::Error::Revert(_)) => Some(tx), | ||
| SimulatorError::Blockchain(_) => Some(tx), |
There was a problem hiding this comment.
The current implementation incorrectly classifies all SimulatorError::Blockchain errors as revertible transaction errors. This includes errors like gas price estimation failures, which are not transaction reverts. This can lead to valid solutions being incorrectly discarded. The is_revert() method on blockchain::Error should be used to correctly classify the error.
| SimulatorError::Blockchain(_) => Some(tx), | |
| SimulatorError::Blockchain(err) => if err.is_revert() { Some(tx) } else { None }, |
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
IMO some of the extractions here should be one in a separate PR to reduce tech debt and reduce the size of this PR too
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.
|
Moved refactoring of common types to #4142. This PR will be rebased and thus easier to review. |
m-sz
force-pushed
the
simulator-refactor
branch
from
d67034c to
f5cb342
Compare
2 participants
Description
drivercontains simulation logic that would benefit other initiatives if it were reusable:The logic in the
driveris heavily intertwined with foundational domain and infra types, so I have made several attempts at cleanly refactoring the simulation logic to a separate crate. This is the smallest in the scope that provides with clear-cut separation of the simulator for it to be reviewable.This can be further improved in follow-up PRs by:
contracts,GasPriceEstimator,Erc20,infra::blockchain, token types acrossdriverandsimulatordriverConfig so there is no need to convert it's subset to simulator's ConfigThe above have been intentionally out of scope of this PR to keep it maneagable.
Changes
simulatorcratedomain::eth,util::{Bytes, serialize}, intosharedcrate for reusedriverto adapt forsimulator's typesHow to test
This is purely a refactor, it is expected to be consistent with previous behavior.