fix(widget): fire ON_BEFORE_APPROVAL widget hook before permit signing#7697
fix(widget): fire ON_BEFORE_APPROVAL widget hook before permit signing#7697tenderdeve wants to merge 10 commits into
Conversation
For permittable tokens, CowSwap signs an EIP-2612 permit instead of performing an on-chain ERC-20 approval. The ON_BEFORE_APPROVAL widget hook, however, was only fired from the on-chain approval path (useApproveCurrency), so widget integrators that subscribed to onBeforeApproval never saw the event when the user was about to sign a permit. From the integrator's perspective the user authorized spending without their pre-approval handler ever running. Fire ON_BEFORE_APPROVAL in the swap and limit order trade flows right before requesting the permit signature, mirroring the payload shape used by useApproveCurrency (chainId, sellToken, sellAmount, walletAddress, spenderAddress). The hook is skipped when a cached permit is reused, matching the behaviour of the existing permit request UI step. If the integrator's hook returns false, the relevant flow aborts cleanly without dispatching a permit request: - swap flow returns false, just like a declined price impact; - limit order flow throws a new WidgetHookDeclineError which is swallowed by useHandleOrderPlacement and only dismisses the trade confirmation modal. Closes cowprotocol#7685
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces ChangesWidget hook approval gating for permit signing
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as OrderPlacement UI
participant TradeFlow as Limit Orders / Swap Flow
participant Widget as Widget Hook
participant Wallet
User->>UI: Initiate order/swap
UI->>TradeFlow: Start trade flow
TradeFlow->>TradeFlow: Check cached permit
alt No cached permit
TradeFlow->>Widget: callWidgetHook(ON_BEFORE_APPROVAL)
alt Widget hook approved
Widget-->>TradeFlow: Permission granted
TradeFlow->>Wallet: Request permit signature
Wallet-->>TradeFlow: Signed
else Widget hook declined
Widget-->>TradeFlow: Declined
TradeFlow-->>UI: WidgetHookDeclineError
UI->>User: Dismiss confirm dialog
end
else Cached permit exists
TradeFlow->>TradeFlow: Proceed to beforePermit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.ts`:
- Around line 102-103: The WidgetHookDeclineError thrown at the throw statement
is an expected abort path that should not be treated as a generic swap error in
error analytics. Locate the catch block that handles exceptions from the code
path containing the WidgetHookDeclineError throw statement, and add a specific
check to detect if the caught error is an instance of WidgetHookDeclineError.
When this error type is caught, handle it separately by either skipping the
generic error capture/analytics logic or routing it through a non-error
telemetry path, ensuring that expected widget-hook declines do not create
analytics noise in the swap error reporting.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 565e6df6-e142-4427-a042-ddee973a2c43
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/limitOrders/hooks/useHandleOrderPlacement.tsapps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.tsapps/cowswap-frontend/src/modules/limitOrders/services/types.tsapps/cowswap-frontend/src/modules/tradeFlow/services/swapFlow/index.ts
| if (!isWidgetHookPassed) throw new WidgetHookDeclineError() | ||
| } |
There was a problem hiding this comment.
Do not report widget-hook declines as swap errors.
At Line 102, WidgetHookDeclineError is an expected abort path, but it currently goes through generic error capture/analytics in the catch block, which will create telemetry noise.
Suggested fix
} catch (err: unknown) {
const error = normalizeError(err)
+ if (error instanceof WidgetHookDeclineError) {
+ throw error
+ }
logTradeFlow('LIMIT ORDER FLOW', 'STEP 9: ERROR: ', error)
const swapErrorMessage = getSwapErrorMessage(error)🤖 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 `@apps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.ts`
around lines 102 - 103, The WidgetHookDeclineError thrown at the throw statement
is an expected abort path that should not be treated as a generic swap error in
error analytics. Locate the catch block that handles exceptions from the code
path containing the WidgetHookDeclineError throw statement, and add a specific
check to detect if the caught error is an instance of WidgetHookDeclineError.
When this error type is caught, handle it separately by either skipping the
generic error capture/analytics logic or routing it through a non-error
telemetry path, ensuring that expected widget-hook declines do not create
analytics noise in the swap error reporting.
|
Cloudflare Pages preview mirror Preview branch URL: https://github.com/cowprotocol/cowswap/tree/cf-preview/pr-7697 Source fork branch:
|
Treat the widget-hook decline as an expected abort path rather than a generic swap error: re-throw before the catch block routes it through captureError and analytics, so declined ON_BEFORE_APPROVAL hooks no longer pollute error telemetry. Addresses @coderabbitai's feedback on cowprotocol#7697.
|
@coderabbitai updated in f9d6050 — re-throw |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the commit f9d605085 to verify the re-throw of WidgetHookDeclineError
git show f9d605085 --stat
git show f9d605085Length of output: 2213
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve, thank you for the fix.
It is working on Limit orders page while is not working on Swap page.
Could you please fix it?
The previous fix only wired ON_BEFORE_APPROVAL into the permit-signing paths (limit orders + swap permit flow). On the Swap page with a non-permittable token, the approval still goes through `useTradeApproveCallback` -> `approveCallback`, which never fired the hook. Result: the widget's onBeforeApproval listener never ran on the Swap page in that scenario. Call the hook in `useTradeApproveCallback` right before the ERC20 `approve` transaction. If the host widget declines, reset the approve progress modal and bail out without sending the tx, matching the behaviour of the other hook call sites. Addresses @elena-zh's review on cowprotocol#7697.
On the Swap page, when the user clicks Approve for a permittable token, the trade form pre-signs an EIP-2612 permit via `useGeneratePermitInAdvanceToTrade`. That path skipped the `ON_BEFORE_APPROVAL` widget hook, so the host widget's `onBeforeApproval` listener never ran on Swap with permittable tokens (it worked on Limit Orders because the permit is signed inside the limit-order `tradeFlow`, which already calls the hook). Call the hook in `useGeneratePermitInAdvanceToTrade` right before `generatePermit`. If the host widget declines, bail out without signing, matching the behaviour of the other hook call sites. Also drop the speculative hook call I added to `useTradeApproveCallback` in 97b1ab5: `useApproveCurrency` (the only path that consumes `useTradeApproveCallback` for the Swap form) already fires the hook, so the extra call would double-prompt. Addresses @elena-zh's follow-up review on cowprotocol#7697.
|
@elena-zh updated in 6653bb1 — root cause was the Swap page's pre-sign permit path ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.ts`:
- Around line 30-45: The ON_BEFORE_APPROVAL hook invocation in the conditional
block for tradeSpenderAddress bypasses the pre-approval gate when
tradeSpenderAddress is falsy, allowing the function to proceed directly to
generatePermit without gating. Move the callWidgetHook invocation for
WidgetHookEvents.ON_BEFORE_APPROVAL outside the if (tradeSpenderAddress)
conditional check so the hook is always invoked before any permit generation
logic, ensure the early return if (!isWidgetHookPassed) return false is always
evaluated regardless of tradeSpenderAddress availability, and adjust the hook
payload to handle cases where tradeSpenderAddress might be undefined while still
maintaining the necessary validation gate.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b334a9f-34e5-4923-a5b2-7592697c9349
📒 Files selected for processing (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.ts
If \`tradeSpenderAddress\` was unavailable, the previous version skipped the \`ON_BEFORE_APPROVAL\` widget hook but still called \`generatePermit\`, letting a permit be signed without the pre-approval gate. Treat a missing spender as a hard precondition (same as missing \`account\` / \`permitInfo\`) so the hook is always evaluated before any permit signature. Addresses @coderabbitai's review on cowprotocol#7697.
|
@coderabbitai addressed in this push — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.tsLength of output: 3057
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai this one is already handled — the limit-order flow's catch block re-throws |
|
Tip For best results, initiate chat on the files or code changes.
|
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 17m): cached permits still trigger the approval hook
Finding: [BLOCKING] Skip ON_BEFORE_APPROVAL when reusing a cached permit
- Location:
useGeneratePermitInAdvanceToTrade.ts:31,limitOrders/services/tradeFlow/index.ts:86 - This one is important: the PR says cached permits skip the hook, but generated permits are cached by chain, token, spender, account, and amount.
- Limit orders call
getCachedPermit(sellToken.address)withoutpermitAmountToSign, so an amount-keyed cached permit is missed. - The advance-permit flow calls the hook before
generatePermit, whose internal cache lookup happens afterward. - Consequently, Swap and Limit flows can ask the integrator for approval again and abort when it returns
false, even though no permit signature is needed. - Related work: #7838, stacked on the permit-cache changes in #7837, implements the same permit-in-advance gate through a shared helper. Its current implementation also invokes the hook before
generatePermitchecks the cache, so it does not address this cached-reuse case.
Suggested fix
- Coordinate with #7838 and make the shared approval gate cache-aware, or invoke it immediately after the permit generator confirms a cache miss.
- Pass the same token, amount, and spender used by permit generation when checking the limit-order cache.
- Add regression tests proving cached permits skip the hook, while an uncached decline prevents permit signing in both flows.
- Avoid merging two divergent implementations; port only #7697’s unique limit-order and lower-level
swapFlowcoverage into the shared approach if those paths require it.
Review scope and related context
- Of the current open PRs, only #7838 overlaps a real source file with #7697; it changes
useGeneratePermitInAdvanceToTrade.ts. The other four files changed by #7697 remain unique. #7699 is only its preview mirror. - The unresolved CodeRabbit telemetry thread is stale: current code rethrows
WidgetHookDeclineErrorbefore error capture and analytics. - The resolved missing-spender finding is fixed by the guard in
useGeneratePermitInAdvanceToTrade. - Unit tests, lint, and typecheck passed in CI. Cypress did not execute its scenarios because
INTEGRATION_TEST_PRIVATE_KEYwas missing. - Security review incomplete: DeepSec PR mode was attempted against the exact five changed files but exited before analysis because
deepsec@2.1.2depends on unpublished@aws-sdk/token-providers@3.1083.0; no comment artifact was produced.
🤖 Prompt for AI agents
Verify this finding against current code and the stacked implementation in PRs #7837/#7838.
Context:
- Generated permits are cached using token, spender, account, and amount.
- Limit-order code checks the cache without permitAmountToSign.
- Both #7697 and #7838 call ON_BEFORE_APPROVAL before generatePermit performs its cache lookup.
- Cached permits must bypass ON_BEFORE_APPROVAL; uncached permits must still call it before signing.
- Prefer one shared implementation and add focused cached/uncached/declined tests.
Generated using the pr-review skill from the CoW Protocol skills repo.
|
Hey @tenderdeve , could you please resolve conflicts in the branch? |
Summary
Closes #7685.
When a permittable token is selected in a widget that has the `onBeforeApproval` hook configured, the widget integrator never sees the hook fire. Instead, the user is immediately asked to sign the EIP-2612 permit and no pre-approval action is dispatched.
Root cause
`ON_BEFORE_APPROVAL` is only fired from the on-chain ERC-20 approval path (`useApproveCurrency`). For permittable tokens, CowSwap signs an EIP-2612 permit instead of performing an on-chain approval, so the hook path is bypassed entirely — even though, from an integrator's perspective, the permit signature is the user's "approval" of token spending.
Fix
Fire `ON_BEFORE_APPROVAL` in both the swap and limit order trade flows right before requesting the permit signature, mirroring the payload shape used by `useApproveCurrency`:
```ts
{ chainId, sellToken, sellAmount, walletAddress, spenderAddress }
```
The hook is skipped when a cached permit is reused, matching the existing permit request UI step's behaviour. If the integrator's hook returns `false`, the flow aborts cleanly:
No other flows are affected (TWAP does not use permit signatures).
Test plan
Summary by CodeRabbit
New Features
Bug Fixes