Add EIP-4626 native price estimator by kaze-cow · Pull Request #4243 · cowprotocol/services

kaze-cow · 2026-03-09T15:06:57Z

Description

Adds a native price estimator for EIP-4626 vault tokens (e.g. sDAI, wrapped yield vaults).

Many vault tokens lack direct DEX liquidity, causing native price estimation to fail. This estimator unwraps the vault by querying the on-chain asset() and convertToAssets() functions, then delegates pricing of the underlying token to the next estimator in the stage.

Changes

New Eip4626 native price estimator (crates/price-estimation/src/native/eip4626.rs):
calls asset() + decimals() in parallel, then convertToAssets(10^decimals) to compute the
shares-to-assets conversion rate, and multiplies by the inner estimator's price for the
underlying token.
Negative cache for non-vault tokens: tokens whose asset() call reverts are remembered in
a Mutex<HashSet<Address>> so subsequent requests skip the RPC entirely. Cleared on process
restart.
Timeout budget forwarding: each vault RPC call is individually bounded by
tokio::time::timeout, and whatever time remains is forwarded to the inner estimator. This
keeps the total wall-clock time within the caller's original timeout, which matters for
recursive vault chains.
Configurable recursion depth: the Eip4626 config variant accepts a depth parameter
(default: 1) controlling how many nested vault layers to unwrap. In the factory, depth layers
of Eip4626 wrap the next estimator in the stage.
Config validation: NativePriceEstimators deserialization rejects stages where Eip4626 is
the last entry (it must be followed by another estimator to price the underlying asset).
Contract bindings: added IERC4626 interface and MockERC4626Wrapper test contract for
e2e tests.
Factory wiring (crates/price-estimation/src/factory.rs): create_native_estimator now
consumes the next estimator from the stage iterator when it encounters Eip4626, wrapping it
in depth layers of instrumented Eip4626 estimators.
Re-added config deserialization tests to crates/configs/src/native_price_estimators.rs
that were lost during the extraction from price-estimation to configs.

How to test

Unit tests: cargo nextest run -p price-estimation eip4626 and cargo nextest run -p configs native_price_estimators
Live mainnet smoke test (requires NODE_URL): NODE_URL=... cargo nextest run -p price-estimation -- eip4626 --run-ignored ignored-only
E2e forked tests (requires FORK_URL_MAINNET):
- Single vault: cargo nextest run -p e2e forked_node_mainnet_eip4626_native_price --test-threads 1 --run-ignored ignored-only
- Recursive vaults: cargo nextest run -p e2e forked_node_mainnet_eip4626_recursive_native_price --test-threads 1 --run-ignored ignored-only

Prices vault tokens by calling `asset()` to find the underlying token, `convertToAssets(1e18)` to get the conversion rate, then delegating to an inner estimator for the underlying token's native price. Configurable as `Eip4626|<inner>`, e.g. `Eip4626|CoinGecko`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request adds a native price estimator for EIP-4626 vault tokens. However, a potential Denial of Service (DoS) vulnerability was identified in the estimator factory. The use of the unreachable! macro when handling nested EIP-4626 estimators can lead to an application panic if a deeply nested configuration is provided from external strings, which could be exploited. A suggestion has been provided to replace the panic with a proper error return. Additionally, a critical logic error was found in the price calculation when vault tokens and underlying assets have different decimal counts, leading to incorrect pricing. The implementation's robustness could be enhanced by parallelizing contract calls and respecting timeouts, and a non-functional unit test was identified, which provides a false sense of coverage.

crates/price-estimation/src/native/eip4626.rs

crates/price-estimation/src/factory.rs

jmg-duarte · 2026-03-09T15:14:07Z

