Skip to content

[EASY] Make solver trade fee non-optional - #4964

Closed
0x2me wants to merge 5 commits into
cowprotocol:mainfrom
0x2me:remove-optional-solver-fee
Closed

0x2me wants to merge 5 commits into
cowprotocol:mainfrom
0x2me:remove-optional-solver-fee

Conversation

@0x2me

@0x2me 0x2me commented Sep 23, 2026

Copy link
Copy Markdown

Description

Finishes #2507. Error::ProtocolFeeOnStaticOrder was already removed in #4832; this is the dependent cleanup the issue asks for: fee on Fulfillment and JitTrade in solvers-dto becomes a plain U256 instead of Option<U256>.

Same approach as #3822 (approved in principle there), but narrower: the driver surplus_fee() refactor is left for a follow-up so this stays a small review.

Breaking change to the solver API (crates/solvers/openapi.yml). fee on Fulfillment was documented as "only gets returned for limit orders" and is now required. A solver response containing a fulfillment without fee is rejected at deserialisation by the driver, including on the quoting path, and the whole response is discarded, not just that trade. JitTrade already listed fee as required, so this only tightens the Fulfillment half. Every in-tree solver always emitted fee, so the exposure is limited to external solver engines that relied on the old wording. If a softer landing is preferred, #[serde(default)] on the two fields delivers the same code cleanup while keeping old payloads working; happy to switch.

Changes

  • solvers-dto: fee: U256 on Fulfillment and JitTrade; serde tests for missing and present fee.
  • solvers: drop the Some(..) wrapping in the solution DTO.
  • driver: read fee directly instead of unwrap_or_default(); the test mock solver always emits fee.
  • e2e: fixtures updated.
  • openapi.yml: fee added to Fulfillment.required, description rewritten and typed as TokenAmount.

How to test

  1. cargo nextest run -p solvers-dto (new: fulfillment_requires_fee, fulfillment_fee_round_trips, jit_trade_requires_fee).
  2. cargo nextest run and cargo clippy --locked --workspace --all-features --all-targets -- -D warnings: green locally.
  3. RUST_MIN_STACK=3145728 cargo nextest run -p driver --test-threads 1 --run-ignored ignored-only: 96/96 locally.
  4. E2E suite was compile-checked only (cargo check -p e2e --tests), not run locally; please watch CI.

Related Issues

Fixes #2507

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@0x2me
0x2me force-pushed the remove-optional-solver-fee branch 2 times, most recently from 3fc121b to 52c1836 Compare September 23, 2026 12:19
@0x2me

0x2me commented Sep 23, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@0x2me

0x2me commented Sep 23, 2026

Copy link
Copy Markdown
Author

recheck

github-actions Bot added a commit that referenced this pull request Sep 23, 2026
@0x2me
0x2me force-pushed the remove-optional-solver-fee branch from 52c1836 to 2e2b6db Compare September 23, 2026 12:36
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

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.

@0x2me

0x2me commented Oct 14, 2026

Copy link
Copy Markdown
Author

Reopened as #5100 (rebased on main; GitHub would not reopen this one after the force-push).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: Remove Error::ProtocolFeeOnStaticOrder

1 participant