Skip to content

fix(safe-eth-flow): wrap slippage-adjusted sell amount for buy orders - #7727

Merged
fairlighteth merged 7 commits into
cowprotocol:developfrom
tenderdeve:fix/7293-safe-weth-wrap-buy-order
Jun 26, 2026
Merged

fix(safe-eth-flow): wrap slippage-adjusted sell amount for buy orders#7727
fairlighteth merged 7 commits into
cowprotocol:developfrom
tenderdeve:fix/7293-safe-weth-wrap-buy-order

Conversation

@tenderdeve

@tenderdeve tenderdeve commented Jun 24, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Closes #7293

Problem

When placing a buy order in a Safe with ETH in the sell field (no WETH balance), the bundled wrap step wraps a lower amount than the order may actually settle. The order then becomes unfillable because the Safe lacks WETH to cover the executed sell amount (see linked order in the issue).

Cause

safeBundleEthFlow was wrapping context.inputAmount (the quoted sell amount). For buy orders, the signed order's sell amount can be up to quotedSell × (1 + slippageThreshold). Result: wrap amount < settle amount.

Fix

Wrap maximumSendSellAmount from useAmountsToSignFromQuote() instead. That value already accounts for the buy-order slippage threshold (and equals the input amount for sell orders), matching what the order may actually sell.

Changes

  • useSafeBundleFlowContext: expose maximumSendSellAmount on the bundle context
  • safeBundleEthFlow: use it for the wrap tx weiAmount
  • SafeBundleFlowContext: typed field added

Testing

  1. Open the app inside a Safe with ETH but no WETH
  2. Pick a Buy order
  3. Place the order with bundled wrap
  4. Wrap amount in the Safe tx matches the signed sell amount (incl. slippage), order fills cleanly

Sell-order behaviour unchanged (maximumSendSellAmount === inputAmount).

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling for Safe bundle trades by using the correct amount for native token wrapping, helping prevent under-wrapping on buy orders.
    • Kept trade flow calculations in sync when the maximum send amount changes, reducing the chance of outdated values being used.

@tenderdeve
fix(safe-eth-flow): wrap slippage-adjusted sell amount for buy orders
0f22b46 
For buy orders sold as ETH in a Safe bundle, the wrap step used the
quoted input amount instead of the maximum sell amount (which includes
the buy-order slippage threshold). When the order filled above the
quoted sell amount, WETH balance was insufficient and the order became
unfillable.

Use `maximumSendSellAmount` from `useAmountsToSignFromQuote()` as the
wrap amount so the bundled wrap covers the full settleable sell amount.

Fixes cowprotocol#7293
@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown

@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f06ce7fc-632c-4ca5-8e29-524853b606ee

📥 Commits

Reviewing files that changed from the base of the PR and between c9a58c1 and 0f22b46.

📒 Files selected for processing (3)
  • apps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.ts
  • apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts
  • apps/cowswap-frontend/src/modules/tradeFlow/types/TradeFlowContext.ts

Walkthrough

SafeBundleFlowContext gains a maximumSendSellAmount field. The useSafeBundleFlowContext hook adds a null-guard for that value and includes it in the useMemo dependency array. In safeBundleEthFlow, the wrap transaction switches from inputAmount.quotient to maximumSendSellAmount.quotient as the weiAmount.

Changes

Safe Bundle ETH Wrap Amount Fix

Layer / File(s) Summary
SafeBundleFlowContext type extension
apps/cowswap-frontend/src/modules/tradeFlow/types/TradeFlowContext.ts		 Adds maximumSendSellAmount: CurrencyAmount<Currency> to the SafeBundleFlowContext interface.
Hook null-guard and memo dependency
apps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.ts		 Extends the useMemo null-guard to bail out when maximumSendSellAmount is missing, and adds it to the dependency array.
Wrap weiAmount fix in safeBundleEthFlow
apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts		 Destructures maximumSendSellAmount from safeBundleContext and uses maximumSendSellAmount.quotient instead of inputAmount.quotient for the native token wrap transaction.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • alfetopito
  • elena-zh
  • shoom3301

Poem

