Remove ethcontract+web3+primitive-types#4106
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 completes the migration from ethcontract, web3, and primitive-types to alloy. The changes are extensive, removing the legacy libraries and refactoring the codebase to use alloy APIs. One critical issue was found where a f64 to U256 conversion could lead to a panic due to not handling infinite values, a case that was previously covered by the removed primitive-types library. The suggested fix addresses the non-finite values, but a potential overflow risk for large finite values remains.
jmg-duarte
force-pushed
the
jmgd/alloy/removal
branch
from
e6aa761 to
27dbf32
Compare
jmg-duarte
force-pushed
the
jmgd/alloy/removal
branch
from
27dbf32 to
5771022
Compare
There was a problem hiding this comment.
Code Review
This pull request completes the Alloy migration by removing the legacy ethcontract, web3, and primitive-types dependencies. The changes are extensive and primarily involve removing old code and adapting to the Alloy library. A key change is moving the observability labeling to the Web3 wrapper to preserve wallet state. My review identified a critical issue in the implementation of the new labeling logic that could lead to incorrect nonce management and transaction failures.
| @@ -1,7 +1,3 @@ | |||
| //! This script is used to vendor Truffle JSON artifacts to be used for code | |||
There was a problem hiding this comment.
Can happen in a different PR but I think it's okay to remove the vendor thingy completely. Seems cleaner to just copy the artifacts that you need.
| (0 | 1, 0) => { | ||
| let (alloy, wallet) = alloy::unbuffered_provider(url.as_str(), label); | ||
| (alloy, wallet) | ||
| } | ||
| None => { | ||
| let legacy = Web3Transport::new(http); | ||
| let (alloy, wallet) = alloy::unbuffered_provider(url.as_str()); | ||
| (legacy, alloy, wallet) | ||
| _ => { | ||
| let (alloy, wallet) = alloy::provider(url.as_str(), args, label); | ||
| (alloy, wallet) | ||
| } |
There was a problem hiding this comment.
| (0 | 1, 0) => { | |
| let (alloy, wallet) = alloy::unbuffered_provider(url.as_str(), label); | |
| (alloy, wallet) | |
| } | |
| None => { | |
| let legacy = Web3Transport::new(http); | |
| let (alloy, wallet) = alloy::unbuffered_provider(url.as_str()); | |
| (legacy, alloy, wallet) | |
| _ => { | |
| let (alloy, wallet) = alloy::provider(url.as_str(), args, label); | |
| (alloy, wallet) | |
| } | |
| (0 | 1, 0) => alloy::unbuffered_provider(url.as_str(), label), | |
| _ => alloy::provider(url.as_str(), args, label), |
Alternatively those function might as well already return the fully built Web3 instance.
There was a problem hiding this comment.
Alternatively those function might as well already return the fully built Web3 instance.
They're used in more places and I tried to keep the diff simple, we can clean this up further later
| // Note on truncation: previously we used primitive_types::U256::from_f64_lossy which | ||
| // truncated the floating point, while alloy is slightly more faithful to the original | ||
| // value and rounds to closest integer: [0, 0.5) => 0, [0.5, 1] => 1 | ||
| v => U256::from(v.trunc()), |
There was a problem hiding this comment.
Unless this caused issues in tests I think we can gloss over this minor difference (considering the magnitude of values we work with).
There was a problem hiding this comment.
it did cause issues in tests
| @@ -167,8 +167,6 @@ impl Contracts { | |||
|
|
|||
| #[derive(Debug, Error)] | |||
| pub enum Error { | |||
There was a problem hiding this comment.
It seems like this enum is useless now.
There was a problem hiding this comment.
Its actually exported, it can be cleaned up and just simplified but lets leave this for a follow up too
| pub struct Web3<T: Transport = DynTransport> { | ||
| pub legacy: web3::Web3<T>, | ||
| pub struct Web3 { | ||
| pub alloy: AlloyProvider, |
There was a problem hiding this comment.
Maybe now we can give it a more meaningful name.
| pub alloy: AlloyProvider, | |
| pub provider: AlloyProvider, |
There was a problem hiding this comment.
This leads to ~78 changes, I'll rename in a follow up PR just to keep the already reviewed diff minimal
3 participants
Completes the Alloy migration by removing the last remaining legacy Ethereum libraries:
ethcontract,web3, andprimitive-types. These dependencies are no longer needed and can be fully removed, simplifying the dependency tree.Key change: The labelling layer for observability now operates at the
Web3wrapper level instead of directly onDynProvider, ensuring the wallet state is properly preserved when creating labeled provider instances.Changes
ethcontract,web3, andprimitive-typesfrom workspace dependenciesbuffered.rs,http.rs,instrumented.rs,alloy/conversions.rs)ProviderLabelingExtfromDynProvidertoWeb3wrapper, preserving wallet state across labeled instancesHow to test
Existing tests