Add native token fill support - #414
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds native-token detection and native ETH handling across configs, token management, cost/profit, order validation, strategy execution, admin and quote APIs, plus an e2e native fill/settle test. It also extends default token name mappings for POL, BNB, and AVAX. ChangesNative ETH token support
Default token name mappings
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant OrderValidator as "_7683 validate_order"
participant Strategy as "SimpleStrategy"
participant TokenManager as "TokenManager"
participant CostProfit as "cost_profit"
participant AdminAPI as "admin balances"
participant QuoteValidation as "quote validation"
OrderValidator->>OrderValidator: reject native inputs
Strategy->>TokenManager: check native balance with chain_id and None
TokenManager-->>Strategy: native balance
Strategy->>Strategy: execute using native balance key
CostProfit->>CostProfit: convert native gas and fees to USD
CostProfit->>CostProfit: set fill tx value for native output
QuoteValidation->>TokenManager: native-compatible destination balance lookup
AdminAPI->>AdminAPI: build native balance row from configured metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/solver-types/src/utils/conversion.rs (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading doc comment on
is_native_address.The comment says "True iff a solver address is exactly the 20-byte zero address," but the function is used generically for any token/config address (e.g. detecting native tokens in
TokenManager,context.rs, admin balances), not specifically the solver's own address.📝 Suggested doc fix
-/// True iff a solver address is exactly the 20-byte zero address. +/// True iff the given address is exactly the 20-byte zero address (native token sentinel). pub fn is_native_address(addr: &Address) -> bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/solver-types/src/utils/conversion.rs` around lines 30 - 33, The doc comment for is_native_address is misleading because the helper is used as a generic zero-address check for token/config addresses, not just a solver address. Update the comment on is_native_address in conversion.rs to describe it as checking whether an Address is the 20-byte zero address and note its generic native-token/native-address usage, so callers like TokenManager and context.rs are accurately documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/solver-types/src/utils/conversion.rs`:
- Around line 30-33: The doc comment for is_native_address is misleading because
the helper is used as a generic zero-address check for token/config addresses,
not just a solver address. Update the comment on is_native_address in
conversion.rs to describe it as checking whether an Address is the 20-byte zero
address and note its generic native-token/native-address usage, so callers like
TokenManager and context.rs are accurately documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94a44ca3-bfbb-4a11-a894-225f96e5dc01
📒 Files selected for processing (22)
config/demo.jsonconfig/example-broadcaster.jsonconfig/example-hyperlane.jsonconfig/non-seeded-networks-example.jsonconfig/seed-overrides-kms-example.jsonconfig/seed-overrides-mainnet.jsonconfig/seed-overrides-testnet.jsoncrates/solver-core/src/engine/context.rscrates/solver-core/src/engine/cost_profit.rscrates/solver-core/src/engine/token_manager.rscrates/solver-core/src/handlers/intent.rscrates/solver-e2e-tests/src/lib.rscrates/solver-e2e-tests/tests/happy_e2e_native_fill_settle.rscrates/solver-order/src/implementations/standards/_7683.rscrates/solver-order/src/implementations/strategies/simple.rscrates/solver-pricing/src/lib.rscrates/solver-service/src/apis/admin.rscrates/solver-service/src/apis/quote/mod.rscrates/solver-service/src/apis/quote/validation.rscrates/solver-types/src/lib.rscrates/solver-types/src/utils/conversion.rscrates/solver-types/src/utils/mod.rs
Native output + Dutch-auction:
|
Native pricing fallback is asymmetric: gas has it, output/input valuation doesn'tVerified in
Net effect is safe (fails closed — the order errors out rather than being mispriced), so this isn't a fund-safety bug. But it means the "config-first with known-chain fallback" behavior applies to gas only: to actually serve a native output, an operator must add a zero-address Suggestion: either route native amounts in |
Non-18-decimal native tokens are not cleanly supported (gas leg couples to output decimals)Traced the decimals handling across the changed paths. This PR effectively assumes an 18-decimal native token (all shipped configs, tests, and the fallback table assume 18). A chain whose native token isn't 18 decimals isn't safely supported. Details: Works regardless of decimals:
Config-driven, honors non-18 if a zero-address
Assumes 18:
The actual problem — the gas leg couples to the output-token Separately, the Suggestion: if 18-decimal-only is the intended scope, assert/validate it (reject a zero-address |
|
@pepebndc pushed a follow-up. native output now errors clearly when the chain has no native token config (kept the gas fallback, it has to price every order incl erc20-only). documented the 18-dec native assumption + added a debug_assert, and fixed the i64 pow overflow (there was a second unguarded spot in convert_usd_to_token_amount too, both use checked i128 now). left the simple.rs check as is since the preflight already gates gas+value, just added a comment. re dutch native: we reject dutch/exclusive outputs at validation, so it never reaches the fill builder. added a test to lock that. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/solver-core/src/engine/cost_profit.rs`:
- Around line 3194-3196: The native valuation error messages in
CostProfitError::Calculation are exceeding Rust’s 100-column limit. Wrap the
long strings in cost_profit.rs by splitting the message across adjacent string
literals or formatting the format! call over multiple lines, and apply the same
fix to the other native valuation error block referenced in the same area so the
Rust formatting stays within rustfmt rules.
- Around line 2929-2934: The native gas pricing path in cost_profit.rs only uses
debug_assert_eq! to guard the 18-decimal assumption, so release builds can still
misprice when TokenConfig.decimals is not 18. Update the native gas conversion
logic around Self::convert_raw_token_to_usd to explicitly fail closed at runtime
for non-18 decimal native tokens (for example by returning an error or otherwise
aborting pricing for that config) instead of relying on debug assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49e2dd28-0e9f-41c5-aeca-5df7dfaa790b
📒 Files selected for processing (2)
crates/solver-core/src/engine/cost_profit.rscrates/solver-order/src/implementations/strategies/simple.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/solver-order/src/implementations/strategies/simple.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/solver-core/src/engine/cost_profit.rs (1)
3148-3153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: unify with the checked/
try_variant used inconvert_usd_to_token_amount.Here the
> 28guard makes10_i128.pow(...)andfrom_i128_with_scalesafe, but the sibling path at Line 3324 useschecked_pow+try_from_i128_with_scaleto fail cleanly. Aligning both on the non-panicking variants would make the invariant self-documenting and robust to future edits that relax the guard. No behavioral change today.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/solver-core/src/engine/cost_profit.rs` around lines 3148 - 3153, The power/divisor construction in the token amount conversion path currently relies on the `decimals <= 28` guard, but the sibling `convert_usd_to_token_amount` uses checked/try-based APIs for safer failure behavior. Update the `raw_amount_decimal / divisor` logic to use the same `checked_pow` and `try_from_i128_with_scale` pattern as `convert_usd_to_token_amount`, so the `cost_profit` conversion path stays self-documenting and remains robust if the guard changes later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/solver-core/src/engine/cost_profit.rs`:
- Around line 3148-3153: The power/divisor construction in the token amount
conversion path currently relies on the `decimals <= 28` guard, but the sibling
`convert_usd_to_token_amount` uses checked/try-based APIs for safer failure
behavior. Update the `raw_amount_decimal / divisor` logic to use the same
`checked_pow` and `try_from_i128_with_scale` pattern as
`convert_usd_to_token_amount`, so the `cost_profit` conversion path stays
self-documenting and remains robust if the guard changes later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29ec745d-9bba-42a0-a316-e9deecb8f0dd
📒 Files selected for processing (1)
crates/solver-core/src/engine/cost_profit.rs
The PR adds the native-output e2e test but never wired it into CI, so the headline feature's end-to-end coverage would not run on any PR. Verified passing locally twice against real Anvil chains. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
msg.valuein runtime and quote-time fill transaction builders.Validation
cargo fmt --all -- --checkcargo clippy --all-features --all-targets -- -D warnings --allow deprecatedcargo check --all-targetscargo test -p solver-configcargo test -p solver-ordercargo test -p solver-corecargo test -p solver-serviceOIF_CONTRACTS_PATH=/Users/nahimdhaney/openzeppelin/oif-solver/oif-contracts cargo test -p solver-e2e-tests --test happy_e2e_native_fill_settle -- --ignored --test-threads=1 --nocaptureSummary by CodeRabbit
New Features
valuebehavior).Bug Fixes
Chores