-
Notifications
You must be signed in to change notification settings - Fork 151
Migrate Balancer V2 from ethcontract to alloy primitives #4002
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
Migrate Balancer V2 from ethcontract to alloy primitives #4002
Conversation
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
d3722e9 to
b164ff1
Compare
|
/gemini review |
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.
Code Review
This pull request is a significant refactoring to migrate from ethcontract primitives to alloy primitives. The changes are extensive but mostly mechanical, and you've done a great job handling the API differences between the two libraries. My review focuses on a few areas where the new alloy types have different behavior, particularly in test helper functions and debug implementations, which could be improved for correctness and clarity. I've also suggested a small simplification for parsing a large constant. Overall, this is a solid piece of work.
crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs
Outdated
Show resolved
Hide resolved
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.
Code Review
This pull request migrates Balancer V2 code from ethcontract primitives to alloy primitives. The changes are extensive and mostly mechanical, replacing types like U256 and removing conversion traits. The code is generally cleaner and more consistent after these changes. I've found a few places where the removal of conversion adapters seems incomplete, leaving verbose and explicit calls to traits that are meant to be removed. My review comments focus on these inconsistencies to fully align the code with the PR's goal.
jmg-duarte
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.
There are some things that need to be addressed before merging, I'd prefer keeping the code as close to the original as possible since (AFAIK) this is already a port from a smart contract of sorts
|
|
||
| Ok(PoolInfo { | ||
| id: pool_id, | ||
| id: H256(pool_id.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.
Should've changed to B256. but my PR changes this, so it's ok for now
crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs
Outdated
Show resolved
Hide resolved
crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs
Outdated
Show resolved
Hide resolved
Removes unnecessary type conversions between ethcontract and alloy types in Balancer V2 pool code. The conversions are no longer needed as the codebase migrates to native alloy types. Updates affected areas: - Remove IntoAlloy/IntoLegacy trait usage for U256, addresses, and pool data - Convert ethcontract::U256 to alloy::primitives::U256 in tests and implementations - Simplify type conversions in stable, weighted, composable stable, and liquidity bootstrapping pools Signed-off-by: Aryan Godara <[email protected]>
- Replace lossy unwrap_or fallbacks with direct unwrap() in Bfp Debug impl - Fix to_f64_lossy() to use string parsing for accurate large value conversion - Update test helper functions to use string-based U256/f64 conversions instead of lossy casts - Simplify into_legacy() calls by using method syntax instead of fully qualified paths
70985b3 to
2d3185d
Compare
jmg-duarte
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.
I have nits (some qualified paths and U256::from(1)) but I'll fix them in my PR
Please address the suggestion I left and IMO it's good to go
| fn invariant_two_tokens_ok() { | ||
| let amp = 100.; | ||
| let amplification_parameter = U256::from_f64_lossy(amp * AMP_PRECISION.to_f64_lossy()); | ||
| let amplification_parameter = u256_from_f64_lossy(amp * f64::from(*AMP_PRECISION)); |
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.
and on the other usages
| let amplification_parameter = u256_from_f64_lossy(amp * f64::from(*AMP_PRECISION)); | |
| let amplification_parameter = U256::from(amp * f64::from(*AMP_PRECISION)); |
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.
Oh, I fixed f64:from , but missed the other one, fixed now, pushing
- Replace all calls with direct U256::from() conversions for amplification parameter
squadgazzz
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.
G2G as is
|
|
||
| assert_eq!( | ||
| AmplificationParameter::try_new(1.into(), 0.into()) | ||
| AmplificationParameter::try_new(U256::from(1u64), U256::from(0u64)) |
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.
| AmplificationParameter::try_new(U256::from(1u64), U256::from(0u64)) | |
| AmplificationParameter::try_new(U256::ONE, U256::ZERO) |
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.
Will fix this on my PR
Description
Migrate the balancer V2
Changes
ethcontract::primitiveswithalloy::primitivesacross crates:How to test
cargo check -p e2e --tests