-
Notifications
You must be signed in to change notification settings - Fork 154
[TRIVIAL] Migrate from H160 to alloy Address types #3927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I converted it to a draft to avoid notifications noise until all the issues with this PR are solved. |
MartinquaXD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description claims all instances of H160 would be replaced but there are still 714 remaining. I'm fine with doing partial migrations but this should then be stated in the description. Do you want to continue with this as a partial migration or update the PR to replace the remaining instances of H160 as well?
My fault, I had Claude write the PR description. I'll update
Partial migration, the instances you highlighted are plentiful and I suspect they require changing a bunch of code alone |
crates/shared/src/sources/balancer_v2/pool_fetching/pool_storage.rs
Outdated
Show resolved
Hide resolved
crates/shared/src/sources/balancer_v2/pool_fetching/pool_storage.rs
Outdated
Show resolved
Hide resolved
| for (spender, token, allowance) in results { | ||
| let allowance = match allowance { | ||
| Ok(value) => U256::from(value.0.as_slice()), | ||
| Ok(allowance) => U256::from_be_slice(&allowance.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the future that produces this value uses Erc20Instance::allowance() instead of web3.eth().call() alloy would do the right conversion for us and we wouldn't have to guess the endinanness here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted this piece, but the issue is that it would not pass by the web3 mock. This is just a "I forgot what the original code was" moment, I'll fix the mock issue right after this PR
Simple despite being huge
Description
This PR continues the migration from the
ethcontractlibrary to thealloylibrary byethcontract::H160andprimitive_types::H160withalloy::primitives::Addressethcontract::U256andprimitives_types::U256withalloy::primitives::U256There are plenty left, the current commit has a focus on some structures are related code, however some of the structures only got a partial migration to avoid an even more complex PR
This migration simplifies the codebase by reducing the need for type conversions and aligns the entire codebase with alloy's native types.
Changes
H160withAddressethcontract::U256withalloy::primitives::U256.into_alloy()and.into_legacy()conversion callsHow to test
Existing tests