Uses inline alloy::sol! { #[sol(rpc)] } for the minimal IERC4626 interface (no generated artifact needed)

The generated artifacts are useful to avoid paying the compilation time and so we can Cmd+Click into their implementation

github-actions · 2026-03-17T00:14:35Z

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.

kaze-cow · 2026-03-17T04:58:18Z

The generated artifacts are useful to avoid paying the compilation time and so we can Cmd+Click into their implementation

What exactly is "their implementation"? The definition right there is technically the definition of the interface... inline. The implnementation of an EIP4626 contract does not exist in this repository and I suspect E2E tests against mainnet fork would be best overall here. Thoughts?

- Change `Eip4626(Box<NativePriceEstimator>)` to `Eip4626` unit variant, eliminating recursive type that caused serde to hit the monomorphization recursion limit with `#[serde(tag = "type")]`. - Eip4626 now wraps the next estimator in the config stage list at construction time instead of nesting inside the enum. - Move Eip4626 handling into `create_native_estimator` by passing the stage iterator, removing the special-case in the caller. - Add deserialization validation rejecting Eip4626 as last in a stage. - Use vendored IERC4626 contract binding and query vault decimals for accurate conversion rate. - Fund sDAI whale with ETH in e2e test to fix gas error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

github-actions · 2026-03-27T00:16:12Z

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.

MartinquaXD · 2026-04-07T14:45:13Z

@kaze-cow did you already verify that the token quote coverage will be an issue for those EIP 4626 tokens or is this currently an assumption that this will be needed? The change seems reasonable overall but would still be good to understand what prio this should get.

kaze-cow · 2026-04-08T06:21:14Z

did you already verify that the token quote coverage will be an issue for those EIP 4626 tokens or is this currently an assumption that this will be needed?

Just to give an idea right now on staging we need to add a manual override for the token in order for it to be supported, despite having 3 solvers quoting/solving euler orders. So while I am not well aware of the details, this type of change does appear to be needed, and in any case, is likely to come in handy in improving our native token pricing coverage.

Introduces a new native price estimator that prices EIP-4626 vault tokens by querying the vault's underlying asset and conversion rate, then delegating to an inner estimator for the underlying token's price. Includes IERC4626 contract bindings and resolves merge conflicts with the contracts crate refactor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Remove duplicate type definitions from price-estimation/src/lib.rs that conflicted with the canonical definitions in the configs crate. Add the Eip4626 validation (must not be last in a stage) to the configs crate deserializer. Fix e2e test imports to use configs crate paths and correct start_protocol_with_args signature. Change recursive vault test to query native price directly instead of submitting a quote, since the freshly deployed mock wrapper has no DEX liquidity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…e-price # Conflicts: # crates/configs/src/native_price_estimators.rs # crates/e2e/tests/e2e/eip4626.rs

- Add negative cache (Mutex<HashSet>) for non-vault tokens to avoid wasted RPC calls on every estimation cycle - Enforce timeout on vault RPC calls via tokio::time::timeout so a stuck node cannot block the pipeline indefinitely - Forward remaining time budget to the inner estimator for correct deadline propagation through recursive chains - Change Eip4626 config from unit variant to Eip4626 { depth: NonZeroU8 } so recursive depth is declared once instead of repeating the variant - Extract conversion_rate() for readability Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…ding - Add `MintableToken::at` constructor to wrap ABI-compatible contracts (e.g. MockERC4626Wrapper) as MintableToken for use with `seed_uni_v2_pool` - Split native price estimator config into two stages with results_required=1 so the EIP-4626 chain has priority over the standalone driver fallback - Seed Uniswap V2 pools for recursive wrapper tokens so the solver can find routes, enabling full quote submission - Verify native price ratios between wrappers match their vault conversion rates Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

kaze-cow

looking good, thanks for taking this on!

crates/e2e/tests/e2e/eip4626.rs

kaze-cow · 2026-04-13T14:42:27Z

crates/e2e/tests/e2e/eip4626.rs

+    let mut prices = Vec::with_capacity(rates.len());
+    for (wrapper, &(num, den)) in wrappers.iter().zip(rates) {
+        let addr = *wrapper.address();
+        wait_for_condition(TIMEOUT, || async {
+            services.get_native_price(&addr).await.is_ok()
+        })
+        .await
+        .unwrap_or_else(|_| panic!("native price for wrapper ({num}/{den}) should be available"));
+
+        prices.push(services.get_native_price(&addr).await.unwrap().price);
+    }
+
+    for (i, &(num_i, den_i)) in rates.iter().enumerate() {
+        for (j, &(num_j, den_j)) in rates.iter().enumerate().skip(i + 1) {
+            let price_ratio = prices[i] / prices[j];
+            let expected_ratio = (num_i * den_j) as f64 / (num_j * den_i) as f64;
+            let relative_err = (price_ratio - expected_ratio).abs() / expected_ratio;
+            assert!(
+                relative_err < 0.01,
+                "price ratio between ({num_i}/{den_i}) and ({num_j}/{den_j}) should match rate \
+                 ratio: got {price_ratio:.6}, expected {expected_ratio:.6}",
+            );
+        }
+    }


i suppose the idea with all this code is to test all the combinations of prices to make sure their ratio is correct, but I think it would be just as effective and easier for a human to check if a few cases were spot checked with hardcoded values?

i've simplified it drastically

crates/e2e/tests/e2e/eip4626.rs

crates/price-estimation/src/native/eip4626.rs

kaze-cow · 2026-04-13T14:55:01Z

crates/price-estimation/src/native/eip4626.rs

+            ))
+        })?;
+
+        let shares = U256::from(10u64).pow(U256::from(decimals));


