fix(trading)!: eth-flow order-id collision avoidance was completely inert

[gomesalexandre]

tl;dr

calculateUniqueOrderId's entire job is to avoid eth-flow order-id collisions. It never actually did — the transaction it hands back always recreates the exact colliding order, which reverts on-chain with OrderIsAlreadyOwned and burns the user's gas.

The bug

calculateUniqueOrderId (packages/trading/src/calculateUniqueOrderId.ts) detects a collision, nudges buyAmount down 1 wei, and recurses to compute a fresh, collision-free orderId — but it only ever returned that bare orderId string, never the adjusted order that produced it:

function adjustAmounts(order: UnsignedOrder): UnsignedOrder {
  return { ...order, buyAmount: (buyAmount - BigInt(1)).toString() }   // new object, discarded
}
// ...
return orderId   // just the id — the adjusted order it came from is gone

The caller, getEthFlowTransaction.ts, builds the actual on-chain transaction from its own original, un-nudged order:

const orderId = await calculateUniqueOrderId(chainId, orderToSign, checkEthFlowOrderExists, protocolOptions)
const ethOrderParams: EthFlowOrderData = {
  ...
  buyAmount: orderToSign.buyAmount,     // the ORIGINAL, unadjusted amount
}
const data = contract.interface.encodeFunctionData('createOrder', [ethOrderParams])

So the returned orderId describes an order that never actually gets built. The transaction that gets sent recreates the exact order checkEthFlowOrderExists just reported as colliding, and CoWSwapEthFlow.createOrder reverts with OrderIsAlreadyOwned. This also flows into the public OrderPostingResult returned by postSellNativeCurrencyOrder — any caller polling orderId tracks an order that will never exist.

The fix

calculateUniqueOrderId now returns { orderId, order } — the order that actually produced that id — and getEthFlowTransaction builds ethOrderParams, the gas estimate, the tx value, and its own returned orderToSign from that order, not from its original input.

BREAKING CHANGE: calculateUniqueOrderId's return type changes from Promise<string> to Promise<{ orderId: string; order: UnsignedOrder }>. It's exported public API (index.ts); any external caller reading the bare string needs to destructure .orderId. Given the old signature was silently unusable for its stated purpose, I think this is worth doing now rather than deprecating in place — happy to go a different route if maintainers prefer.

Also added a guard while I was in there: the nudge-by-1 logic had no floor. A buyAmount of 1 (or a second collision hitting an already-adjusted order) would silently produce buyAmount: 0 or a negative BigInt that can't encode as uint256. It now throws a clear error instead of building an invalid order.

Out of scope (not fixed here, flagging for a maintainer)

• calculateUniqueOrderId hashes order.kind as supplied, but the EthFlow contract always treats orders as SELL. For a BUY-kind order, the computed id could diverge from the transaction's real id. This is pre-existing and unrelated to the collision bug — didn't want to conflate two different fixes in one PR.
• getEthFlowTransaction's gas-estimation .catch() swallows errors (including a TOCTOU OrderIsAlreadyOwned revert from a different order created between the collision check and submission) and falls back to a default gas limit rather than surfacing them. Also pre-existing, also a separate concern from what this PR fixes.

Testing

Real repro before touching code — ported the control flow of both functions with a stubbed checkEthFlowOrderExists reporting a collision, confirmed the returned orderId didn't match what the returned transaction's calldata would actually create, and confirmed unfixed code returns the original colliding buyAmount.

New regression tests confirmed to fail on unfixed code and pass on fixed code (verified via git stash back to the original source with the new tests still in place):

● calculateUniqueOrderId › ... Then the returned order (not just the id) reflects the adjusted amounts
● calculateUniqueOrderId › ... Then the returned order is exactly the original order, unmodified
● getEthFlowTransaction › ... builds the on-chain transaction from the adjusted order, not the original

All three fail on main, all three (plus 2 new underflow-guard tests) pass after the fix.

Full package suite: 261 passed, 2 skipped (pre-existing skips, unrelated to this change). pnpm run typecheck clean across the whole monorepo (confirms no other consumer of the changed return type broke). eslint clean on all four touched files.

Ran Codex (GPT-5.6) as an adversarial reviewer against the diff. It found two real issues, both fixed in this PR: the buyAmount underflow case above, and correctly noting the kind/TOCTOU items are pre-existing and out of scope (both listed above rather than folded in).

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of duplicate order identifiers by consistently applying adjusted order amounts throughout transaction creation.
  • Prevented order adjustments when the buy amount is too small to change safely; a clear error is now returned.
  • Ensured transaction data, gas estimates, and payment values reflect the finalized order.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: cabbc514-9b93-4738-965c-efd72530d4e4

📥 Commits

Reviewing files that changed from the base of the PR and between b4ff485 and 0fdf3e9.

📒 Files selected for processing (4)

• packages/trading/src/calculateUniqueOrderId.test.ts
• packages/trading/src/calculateUniqueOrderId.ts
• packages/trading/src/getEthFlowTransaction.test.ts
• packages/trading/src/getEthFlowTransaction.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

calculateUniqueOrderId now returns the generated ID and the order used to generate it. Collision adjustments propagate through EthFlow transaction construction. Amounts that cannot be reduced further now cause an error.

Changes

EthFlow collision handling

Layer / File(s)	Summary
Return the collision-adjusted order packages/trading/src/calculateUniqueOrderId.ts, packages/trading/src/calculateUniqueOrderId.test.ts	calculateUniqueOrderId returns orderId with the order that produced it. Collision tests cover adjusted and unchanged orders. Amounts of 1 or less now throw the adjustment error.
Use the adjusted order in EthFlow transactions packages/trading/src/getEthFlowTransaction.ts, packages/trading/src/getEthFlowTransaction.test.ts	EthFlow calldata, transaction value, gas estimation, and orderToSign use the collision-adjusted order. Tests verify adjusted and unchanged amounts.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to 0fdf3

The fix now submits the same adjusted order used to derive the collision-free ID, while the public calculateUniqueOrderId API changes from returning a string to returning an object; external consumers that have not migrated could break or track the result incorrectly. The PR is mergeable with explicit owner awareness of that bounded migration risk.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant getEthFlowTransaction
  participant calculateUniqueOrderId
  participant EthFlowContract
  Caller->>getEthFlowTransaction: Submit order
  getEthFlowTransaction->>calculateUniqueOrderId: Generate unique order ID
  calculateUniqueOrderId->>EthFlowContract: Check order ID collision
  EthFlowContract-->>calculateUniqueOrderId: Collision result
  calculateUniqueOrderId-->>getEthFlowTransaction: ID and adjusted order
  getEthFlowTransaction->>EthFlowContract: Encode adjusted createOrder calldata
  getEthFlowTransaction-->>Caller: Return transaction and orderToSign

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary change: fixing inert EthFlow order-ID collision avoidance. It is concise, specific, and related to the changeset.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}