Fix EIP-4626 contract revert classification

[jmg-duarte]

Description

Testing the Euler vault tokens I found that the revert condition is a bit too strict for the Eip4626 purposes and it would think that the underlying asset was not valid because asset() failed; even though that's the expected behavior.

This PR adds a new condition for a revert caused by a lack of selector, specifically for this case.

Tested in staging, results can be seen in — https://victorialogs.dev.cow.fi/goto/ffjuyxx1tk6psc?orgId=1
native_price requests were done for 0x53afe3343f322c4189ab69e0d048efd154259419

Changes

• More logs, helps tracing through the layer peeling
• Less strict variant of is_contract_error, including empty reverts as contract errors

How to test

Call the native_price endpoint with a Euler vault address (ensure that staging has them enabled)

[gemini-code-assist]

Code Review

This pull request introduces the is_contract_revert method to the ContractErrorExt trait to specifically handle contract-level reverts, including those with empty data, and updates the EIP-4626 price estimator to utilize this check. Review feedback identifies a potential regression where DecodingError is no longer classified as a non-vault condition and highlights that transport errors should be explicitly mapped to ProtocolInternal to ensure the failover mechanism functions correctly.

[gemini-code-assist]

The implementation of is_contract_revert introduces a regression compared to the previous is_contract_error check. Specifically, it returns false for DecodingError, which occurs when a contract implements the expected selector but returns data that doesn't match the ABI (e.g., a non-vault contract that happens to have an asset() function returning a string). Previously, these were correctly classified as non-vaults in the EIP-4626 estimator. To maintain robustness against interface mismatches, DecodingError should be included.

Suggested change

	ContractError::ZeroData(..) => true,
	ContractError::ZeroData(..) | ContractError::DecodingError(..) => true,

[jmg-duarte]

The suggested error is not a thing. UnknownFunction and UnknownSelector are, and they fit this use case

[gemini-code-assist]

The error handling here does not correctly distinguish between contract-level reverts and node/transport errors. When asset() fails due to a transient transport issue (e.g., timeout or connection error), is_contract_revert() returns false, and the error falls through to be wrapped in EstimatorInternal. This prevents the FallbackNativePriceEstimator from triggering a failover to a healthy node, as it only reacts to ProtocolInternal errors. Transport errors should be explicitly caught and propagated as ProtocolInternal.

Suggested change

	Err(err) if err.is_contract_revert() && decimals_result.is_ok() => Ok(None),
	Err(err) if err.is_contract_revert() && decimals_result.is_ok() => Ok(None),
	Err(err) if err.is_node_error() => Err(PriceEstimationError::ProtocolInternal(err.into())),

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[squadgazzz]

@claude review

[claude]

Claude encountered an error —— View job

---

Code review

• Gather context
• Read changed files
• Analyze code
• Post review

---

Summary

The core fix is sound. The root cause was that empty-data reverts (e.g. calling a missing asset() selector on a non-vault) were flowing through TransportError(ErrorResp) with as_revert_data() == None, making is_node_error() return true and is_contract_error() return false — so the old check err.is_contract_error() failed to classify them as non-vault. The new is_contract_revert() correctly identifies these by checking error code 3 and the "revert" substring in the message.

crates/ethrpc/src/alloy/errors.rs

is_contract_revert() implementation (lines 60-73): The logic is correct for the Euler vault case. The three OR conditions cover the cases well:

• as_revert_data().is_some() — standard revert with ABI-encoded error
• code == 3 — Geth's execution revert (the key fix for empty-data reverts)
• message contains "revert" — fallback for non-standard nodes

ZeroData returning true is reasonable: a call returning zero bytes when an address is expected is a contract-level response, not a transport failure.

Gemini's DecodingError concern — I agree this is a valid regression. DecodingError means the contract returned data that couldn't be decoded as the expected type (e.g. a non-vault contract happens to share the asset() 4-byte selector but returns a string). Under the old is_contract_error(), this would be caught and the token classified as non-vault. Under the new is_contract_revert(), it falls to the Err(err) branch and becomes an EstimatorInternal error. The suggested fix is correct: Fix this →

// current
ContractError::ZeroData(..) => true,

// suggested
ContractError::ZeroData(..) | ContractError::DecodingError(..) => true,

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

fetch_vault_info condition (line 141): The && decimals_result.is_ok() guard is actually a meaningful improvement over the old code. It prevents incorrectly caching a token as non-vault when the RPC node is unhealthy (both calls fail). Good addition.

Gemini's ProtocolInternal concern — this is a pre-existing issue, not introduced by this PR. In the old code, transport errors for asset() also fell through to EstimatorInternal (since is_contract_error() = false for true node errors). The new code preserves the same behavior here. Fixing it would be an improvement but is out of scope for this PR.

Tests

The new tests in both files are well-structured and cover the key scenarios: empty reverts, transport failures, caller-usage errors, and the full vault-chain unwrap. No issues here.

Verdict

One blocking issue: DecodingError exclusion from is_contract_revert() is a regression. The rest of the changes look correct and well-tested.

[MartinquaXD]

Change looks reasonable to me. 👌

[MartinquaXD]

Wouldn't it be nicer to make this a forked test and test with the real USDC token?
That way, if we encounter another token that behaves weirdly we could just plug that one in and see what happens.

[jmg-duarte]

done

[squadgazzz]

LGTM