might be good to make this 10u64 configurable somehow. While I expect that 10e64 should be fine in most cases, maybe its a lot when you are talking about high-value tokens (ex. BTC)

also in general it would probably be fine to query for a universally small amount with sufficiently high precision (ex. 1000000, even if its a 1e18 token, the conversion rate returned will still be useful for native pricing).

Also as an unrelated, this is an edge case, but do keep in mind certain vaults may have high difference between underlying.decimals() and vault.decmials(), so for example if its USDC with 6 decimals, and a vault that uses ethereum standard 18 decimals, it would take 12 places of precision to understand the conversion rate of convertToAssets()

might be good to make this 10u64 configurable somehow. While I expect that 10e64 should be fine in most cases, maybe its a lot when you are talking about high-value tokens (ex. BTC)

Not sure I follow this, the formula there is 10 ^ decimals; each already in U256 so there's no overflow problems

Also as an unrelated, this is an edge case, but do keep in mind certain vaults may have high difference between underlying.decimals() and vault.decmials(), so for example if its USDC with 6 decimals, and a vault that uses ethereum standard 18 decimals, it would take 12 places of precision to understand the conversion rate of convertToAssets()

Not accounting for the difference between decimals, you're right, but w.r.t. space, U256 should cover it easily. Am I missing something?

I've updated the PR with the new decimals calculations though

…rences

gemini-code-assist

Code Review

This pull request introduces a new native price estimator for EIP-4626 vault tokens, enabling the protocol to price vaults by querying their underlying assets and conversion rates. The implementation includes auto-generated contract bindings, a mock wrapper for testing, and comprehensive e2e tests. Feedback identifies high-severity issues in the Eip4626 estimator: the negative cache for non-vault tokens must be size-bounded to prevent memory exhaustion, and it should distinguish between transient network errors and permanent execution reverts to avoid incorrect caching. Furthermore, checked arithmetic is required when calculating token powers to prevent potential overflows.

gemini-code-assist · 2026-04-14T11:52:36Z

crates/price-estimation/src/native/eip4626.rs

+    provider: AlloyProvider,
+    /// Addresses that are known *not* to be EIP-4626 vaults (i.e. `asset()`
+    /// reverted). Checked before making any RPC calls.
+    non_vault_tokens: Mutex<HashSet<Address>>,


The non_vault_tokens cache is an unbounded HashSet. Since it stores addresses derived from user-provided requests, it is vulnerable to memory exhaustion attacks if an attacker queries a large number of random addresses. Per the general rules, caches storing user-provided data must be size-bounded.

References

Caches that store user-provided data must be size-bounded to prevent potential Denial of Service attacks via memory exhaustion.

gemini-code-assist · 2026-04-14T11:52:36Z

crates/price-estimation/src/native/eip4626.rs

+            Err(e) => {
+                {
+                    let mut cache = self.non_vault_tokens.lock().unwrap();
+                    cache.insert(token);
+                    metrics::non_vault_cache_size(cache.len());
+                }


The code currently caches a token as a 'non-vault' on any error from the asset() call. This includes transient errors such as transport failures, rate limits (429), or node timeouts. Caching these transient errors will cause the estimator to incorrectly skip valid vaults until the process restarts. You should inspect the error and only perform negative caching if the error indicates that the method is missing or execution reverted.

References

Do not cache timeouts or transient failures as this might lead to incorrect caching of failures and cause the system to skip valid items.

gemini-code-assist · 2026-04-14T11:52:36Z

crates/price-estimation/src/native/eip4626.rs

+            ))
+        })?;
+
+        let one_token = U256::from(10u64).pow(U256::from(vault_decimals));


