Skip to content

Use the widening mul_ratio helper in the driver fee-to-sell-token conversion#4671

Open
squadgazzz wants to merge 1 commit into
mainfrom
driver/overflow-safe-fee-conversion
Open

Use the widening mul_ratio helper in the driver fee-to-sell-token conversion#4671
squadgazzz wants to merge 1 commit into
mainfrom
driver/overflow-safe-fee-conversion

Conversation

@squadgazzz

@squadgazzz squadgazzz commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Description

Companion to #4669. The driver's protocol_fee_in_sell_token converts a buy-token fee into the sell token by multiplying with the solution's clearing prices in 256 bits, the same unwidened pattern that zeroed 26 executions on xdai when a solver scaled its prices to 1e60. Here the input is a /solve response rather than solver-authored calldata, so nothing has broken yet, but an overflow silently discards an otherwise valid solution and puts an arbitrary cap on how solvers may scale their solution prices.

The conversion now goes through number::U256Ext::checked_mul_ratio, the shared helper #4669 introduced (the driver's own util::math wrappers were folded into it there).

Changes

  • protocol_fee_in_sell_token uses checked_mul_ratio with an explicit zero-divisor guard, preserving the DivisionByZero vs Overflow distinction

How to test

Existing tests. The conversion now delegates to checked_mul_ratio, whose unit tests already cover the widening and rounding behavior; a fixture-heavy test at this level would only re-prove the helper.

Related issues

Fixes #4670

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @squadgazzz's task in 1m 50s —— View job


PR Review: Overflow-safe fee conversion

  • Read the changed file and diff
  • Review mul_ratio helper and call site semantics
  • Check overflow/rounding/zero-divisor behavior
  • Post review feedback

Verdict: LGTM. The change is correct and faithfully preserves the previous semantics while fixing the latent false-overflow.

  • Rounding preserved — the old checked_mul(...).checked_div(...) truncated (rounded down); mul_ratio also rounds down (x * q / d), so the fee amount is unchanged for all inputs that didn't overflow before.
  • Error distinction preservedmul_ratio collapses a zero divisor into None (→ Overflow), so the explicit prices.sell.is_zero() guard is exactly what's needed to keep returning DivisionByZero. Good call keeping that.
  • Fix is real — widening fee * prices.buy to 512 bits means a solver scaling clearing prices no longer trips a spurious Overflow when the final fee * buy / sell still fits in 256 bits. The only remaining Overflow is a genuine one.

Non-blocking follow-up (out of scope): adjust_quote_to_order_limits in the same file carries the identical unwidened checked_mul(...).ok_or(Overflow)?.checked_div(...) pattern (sell side fee.rs:268-278, buy side fee.rs:290-294). Those operate on quote/order amounts rather than solver-scaled clearing prices, so they're far less exposed — but they're the same latent shape this series is targeting. Might be worth folding into mul_ratio in a later PR for consistency.

Skipping a fixture-heavy test here is reasonable given the helper's existing unit coverage of widening/rounding.

@squadgazzz
squadgazzz force-pushed the driver/overflow-safe-fee-conversion branch from 27e30dc to f786297 Compare July 27, 2026 09:14
@squadgazzz
squadgazzz changed the base branch from main to autopilot/overflow-safe-settlement-fee-math July 27, 2026 09:14

@jmg-duarte jmg-duarte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but another concern about how we're modelling the math

Comment on lines +217 to +218
.checked_mul_ratio(&prices.buy, &prices.sell)
.ok_or(Math::Overflow)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just make the checked_mul_ratio return Result instead of Option; since it already checks for the div by zero internally, it would make more sense to erase the error into an option as needed instead of duplicating the check to augment the option into a result (with error info)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes sense now. Will open a follow-up PR out of the hotfix.

Base automatically changed from autopilot/overflow-safe-settlement-fee-math to main July 27, 2026 10:01
@squadgazzz
squadgazzz force-pushed the driver/overflow-safe-fee-conversion branch from f786297 to 1fdc3e0 Compare July 27, 2026 10:18
@squadgazzz
squadgazzz enabled auto-merge July 27, 2026 10:23
@squadgazzz
squadgazzz disabled auto-merge July 27, 2026 10:23
@squadgazzz
squadgazzz force-pushed the driver/overflow-safe-fee-conversion branch from 1fdc3e0 to 8aa8fa1 Compare July 27, 2026 10:26
@squadgazzz
squadgazzz enabled auto-merge July 27, 2026 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Driver protocol_fee_in_sell_token can overflow on large solution prices

3 participants