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.

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!

…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.

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

+        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?

if asset fails but decimals succeeds we have a cacheable token
if decimal fails (asset must fail too) we don't, so we can't as try_join mixes the two

MartinquaXD · 2026-04-16T08:29:49Z

+                    .context("Eip4626 must be followed by another estimator in the same stage")?;
+                let (mut name, mut current) =
+                    Box::pin(self.create_native_estimator(next, rest, weth)).await?;
+                for _ in 0..depth.get() {


Originally I completely misunderstood the depth, why it's needed and how it's used.
My current understanding:

EIP4626 tokens could be wrapped multiple times (4626AdapterToken -> 4626AdapterToken -> UnderlyingAsset)

the depth is supposed to control how many layers of indirections we can resolve

it does this by wrapping that number of Eip4626 price estimation wrappers in each other

The current implementation seems strange for 2 reasons:

what happens if a token has a higher recursion that we support? Ideally we don't have a configurable depth and simply unwrap as much as we have to. If this eats into our time estimation budget too much we need to adjust the caching logic (resolve the final underlying asset + conversion rate once and reuse for subsequent calls).

why wrap multiple Eip4626 price estimation wrappers in one another if we could have a single one that does all the unrolling internally? That way we don't need a customizable limit, have all the recursion logic in one place, and don't create a bunch of different caches for the same thing.

Refactor the estimate method to iteratively unwrap vault layers, accumulating the conversion rate, instead of handling only a single vault layer. Extract `unwrap_vault_layer` for clarity. Pre-seed WETH in the non-vault token set to avoid unnecessary RPC calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

MartinquaXD · 2026-04-17T11:34:45Z

+        Ok((asset, vault_decimals))
+    }
+
+    /// Queries `convertToAssets(10^vault_decimals)` and the asset's decimals.


This comment requires the reader to have some extra context.
convertToAssets(10^vault_decimals) returns how many underlying asset tokens are equivalent to 1 vault token, right? This could be made clearer in the comment.

MartinquaXD

LGTM

fafk · 2026-04-17T15:20:29Z

+        let mut current_token = token;
+        let mut cumulative_rate = 1.0;
+        while let Some((asset, rate)) = self.unwrap_vault_layer(current_token).await? {
+            cumulative_rate *= rate;


Are we happy with using floats here instead of BigDecimal? I am assuming yes and it doesn't have to be super precise?

I must defer to @kaze-cow, I honestly don't know how precise we need to be

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

Comment thread crates/price-estimation/src/native/eip4626.rs Outdated

Comment thread crates/price-estimation/src/native/eip4626.rs Outdated

Comment thread crates/price-estimation/src/native/eip4626.rs Outdated

MartinquaXD reviewed Apr 14, 2026

View reviewed changes

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

3cb19d4

MartinquaXD requested changes Apr 16, 2026

View reviewed changes

jmg-duarte and others added 4 commits April 16, 2026 13:50

simplify approach

6dbd90a

minimize diff

5258bf5

wrong config

4eb73f4

MartinquaXD reviewed Apr 17, 2026

View reviewed changes

jmg-duarte added 3 commits April 17, 2026 15:00

Address comments

e492b47

missing detail

54f64d7

fmt

9ec32bf

MartinquaXD approved these changes Apr 17, 2026

View reviewed changes

fafk reviewed Apr 17, 2026

View reviewed changes

fafk approved these changes Apr 17, 2026

View reviewed changes

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

MartinquaXD Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

jmg-duarte Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinquaXD Apr 16, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinquaXD Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

MartinquaXD left a comment

Choose a reason for hiding this comment

Uh oh!

fafk Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

jmg-duarte Apr 17, 2026

Choose a reason for hiding this comment

kaze-cow commented Mar 9, 2026 •

edited by jmg-duarte

Loading

MartinquaXD Apr 16, 2026 •

edited

Loading