Calculating 10^vault_decimals using U256::pow will panic if the result overflows U256 (which happens if vault_decimals > 77). Since vault_decimals is retrieved from an on-chain uint8 call, it can be up to 255. Use checked_pow to handle potential overflows gracefully.

Suggested change

let one_token = U256::from(10u64).pow(U256::from(vault_decimals));

let one_token = U256::from(10u64).checked_pow(U256::from(vault_decimals)).ok_or_else(|| {

PriceEstimationError::EstimatorInternal(anyhow::anyhow!(

"vault decimals {vault_decimals} for {token} cause U256 overflow"

))

})?;

MartinquaXD · 2026-04-14T13:26:40Z

crates/configs/src/native_price_estimators.rs

+    /// Prices EIP-4626 vault tokens by looking up the underlying `asset()` and
+    /// applying `convertToAssets()` as a conversion rate. Must be followed by
+    /// another estimator in the same stage to price the underlying asset.
+    /// `depth` controls how many nested vault layers to unwrap (default: 1).
+    Eip4626 {
+        #[serde(default = "default_eip4626_depth")]
+        depth: NonZeroU8,
+    },


Not sure about how this is getting configured. While the current implementation gives us the most control and can theoretically produce more accurate prices I'm not sure this granularity will actually be needed in practice.
Instead one could consider a simple enable-eip-4626 support flag and wrapping CompetitionPriceEstimator in a single instance of this wrapper.

MartinquaXD · 2026-04-14T13:32:50Z

crates/configs/src/native_price_estimators.rs

+    Eip4626 {
+        #[serde(default = "default_eip4626_depth")]
+        depth: NonZeroU8,
+    },


Configuration options for this new wrapper might need an exemption list.
For example the price of sDai is pretty different from Dai.

MartinquaXD · 2026-04-14T13:36:15Z

crates/price-estimation/src/native/eip4626.rs

+    provider: AlloyProvider,
+    /// Addresses that are known *not* to be EIP-4626 vaults (i.e. `asset()`
+    /// reverted). Checked before making any RPC calls.
+    non_vault_tokens: Mutex<HashSet<Address>>,


Let's use DashSet since the access patterns are simple enough (i.e. no fine grained control about mutually exclusivity is needed).

MartinquaXD · 2026-04-14T13:37:17Z

crates/price-estimation/src/native/eip4626.rs

+        // matters for recursive Eip4626 chains where each layer spends time
+        // on vault RPC calls.


Why would we ever have recursive 4626 wrappers?

MartinquaXD · 2026-04-14T13:41:58Z

crates/price-estimation/src/native/eip4626.rs

+        let vault = IERC4626::Instance::new(token, self.provider.clone());
+        let vault_erc20 = ERC20::Instance::new(token, self.provider.clone());


nit: You don't have to clone the provider here.

Suggested change

let vault = IERC4626::Instance::new(token, self.provider.clone());

let vault_erc20 = ERC20::Instance::new(token, self.provider.clone());

let vault = IERC4626::IERC4626::new(token, &self.provider);

let vault_erc20 = ERC20::ERC20::new(token, &self.provider);

MartinquaXD · 2026-04-14T13:42:58Z

crates/price-estimation/src/native/eip4626.rs

+        let asset_fut = vault.asset();
+        let decimals_fut = vault_erc20.decimals();
+        let (asset_result, decimals_result) = tokio::time::timeout(timeout, async {
+            tokio::join!(asset_fut.call(), decimals_fut.call())


This can use a try_join!, right?

MartinquaXD · 2026-04-14T13:43:56Z

crates/price-estimation/src/native/eip4626.rs

+        })?;
+
+        let one_token = U256::from(10u64).pow(U256::from(vault_decimals));
+        let asset_erc20 = ERC20::Instance::new(asset, self.provider.clone());


Suggested change

let asset_erc20 = ERC20::Instance::new(asset, self.provider.clone());

let asset_erc20 = ERC20::ERC20::new(asset, &self.provider);

MartinquaXD · 2026-04-14T13:44:41Z

crates/price-estimation/src/native/eip4626.rs