🐰 Hop hop, the ETH wrap was short,
A buy order left unfilled, out of sport.
Now maximumSendSellAmount leads the way,
Wrapping just enough to save the day!
No more under-wraps in Safe's bundle play. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: wrapping the slippage-adjusted sell amount for Safe buy orders.
Description check ✅ Passed The description covers summary, problem, cause, fix, changes, and testing, though it doesn't fully follow the template's checkbox-style test format.
Linked Issues check ✅ Passed The code matches #7293 by switching Safe ETH wrapping to maximumSendSellAmount so buy orders have enough WETH to settle.
Out of Scope Changes check ✅ Passed Changes stay focused on the Safe ETH bundle flow and the new context field, with no clear unrelated edits.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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

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

@elena-zh elena-zh added the trigger-preview Add to a fork PR to trigger CF-pages preview. See https://github.com/cowprotocol/cowswap/pull/7615 label Jun 24, 2026
@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
Closed
@github-actions

github-actions Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Cloudflare Pages preview mirror

Preview branch URL: https://github.com/cowprotocol/cowswap/tree/cf-preview/pr-7727
Mirror PR: #7731
Cloudflare Pages preview links will be posted on the mirror PR by the Cloudflare Pages GitHub integration once builds complete.

Source fork branch: tenderdeve/cowswap:fix/7293-safe-weth-wrap-buy-order
Approval target SHA: ccc169c72bb8
Last mirrored SHA: 007f927568f0
Last comment update: @fairlighteth at 2026-06-26T16:23:47.029Z

  • Sync Cloudflare preview to approval target commit

@github-actions github-actions Bot removed the trigger-preview Add to a fork PR to trigger CF-pages preview. See https://github.com/cowprotocol/cowswap/pull/7615 label Jun 24, 2026
@vercel
vercel Bot temporarily deployed to Preview – storybook June 24, 2026 10:40 Inactive
@vercel
vercel Bot temporarily deployed to Preview – widget-configurator June 24, 2026 10:41 Inactive
@vercel
vercel Bot temporarily deployed to Preview – cowfi June 24, 2026 10:41 Inactive
elena-zh
elena-zh approved these changes Jun 25, 2026
View reviewed changes

@elena-zh elena-zh 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.

Thanks for fixing it

@elena-zh
elena-zh requested a review from limitofzero June 25, 2026 13:45
fairlighteth
fairlighteth reviewed Jun 26, 2026
View reviewed changes

@fairlighteth fairlighteth 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.

✅ AI Review (Codex GPT-5, worked 4m 28s): no new findings

Review completed. I found no new non-duplicate comments worth posting.

Review scope and related context

  • Checked current PR head bd7ffd2c1036b2af7ef790cf0a45e69453186409 against develop.
  • The linked issue is specifically an under-wrapped Safe buy order with ETH as the sell token; the patch makes the Safe wrap tx use maximumSendSellAmount, which is already slippage-adjusted for buy swap orders and converted to WETH in Safe ETH flow.
  • The approval path already derives needsApproval / amountToApprove from the same maximum amount, so wrap and approval are aligned.
  • Existing review state: no review threads, no actionable bot comments, and a member approval on the current head.
  • I did not do manual Safe/browser QA here; the PR body's Safe buy-order test note plus existing approval were enough that I would not raise a separate QA traceability comment.

Generated using the pr-review skill from the CoW Protocol skills repo.

@fairlighteth

fairlighteth commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
🧪 Scoped automated QA passed: mocked Safe ETH buy-order bundle construction on PR head; real Safe/browser execution not checked

Outcome

  • PR head bd7ffd2c1036b2af7ef790cf0a45e69453186409 was checked with a targeted local Jest harness against the Safe bundle-flow logic touched by this PR.
  • Buy-order ETH wrapping uses the slippage-adjusted maximumSendSellAmount: mocked quote input 1.00 ETH, maximum send 1.01 WETH, observed Safe wrap tx value 1.01 ETH.
  • Existing sell-order wrapping behavior stayed unchanged when the maximum sell amount matched the quoted input amount.
  • When allowance was required, the Safe batch order was wrap -> approve -> presign; without allowance, it stayed wrap -> presign.
  • Captured mocked-state artifact records the Safe batch payloads emitted by the executed code path.

Artifacts

