Migrate Balancer V2 from ethcontract to alloy primitives #4002

AryanGodara · 2025-12-22T04:42:11Z

Description

Migrate the balancer V2

Changes

Replace ethcontract::primitives with alloy::primitives across crates:
- driver/src/boundary/liquidity/balancerv2
- shares/sources/balancerv2
- solvers/src/liquidity/balancerv2
- solvers/src/boundary/liquidity
Remove the now redundant intoalloy and intolegacy adapters.

How to test

cargo check -p e2e --tests
Run e2e tests

github-actions · 2025-12-30T00:10:08Z

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.

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]>

AryanGodara · 2026-01-06T10:37:19Z

/gemini review

gemini-code-assist

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.

gemini-code-assist · 2026-01-06T10:39:37Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

    fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
-        write!(
-            formatter,
-            "{}.{:0>18}",
-            self.0 / *ONE_18,
-            (self.0 % *ONE_18).as_u128()
-        )
+        let remainder = self.0 % *ONE_18;
+        let low: u128 = remainder.try_into().unwrap_or(0);
+        write!(formatter, "{}.{:0>18}", self.0 / *ONE_18, low)
    }


The use of unwrap_or(0) in the Debug implementation can be misleading. Since remainder is the result of a modulo operation with 10^18, it will always be smaller than u128::MAX and should fit into a u128. Using unwrap() is more idiomatic and will panic if this assumption is ever violated, which is better for debugging than silently printing an incorrect value.

Suggested change

fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {

write!(

formatter,

"{}.{:0>18}",

self.0 / *ONE_18,

(self.0 % *ONE_18).as_u128()

)

let remainder = self.0 % *ONE_18;

let low: u128 = remainder.try_into().unwrap_or(0);

write!(formatter, "{}.{:0>18}", self.0 / *ONE_18, low)

}

fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {

let remainder = self.0 % *ONE_18;

let low: u128 = remainder.try_into().unwrap();

write!(formatter, "{}.{:0>18}", self.0 / *ONE_18, low)

}

gemini-code-assist · 2026-01-06T10:39:37Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

    pub fn to_f64_lossy(self) -> f64 {
-        self.as_uint256().to_f64_lossy() / 1e18
+        // Convert U256 to f64 by taking the high bits
+        let val: u128 = self.as_uint256().try_into().unwrap_or(u128::MAX);
+        (val as f64) / 1e18
    }


The comment "Convert U256 to f64 by taking the high bits" is misleading. The implementation clamps values larger than u128::MAX to u128::MAX, which is different from taking the high bits. This is also a behavior change from the previous ethcontract implementation, which could affect test accuracy for large numbers.

A more accurate conversion for tests can be achieved by converting to a string and then parsing to f64.

Suggested change

pub fn to_f64_lossy(self) -> f64 {

self.as_uint256().to_f64_lossy() / 1e18

// Convert U256 to f64 by taking the high bits

let val: u128 = self.as_uint256().try_into().unwrap_or(u128::MAX);

(val as f64) / 1e18

}

pub fn to_f64_lossy(self) -> f64 {

// NOTE: This conversion is lossy and may not be accurate for large values.

// It is only intended for use in tests.

let val: f64 = self

.as_uint256()

.to_string()

.parse()

.unwrap_or(f64::INFINITY);

val / 1e18

}

gemini-code-assist · 2026-01-06T10:39:37Z

crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs

        0 => {
-            I256::from_dec_str("38877084059945950922200000000000000000000000000000000000").unwrap()
+            // 38877084059945950922200000000000000000000000000000000000
+            I256::try_from(
+                U256::from_str_radix(
+                    "38877084059945950922200000000000000000000000000000000000",
+                    10,
+                )
+                .unwrap(),
+            )
+            .unwrap()
        }


The parsing of this large constant can be simplified. Instead of parsing to U256 and then converting to I256, you can parse it directly into an I256 using from_str_radix, which makes the code more concise.

0 => { I256::from_str_radix( "38877084059945950922200000000000000000000000000000000000", 10, ) .unwrap() }

Gemini is dumb, but you have from_dec_str