+        let asset_decimals_fut = asset_erc20.decimals();
+        let remaining = deadline.saturating_duration_since(Instant::now());
+        let (convert_result, asset_decimals_result) = tokio::time::timeout(remaining, async {
+            tokio::join!(convert_fut.call(), asset_decimals_fut.call())


MartinquaXD · 2026-04-14T13:46:53Z

crates/price-estimation/src/native/eip4626.rs

+
+        let asset_fut = vault.asset();
+        let decimals_fut = vault_erc20.decimals();
+        let (asset_result, decimals_result) = tokio::time::timeout(timeout, async {


Rather than handling timeouts 2 times in this function you can handle them once in the caller, no?

MartinquaXD · 2026-04-14T14:06:46Z

crates/price-estimation/src/factory.rs

+    async fn create_native_estimator<'b>(
        &mut self,
        source: &NativePriceEstimatorSource,
+        rest: &mut impl Iterator<Item = &'b NativePriceEstimatorSource>,


This is quite complicated due to the choice of how this is getting configured. Should be a lot simpler if we just have 1 global flag that determines whether the competition estimator gets wrapped or not.

kaze-cow requested a review from a team as a code owner March 9, 2026 15:06

kaze-cow self-assigned this Mar 9, 2026

kaze-cow marked this pull request as draft March 9, 2026 15:07

gemini-code-assist bot reviewed Mar 9, 2026

View reviewed changes

github-actions bot added the stale label Mar 17, 2026

github-actions bot removed the stale label Mar 18, 2026

github-actions bot added the stale label Mar 27, 2026

github-actions bot closed this Apr 3, 2026

jmg-duarte reopened this Apr 6, 2026

jmg-duarte removed the stale label Apr 6, 2026

jmg-duarte and others added 12 commits April 9, 2026 16:48

Merge branch 'main' into feat/eip4626-native-price

0da19ab

Merge branch 'main' into feat/eip4626-native-price

e5033a8

clean up post merge

2350314

Merge branch 'jmgd/feat/eip4626-native-price' into feat/eip4626-nativ…

c8fa43e

…e-price # Conflicts: # crates/configs/src/native_price_estimators.rs # crates/e2e/tests/e2e/eip4626.rs

Consider rate when testing recursive vaults

2144c80

Re-add lost tests

64916da

remove extra contract

b22aef1

remove redundant test

a8b6c5b

kaze-cow commented Apr 13, 2026

View reviewed changes

jmg-duarte added 2 commits April 14, 2026 11:52

Fix EIP-4626 conversion rate to account for asset/vault decimal diffe…

aeddf58

…rences

simplify test

9b74dce

jmg-duarte marked this pull request as ready for review April 14, 2026 11:46

gemini-code-assist bot reviewed Apr 14, 2026

View reviewed changes

MartinquaXD reviewed Apr 14, 2026

View reviewed changes

-        let one_token = U256::from(10u64).pow(U256::from(vault_decimals));
+        let one_token = U256::from(10u64).checked_pow(U256::from(vault_decimals)).ok_or_else(|| {
+            PriceEstimationError::EstimatorInternal(anyhow::anyhow!(
+                "vault decimals {vault_decimals} for {token} cause U256 overflow"
+            ))
+        })?;

		// matters for recursive Eip4626 chains where each layer spends time
		// on vault RPC calls.

		let vault = IERC4626::Instance::new(token, self.provider.clone());
		let vault_erc20 = ERC20::Instance::new(token, self.provider.clone());

	let asset_erc20 = ERC20::Instance::new(asset, self.provider.clone());
	let asset_erc20 = ERC20::ERC20::new(asset, &self.provider);

Conversation

kaze-cow commented Mar 9, 2026 • edited by jmg-duarte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmg-duarte commented Mar 9, 2026

Uh oh!

github-actions bot commented Mar 17, 2026

Uh oh!

kaze-cow commented Mar 17, 2026

Uh oh!

github-actions bot commented Mar 27, 2026

Uh oh!

MartinquaXD commented Apr 7, 2026

Uh oh!

kaze-cow commented Apr 8, 2026

Uh oh!

kaze-cow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmg-duarte Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 14, 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!

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

kaze-cow commented Mar 9, 2026 •

edited by jmg-duarte

Loading

jmg-duarte Apr 14, 2026 •

edited

Loading