Summary:

PR 7727 scoped automated QA summary

Captured mocked Safe state:

PR 7727 captured mocked Safe batch state

Run details

  • Source under test: PR fix(safe-eth-flow): wrap slippage-adjusted sell amount for buy orders #7727 head bd7ffd2c1036b2af7ef790cf0a45e69453186409 via local checkout.
  • Verification type: automated Jest assertions with mocked Safe batch sender, WETH contract data, allowance hook, quote-derived amounts, and order/app-data side effects.
  • Result: 3 test suites passed, 6 tests passed.
  • No browser recording/screenshot was produced for this run; this was not browser QA or a real Safe execution pass.

How to retest

  • Recreate the same targeted assertions around safeBundleEthFlow and useSafeBundleFlowContext.
  • Required scenarios:
    • buy order with native ETH sell token where maximumSendSellAmount > inputAmount;
    • sell order where the wrap amount remains the quoted input amount;
    • buy order requiring WETH allowance, expecting wrap -> approve -> presign;
    • artifact capture of the mocked Safe batches for wrap -> presign and wrap -> approve -> presign.

Not checked / follow-up

  • Real Safe App UI execution and Safe transaction signing.
  • Safe connected through WalletConnect.
  • Browser-visible swap flow, wallet balances, RPC responses, or order submission/fill.
  • On-chain execution of the generated Safe batch.

AI usage: Codex created the temporary local Jest harness, ran the targeted assertions, generated/uploaded the sanitized artifacts, and drafted this summary from the observed command result.

@fairlighteth
fairlighteth self-requested a review June 26, 2026 09:58
@fairlighteth

fairlighteth commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
✅ Security Review (Codex GPT-5 + DeepSec PR mode, worked ~15m): no verified security findings

Review completed. DeepSec PR mode produced no net-new findings, and follow-up code inspection found no verified security findings worth posting.

Review scope and related context
  • Checked: PR fix(safe-eth-flow): wrap slippage-adjusted sell amount for buy orders #7727 head 068b0dad3de63c0ece7d113c2e80acb4fd248d12 against develop; Safe ETH bundle context propagation, maximumSendSellAmount derivation, Safe batch construction, WETH wrap/approval ordering, and the swap flow dispatch into safeBundleEthFlow.
  • Harness used: DeepSec PR mode.
  • Source: DeepSec PR-mode output, verified against current code.
  • Harness/scanner context: run-deepsec-pr-mode.sh --base-ref origin/develop; DeepSec run 20260626104745-164b714fe7923039; exit code 0; analyses 3; findings 0; comment artifact: none written because no net-new findings were produced.
  • DeepSec status: DeepSec run.
  • Lifecycle: no verified-vulnerability findings.
  • Existing comments: CodeRabbit had no actionable comments. Earlier AI review, QA, and security-review comments referenced the previous PR head bd7ffd2c1036b2af7ef790cf0a45e69453186409; this rerun supersedes the earlier security verdict for current head 068b0dad3de63c0ece7d113c2e80acb4fd248d12.
  • Security assessment: the changed amount is derived from useAmountsToSignFromQuote() as the signed-order-side maximum sell amount, converted to WETH for Safe ETH flow, then used as the Safe deposit() value. Approval still uses the existing amount-to-approve path with the same WETH contract and trade spender. I did not find a new path for attacker-controlled transaction target/spender, order receiver, signature misuse, or unintended asset movement.
  • Confidence limits: this security pass did not exercise real Safe App signing, WalletConnect signing, browser UI flow, or on-chain execution.

Generated using the security-review skill from the CoW Protocol skills repo.

@fairlighteth
fairlighteth enabled auto-merge (squash) June 26, 2026 16:23
@fairlighteth
fairlighteth merged commit 628fc94 into cowprotocol:develop Jun 26, 2026
12 of 18 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@limitofzero limitofzero limitofzero approved these changes
@fairlighteth fairlighteth fairlighteth approved these changes
@elena-zh elena-zh elena-zh approved these changes

Assignees

@tenderdeve tenderdeve

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Not enought WETH wrapped for a buy order selling ETH in Safe

4 participants

@tenderdeve @fairlighteth @limitofzero @elena-zh