https://docs.rs/alloy-primitives/1.4.1/alloy_primitives/struct.Signed.html#tymethod.from_dec_str

gemini-code-assist · 2026-01-06T10:39:37Z

crates/shared/src/sources/balancer_v2/swap/stable_math.rs

+    fn u256_from_f64_lossy(val: f64) -> U256 {
+        U256::from(val as u64)
+    }
+
+    fn u256_to_f64_lossy(val: U256) -> f64 {
+        let v: u128 = val.try_into().unwrap_or(u128::MAX);
+        v as f64
+    }


The u256_from_f64_lossy and u256_to_f64_lossy test helper functions are very lossy and their behavior has changed significantly from the ethcontract versions. u256_from_f64_lossy truncates to u64, and u256_to_f64_lossy clamps to u128::MAX. This could impact test accuracy.

Consider using string-based conversions for better precision in tests.

fn u256_from_f64_lossy(val: f64) -> U256 { U256::from_str_radix(&val.round().to_string(), 10).unwrap_or(U256::MAX) } fn u256_to_f64_lossy(val: U256) -> f64 { val.to_string().parse().unwrap_or(f64::INFINITY) }

gemini-code-assist

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.

gemini-code-assist · 2026-01-06T10:42:21Z

crates/shared/src/sources/balancer_v2/pools/common.rs

        block: BlockId,
    ) -> BoxFuture<'static, Result<PoolState>> {
-        let block = block.into_alloy();
+        let block = ethrpc::alloy::conversions::IntoAlloy::into_alloy(block);


This explicit call to IntoAlloy::into_alloy seems to contradict the PR's goal of removing the intoalloy and intolegacy adapters. Was this an oversight?

To align with the PR's goal, it would be best to perform the conversion manually if a direct replacement function isn't available. You could introduce a local helper function for the ethcontract::BlockId to alloy::eips::BlockId conversion to keep the code clean and self-contained.

In crates/shared/src/sources/balancer_v2/pools/common.rs, the conversion is done once at the beginning (line 114), then the converted block variable is used throughout. However, in stable.rs, the conversion is done inline within the async block, which requires the full trait path.
But since it's only done once, I'm keeping this as is; as in future, we'll simply remove this legacy adapter.

So no changes in code

I don't understand why this needed to be changed

Not sure I understand from your comment why can't continue using it in the block.into_legacy() way.

gemini-code-assist · 2026-01-06T10:42:21Z

crates/shared/src/sources/balancer_v2/pools/stable.rs

            pool_contract
                .getAmplificationParameter()
-                .block(block.into_alloy())
+                .block(ethrpc::alloy::conversions::IntoAlloy::into_alloy(block))


This explicit call to IntoAlloy::into_alloy seems to contradict the PR's goal of removing the intoalloy and intolegacy adapters. Was this an oversight?

To align with the PR's goal, it would be best to perform the conversion manually if a direct replacement function isn't available. You could introduce a local helper function for the ethcontract::BlockId to alloy::eips::BlockId conversion to keep the code clean and self-contained.

Not changing here either; we'll just remove this adapter once we're done migrating entirely to alloy

Not sure why you needed to change here either

gemini-code-assist · 2026-01-06T10:42:21Z

crates/solvers/src/boundary/liquidity/stable.rs

    Some(Pool {
        common: CommonPoolState {
-            id: id.into_legacy(),
+            id: ethrpc::alloy::conversions::IntoLegacy::into_legacy(id),


This explicit call to IntoLegacy::into_legacy seems to contradict the PR's goal of removing the intoalloy and intolegacy adapters.

You can convert from alloy::primitives::B256 to ethcontract::H256 directly, which is cleaner and removes the dependency on the adapter trait.

Suggested change

id: ethrpc::alloy::conversions::IntoLegacy::into_legacy(id),

id: ethcontract::H256(id.0),

ethcontract is not a dependency in solvers crate, so it gives errors; I don't want to add a new dep, so keeping this as is; open to feedback on this 👀 (same for comment below)

gemini-code-assist · 2026-01-06T10:42:21Z

crates/solvers/src/boundary/liquidity/weighted_product.rs

    Some(Pool {
        common: CommonPoolState {
-            id: id.into_legacy(),
+            id: ethrpc::alloy::conversions::IntoLegacy::into_legacy(id),


This explicit call to IntoLegacy::into_legacy seems to contradict the PR's goal of removing the intoalloy and intolegacy adapters.

You can convert from alloy::primitives::B256 to ethcontract::H256 directly, which is cleaner and removes the dependency on the adapter trait.

Suggested change

id: ethrpc::alloy::conversions::IntoLegacy::into_legacy(id),

id: ethcontract::H256(id.0),

- 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

jmg-duarte

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

jmg-duarte · 2026-01-06T13:18:51Z

crates/shared/src/sources/balancer_v2/pools/common.rs


        Ok(PoolInfo {
-            id: pool_id,
+            id: H256(pool_id.0),


Should've changed to B256. but my PR changes this, so it's ok for now

jmg-duarte · 2026-01-06T13:19:25Z

crates/shared/src/sources/balancer_v2/pools/common.rs

        block: BlockId,
    ) -> BoxFuture<'static, Result<PoolState>> {
-        let block = block.into_alloy();
+        let block = ethrpc::alloy::conversions::IntoAlloy::into_alloy(block);


I don't understand why this needed to be changed

jmg-duarte · 2026-01-06T13:19:48Z

crates/shared/src/sources/balancer_v2/pools/common.rs

            pool_info,
            PoolInfo {
-                id: pool_id.into_legacy(),
+                id: H256(pool_id.0),


This is the legacy type, you could've kept the into_legacy

jmg-duarte · 2026-01-06T13:20:35Z

crates/shared/src/sources/balancer_v2/pools/stable.rs

            pool_contract
                .getAmplificationParameter()
-                .block(block.into_alloy())
+                .block(ethrpc::alloy::conversions::IntoAlloy::into_alloy(block))


Not sure why you needed to change here either

jmg-duarte · 2026-01-06T13:21:09Z

crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs

    LazyLock::new(|| U256::try_from(*ONE_18).unwrap());
 static MAX_NATURAL_EXPONENT: LazyLock<I256> =
-    LazyLock::new(|| ONE_18.checked_mul(I256::from(130_i128)).unwrap());
+    LazyLock::new(|| ONE_18.checked_mul(I256::try_from(130i64).unwrap()).unwrap());


Not a fan of the unwrap, we should use expect(REASON) instead

jmg-duarte · 2026-01-06T13:29:56Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs


-            Ok(Self(((a_inflated - 1) / other.0) + 1))
+            Ok(Self(
+                ((a_inflated - U256::from(1u64)) / other.0) + U256::from(1u64),


Suggested change

((a_inflated - U256::from(1u64)) / other.0) + U256::from(1u64),

((a_inflated - U256::ONE) / other.0) + U256::ONE,

jmg-duarte · 2026-01-06T13:30:04Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

-        let max_error = raw.mul_up(*MAX_POW_RELATIVE_ERROR)?.add(Bfp(1.into()))?;
+        let max_error = raw
+            .mul_up(*MAX_POW_RELATIVE_ERROR)?
+            .add(Bfp(U256::from(1u64)))?;


jmg-duarte · 2026-01-06T13:30:31Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

+    fn test_exp10(n: u8) -> U256 {
+        U256::from(10u64).pow(U256::from(n))
+    }


No need to add now, but in the U256Ext from number, we should definitely add the exp10

jmg-duarte · 2026-01-06T13:30:50Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

            assert_eq!(
                Bfp::one()
-                    .$fn_name(Bfp(U256::MAX / U256::exp10(18) + 1))
+                    .$fn_name(Bfp(U256::MAX / test_exp10(18) + U256::from(1u64)))


Suggested change

.$fn_name(Bfp(U256::MAX / test_exp10(18) + U256::from(1u64)))

.$fn_name(Bfp(U256::MAX / test_exp10(18) + ONE))

jmg-duarte · 2026-01-06T13:32:19Z

crates/shared/src/sources/balancer_v2/swap/stable_math.rs

    fn in_given_out_two_tokens() {
        let amp = 100.;
-        let amplification_parameter = U256::from_f64_lossy(amp * AMP_PRECISION.to_f64_lossy());
+        let amplification_parameter = u256_from_f64_lossy(amp * u256_to_f64_lossy(*AMP_PRECISION));


Use the f64::from here and in the other places

squadgazzz · 2026-01-06T13:31:03Z

crates/shared/src/sources/balancer_v2/pools/common.rs

        block: BlockId,
    ) -> BoxFuture<'static, Result<PoolState>> {
-        let block = block.into_alloy();
+        let block = ethrpc::alloy::conversions::IntoAlloy::into_alloy(block);


Not sure I understand from your comment why can't continue using it in the block.into_legacy() way.

squadgazzz · 2026-01-06T13:33:41Z

crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs

    match i {
        0 => {
-            I256::from_dec_str("38877084059945950922200000000000000000000000000000000000").unwrap()
+            // 38877084059945950922200000000000000000000000000000000000


Do we need this comment?

Suggested change

// 38877084059945950922200000000000000000000000000000000000

squadgazzz · 2026-01-06T13:44:54Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

+        // NOTE: This conversion is lossy and may not be accurate for large values.
+        // It is only intended for use in tests.


IMO, the function name is already self-explanatory, no need for this comment.

squadgazzz · 2026-01-06T13:50:38Z

crates/shared/src/sources/balancer_v2/swap/fixed_point.rs

+            .as_uint256()
+            .to_string()
+            .parse()
+            .unwrap_or(f64::INFINITY);


I am not sure that would be the expected output. Let's return None in that case.

Also, I am not sure about the performance of to_string/parse conversions. How about something like this?

pub fn u256_to_f64_exact(x: U256) -> Option<f64> { // f64 integers are exact up to 2^53 if x.bits() <= 53 { // safe: value fits in u64 when bits<=53 let v = x.as_limbs()[0]; Some(v as f64) } else { None } }

ruint (the lib alloy uses) already has this conversion, done the proper way https://github.com/recmo/uint/blob/5bd4cff6ae3960591d750cdd7356e24aa086b67a/src/from.rs#L796

AryanGodara self-assigned this Dec 22, 2025

github-actions bot added the stale label Dec 30, 2025

github-actions bot closed this Jan 6, 2026

AryanGodara reopened this Jan 6, 2026

AryanGodara force-pushed the aryan/migrate-balancer-v2-to-alloy-primitives branch from d3722e9 to b164ff1 Compare January 6, 2026 09:46

chore: remove unnecessary closure, fix clippy warning

270db68

AryanGodara marked this pull request as ready for review January 6, 2026 10:36

AryanGodara requested a review from a team as a code owner January 6, 2026 10:36

AryanGodara removed the stale label Jan 6, 2026

gemini-code-assist bot reviewed Jan 6, 2026

View reviewed changes

jmg-duarte mentioned this pull request Jan 6, 2026

[NEEDS #4002] Migrate balancer v2 sources to alloy #4023

Draft

2 tasks

jmg-duarte reviewed Jan 6, 2026

View reviewed changes

squadgazzz reviewed Jan 6, 2026

View reviewed changes

	id: ethrpc::alloy::conversions::IntoLegacy::into_legacy(id),
	id: ethcontract::H256(id.0),

	((a_inflated - U256::from(1u64)) / other.0) + U256::from(1u64),
	((a_inflated - U256::ONE) / other.0) + U256::ONE,

	.$fn_name(Bfp(U256::MAX / test_exp10(18) + U256::from(1u64)))
	.$fn_name(Bfp(U256::MAX / test_exp10(18) + ONE))

		// NOTE: This conversion is lossy and may not be accurate for large values.
		// It is only intended for use in tests.

Migrate Balancer V2 from ethcontract to alloy primitives #4002

Are you sure you want to change the base?

Migrate Balancer V2 from ethcontract to alloy primitives #4002

Conversation

AryanGodara commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Uh oh!

github-actions bot commented Dec 30, 2025

Uh oh!

AryanGodara commented Jan 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

jmg-duarte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AryanGodara commented Dec 22, 2025 •

edited

Loading