feat(approve): adjust partial approves for buy orders #6376
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
WalkthroughReplaces per-token last-approve tracking with a global optimistic allowances store; adds utilities/hooks to set/derive optimistic allowances from approval tx logs; centralizes approve-and-swap orchestration; updates approval hooks, UI components, tests, and removes legacy last-approve plumbing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TradeApproveButton
participant Hook as useApproveAndSwap
participant Permit as PermitFlow
participant Approver as ApprovalFlow
participant Chain as Blockchain
participant Store as OptimisticAllowances
User->>UI: click "Approve"
UI->>Hook: invoke(amount, params)
alt permit path (supported & used)
Hook->>Permit: generate/sign permit
Permit->>Chain: submit signature
Chain-->>Permit: result
Hook->>UI: continue with permit (confirmSwap)
else standard approval
Hook->>Approver: send approval tx
Approver->>Chain: broadcast tx
Chain-->>Approver: tx receipt
Approver->>Approver: processApprovalTransaction(receipt)
Approver->>Store: setOptimisticAllowance(key, { amount, blockNumber, timestamp })
Approver-->>Hook: { txResponse, approvedAmount }
Hook->>UI: validate approvedAmount → confirmSwap()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
shoom3301
changed the title
| return { maximumSendSellAmount, minimumReceiveBuyAmount } | ||
| }, [isQuoteBasedOrder, inputCurrencyAmount, outputCurrencyAmount, afterSlippage]) | ||
| return { | ||
| // Add 1% threshold for buy orders to level out price/gas fluctuations |
There was a problem hiding this comment.
1% addition to approving amount for buy orders
| ONCHAIN_TRANSACTIONS_EVENTS.emit(OnchainTxEvents.BEFORE_TX_FINALIZE, { transaction, receipt }) | ||
|
|
||
| // Once approval tx is mined, we add the priority allowance to immediately allow the user to place orders | ||
| if (transaction.approval) { |
There was a problem hiding this comment.
It was removed because now we have optimistic allowance
| const approvedAmount = tx.approvedAmount | ||
| const isApprovedAmountSufficient = Boolean(approvedAmount && approvedAmount >= amountToApproveBig) | ||
|
|
||
| if (isApprovedAmountSufficient) { |
There was a problem hiding this comment.
Prevent opening swap confirm if there is not enough allowance after tx mined
| amountToApprove={partialAmountToApprove} | ||
| /> | ||
| {typeof currentAllowance === 'bigint' && currencyToApprove && ( | ||
| <TradeAllowanceDisplay currentAllowance={currentAllowance} currencyToApprove={currencyToApprove} /> |
There was a problem hiding this comment.
Display current allowance amount
| txResponse: TransactionReceipt, | ||
| ): SetOptimisticAllowanceParams | null { | ||
| if (txResponse.status !== 1) { | ||
| throw new Error('Approval transaction failed') |
There was a problem hiding this comment.
Prevent successful approval path if tx was not successfuly mined
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveToggle/TradeApproveToggle.tsx (1)
18-23: Unwrapping the fragment is fineDirectly returning Toggle is clean; no behavior change.
Consider returning JSX.Element instead of ReactNode for component return type to be more specific (and drop the ReactNode import).
apps/cowswap-frontend/src/entities/optimisticAllowance/getOptimisticAllowanceKey.ts (1)
1-7: Deterministic composite key looks goodLowercasing token/owner/spender with chainId provides a stable key for the store. Works well with the setter.
If inputs may arrive in mixed formats, consider getAddress normalization before lowercasing to guard against malformed strings.
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
69-81: More robust log parsing (guard topics, pick last match) and safer status check
- Prefer the last matching Approval log to reflect the final state if multiple approvals occur in one tx.
- Guard topics length to avoid exceptions in malformed logs (reduces console noise).
- Treat explicit status === 0 as failure; some providers may return null/undefined.
Apply these diffs:
@@ - if (txResponse.status !== 1) { + if (txResponse.status === 0) { throw new Error('Approval transaction failed') }@@ - // Find the Approval event log - const approvalLog = txReceipt.logs.find((log) => { - // Check if it's from the token contract and has the Approval event signature - if (log.address.toLowerCase() !== tokenAddress.toLowerCase()) return false - if (log.topics[0] !== APPROVAL_EVENT_TOPIC) return false - - // Verify owner and spender match (topics[1] = owner, topics[2] = spender) - const logOwner = getAddress('0x' + log.topics[1].slice(26)) - const logSpender = getAddress('0x' + log.topics[2].slice(26)) - - return logOwner.toLowerCase() === owner.toLowerCase() && logSpender.toLowerCase() === spender.toLowerCase() - }) + // Find the last matching Approval event log (in case multiple occur in one tx) + let approvalLog: typeof txReceipt.logs[number] | undefined + for (let i = txReceipt.logs.length - 1; i >= 0; i--) { + const log = txReceipt.logs[i] + if (log.address.toLowerCase() !== tokenAddress.toLowerCase()) continue + if (!log.topics || log.topics.length < 3) continue + if (log.topics[0] !== APPROVAL_EVENT_TOPIC) continue + try { + const logOwner = getAddress('0x' + log.topics[1].slice(26)) + const logSpender = getAddress('0x' + log.topics[2].slice(26)) + if (logOwner.toLowerCase() === owner.toLowerCase() && logSpender.toLowerCase() === spender.toLowerCase()) { + approvalLog = log + break + } + } catch { + // skip invalid topics + continue + } + }Replace console.error with your app logger for consistency.
Also applies to: 25-27
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts (1)
244-259: Tighten the “returns receipt” test with an explicit assertionThe test name promises a return; currently it only checks state cleanup. Consider asserting the returned object shape to lock the callback’s API.
Example:
const resultValue = await result.current(mockAmount) expect(resultValue).toEqual( expect.objectContaining({ txResponse: expect.objectContaining({ transactionHash: '0xtxhash' }), approvedAmount: undefined, }), ) ```<!-- review_comment_end --> </blockquote></details> <details> <summary>apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)</summary><blockquote> `19-33`: **Guard against stale writes (older blockNumber) when updating optimistic allowances** Without an ordering check, an older tx can overwrite a newer allowance. Compare blockNumbers and skip stale updates. ```diff return useCallback( (params: SetOptimisticAllowanceParams) => { const key = getOptimisticAllowanceKey(params) // Set optimistic allowance immediately - setOptimisticAllowances((state) => ({ - ...state, - [key]: { - amount: params.amount, - blockNumber: params.blockNumber, - timestamp: Date.now(), - }, - })) + setOptimisticAllowances((state) => { + const prev = state[key] + if (prev && params.blockNumber < prev.blockNumber) { + // Ignore stale update + return state + } + return { + ...state, + [key]: { + amount: params.amount, + blockNumber: params.blockNumber, + timestamp: Date.now(), + }, + } + }) }, [setOptimisticAllowances], )apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsx (1)
26-30: Confirm intent: Dai-like permit case excluded from UI blockYou gate the toggle/allowance display for Required and EIP‑2612, but not Dai‑like. If Dai‑like should also show the same UI, include ApproveRequiredReason.DaiLikePermitRequired.
- const isApproveOrPartialPermitRequired = - isApproveRequired === ApproveRequiredReason.Required || - isApproveRequired === ApproveRequiredReason.Eip2612PermitRequired + const isApproveOrPartialPermitRequired = + isApproveRequired === ApproveRequiredReason.Required || + isApproveRequired === ApproveRequiredReason.Eip2612PermitRequired || + isApproveRequired === ApproveRequiredReason.DaiLikePermitRequiredMinor: rename
isApproveRequiredtoapproveReasonfor clarity (it holds an enum reason).Also applies to: 36-45
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.ts (1)
170-219: Add a test for the Safe wallet path (non-TradeApproveResult branch)Currently untested: when handleApprove returns SafeMultisigTransactionResponse, confirmSwap should still be called (else-branch of getIsTradeApproveResult).
@@ describe('approval flow with TradeApproveResult', () => { @@ }) + it('should call confirmSwap when handleApprove returns SafeMultisigTransactionResponse', async () => { + // Emulate a Safe response (any non-TradeApproveResult object) + const safeTx = { safeTxHash: '0xabc', isExecuted: false } as unknown as SafeMultisigTransactionResponse + mockHandleApprove.mockResolvedValue(safeTx) + + const { result } = renderHook(() => + useApproveAndSwap({ + amountToApprove: mockAmountToApprove, + confirmSwap: mockConfirmSwap, + ignorePermit: false, + useModals: true, + }), + ) + + await result.current() + + await waitFor(() => { + expect(mockHandleApprove).toHaveBeenCalledWith(MAX_APPROVE_AMOUNT) + expect(mockConfirmSwap).toHaveBeenCalled() + }) + })Also applies to: 335-359
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (1)
74-79: Tooltip text mismatches partial-approve behaviorIn the partial-approve branch, the tooltip still references “approve the default amount… once per token.” Adjust copy to reflect partial approvals.
- <Trans> - You must give the CoW Protocol smart contracts permission to use your{' '} - <TokenSymbol token={amountToApprove.currency} />. If you approve the default amount, you will only have to - do this once per token. - </Trans> + <Trans> + You must give the CoW Protocol smart contracts permission to use your{' '} + <TokenSymbol token={amountToApprove.currency} />. If you approve only the trade amount, you may need to + approve again for future trades. + </Trans>apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
66-84: Clarify variable naming to avoid confusion
approvedAmountholds SetOptimisticAllowanceParams, not a bigint. Rename for clarity and avoid shadowing meaning.- return response?.wait().then((txResponse) => { + return response?.wait().then((txResponse) => { // Set optimistic allowance immediately after transaction is mined // Extract the actual approved amount from transaction logs - const approvedAmount = processApprovalTransaction( + const optimisticParams = processApprovalTransaction( { currency, account, spender, chainId, }, txResponse, ) - if (approvedAmount) { - setOptimisticAllowance(approvedAmount) + if (optimisticParams) { + setOptimisticAllowance(optimisticParams) } - return { txResponse, approvedAmount: approvedAmount?.amount } + return { txResponse, approvedAmount: optimisticParams?.amount } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/cowswap-frontend/src/common/hooks/useTokenAllowance.ts(2 hunks)apps/cowswap-frontend/src/entities/optimisticAllowance/getOptimisticAllowanceKey.ts(1 hunks)apps/cowswap-frontend/src/entities/optimisticAllowance/optimisticAllowancesAtom.ts(1 hunks)apps/cowswap-frontend/src/entities/optimisticAllowance/types.ts(1 hunks)apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx(2 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.test.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts(4 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveToggle/TradeApproveToggle.tsx(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsx(2 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts(21 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts(2 hunks)apps/cowswap-frontend/src/modules/erc20Approve/pure/TradeAllowanceDisplay/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/pure/TradeAllowanceDisplay/styled.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/utils/getIsTradeApproveResult.ts(1 hunks)apps/cowswap-frontend/src/modules/ethFlow/containers/EthFlow/hooks/useEthFlowActions.ts(1 hunks)apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/hooks/usePendingTransactionsContext.ts(0 hunks)apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/services/finalizeEthereumTransaction.ts(1 hunks)apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/types.ts(0 hunks)apps/cowswap-frontend/src/modules/trade/hooks/useAmountsToSignFromQuote.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts(1 hunks)libs/common-utils/src/fractionUtils.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/types.ts
- apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/hooks/usePendingTransactionsContext.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
PR: cowprotocol/cowswap#6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/erc20Approve/pure/TradeAllowanceDisplay/index.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveToggle/TradeApproveToggle.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
PR: cowprotocol/cowswap#6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.
Applied to files:
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.tsapps/cowswap-frontend/src/modules/ethFlow/containers/EthFlow/hooks/useEthFlowActions.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsxapps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.test.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsxapps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.tsapps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.tsapps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
PR: cowprotocol/cowswap#6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
apps/cowswap-frontend/src/common/hooks/useTokenAllowance.ts
🧬 Code graph analysis (18)
apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (1)
useIsApprovalOrPermitRequired(22-48)
apps/cowswap-frontend/src/entities/optimisticAllowance/getOptimisticAllowanceKey.ts (1)
apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)
SetOptimisticAllowanceParams(7-14)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.ts (6)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts (1)
useApproveCurrency(10-30)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.ts (1)
useGeneratePermitInAdvanceToTrade(12-41)apps/cowswap-frontend/src/modules/erc20Approve/state/useIsPartialApproveSelectedByUser.ts (1)
useIsPartialApproveSelectedByUser(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
useApproveAndSwap(21-69)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
TradeApproveResult(24-24)apps/cowswap-frontend/src/modules/erc20Approve/constants/maxApproveAmount.ts (1)
MAX_APPROVE_AMOUNT(3-3)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)
SetOptimisticAllowanceParams(7-14)
apps/cowswap-frontend/src/modules/erc20Approve/utils/getIsTradeApproveResult.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
TradeApproveResult(24-24)
apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (2)
apps/cowswap-frontend/src/entities/optimisticAllowance/optimisticAllowancesAtom.ts (1)
optimisticAllowancesAtom(5-5)apps/cowswap-frontend/src/entities/optimisticAllowance/getOptimisticAllowanceKey.ts (1)
getOptimisticAllowanceKey(3-7)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsx (4)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (1)
useIsApprovalOrPermitRequired(22-48)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveToggle/TradeApproveToggle.tsx (1)
TradeApproveToggle(13-25)apps/cowswap-frontend/src/modules/erc20Approve/pure/TradeAllowanceDisplay/index.tsx (1)
TradeAllowanceDisplay(13-24)apps/cowswap-frontend/src/modules/erc20Approve/containers/ActiveOrdersWithAffectedPermit/ActiveOrdersWithAffectedPermit.tsx (1)
ActiveOrdersWithAffectedPermit(16-45)
apps/cowswap-frontend/src/entities/optimisticAllowance/optimisticAllowancesAtom.ts (1)
apps/cowswap-frontend/src/entities/optimisticAllowance/types.ts (1)
OptimisticAllowance(1-5)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (5)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx (1)
useGetAmountToSignApprove(21-45)libs/common-hooks/src/useFeatureFlags.ts (1)
useFeatureFlags(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveState.ts (1)
useApproveState(22-38)apps/cowswap-frontend/src/modules/trade/hooks/useDerivedTradeState.ts (1)
useDerivedTradeState(7-9)apps/cowswap-frontend/src/modules/permit/hooks/usePermitInfo.ts (1)
usePermitInfo(46-119)
apps/cowswap-frontend/src/modules/trade/hooks/useAmountsToSignFromQuote.ts (2)
apps/cowswap-frontend/src/modules/trade/hooks/useGetReceiveAmountInfo.ts (1)
useGetReceiveAmountInfo(13-60)libs/common-utils/src/fractionUtils.ts (1)
FractionUtils(11-186)
apps/cowswap-frontend/src/common/hooks/useTokenAllowance.ts (2)
apps/cowswap-frontend/src/entities/optimisticAllowance/optimisticAllowancesAtom.ts (1)
optimisticAllowancesAtom(5-5)apps/cowswap-frontend/src/entities/optimisticAllowance/getOptimisticAllowanceKey.ts (1)
getOptimisticAllowanceKey(3-7)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (3)
libs/wallet/src/api/hooks.ts (1)
useWalletInfo(24-26)apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)
useSetOptimisticAllowance(16-35)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
processApprovalTransaction(21-51)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.test.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
processApprovalTransaction(21-51)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
useApproveAndSwap(21-69)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (2)
TradeApproveResult(24-24)useTradeApproveCallback(30-117)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (1)
useIsApprovalOrPermitRequired(22-48)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (5)
apps/cowswap-frontend/src/modules/erc20Approve/state/useIsPartialApproveSelectedByUser.ts (1)
useIsPartialApproveSelectedByUser(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts (1)
useApproveCurrency(10-30)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.ts (1)
useGeneratePermitInAdvanceToTrade(12-41)apps/cowswap-frontend/src/modules/erc20Approve/constants/maxApproveAmount.ts (1)
MAX_APPROVE_AMOUNT(3-3)apps/cowswap-frontend/src/modules/erc20Approve/utils/getIsTradeApproveResult.ts (1)
getIsTradeApproveResult(6-13)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts (7)
libs/common-hooks/src/useFeatureFlags.ts (1)
useFeatureFlags(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCallback.tsx (1)
useApproveCallback(43-79)apps/cowswap-frontend/src/modules/erc20Approve/state/useUpdateTradeApproveState.ts (1)
useUpdateTradeApproveState(7-9)libs/wallet/src/api/hooks.ts (1)
useWalletInfo(24-26)apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)
useSetOptimisticAllowance(16-35)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
processApprovalTransaction(21-51)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
useTradeApproveCallback(30-117)
🪛 Gitleaks (8.28.0)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.test.ts
[high] 13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (18)
apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/services/finalizeEthereumTransaction.ts (1)
19-19: Ensure optimistic allowance update is dispatched for approvals
All references toupdateLastApproveTxBlockNumberhave been removed. Verify thatfinalizeEthereumTransactionnow dispatches the new optimistic allowance update when processing approval transactions.apps/cowswap-frontend/src/modules/ethFlow/containers/EthFlow/hooks/useEthFlowActions.ts (1)
76-79: Correctly sourcing tx hash from receiptUsing res?.txResponse?.transactionHash aligns with TradeApproveCallback’s returned shape and ethers types. Fixes prior hash lookup mismatch.
apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts (1)
44-44: LGTM! Correctly extracts the reason property.The change correctly adapts to the new return shape of
useIsApprovalOrPermitRequired, which now returns{ reason: ApproveRequiredReason; currentAllowance: Nullish<bigint> }instead of justApproveRequiredReason.libs/common-utils/src/fractionUtils.ts (1)
183-185: LGTM! Clean utility for percentage addition.The implementation correctly calculates the percentage increase using
amount.add(amount.multiply(percent)). The type signature properly preserves the generic currency type.apps/cowswap-frontend/src/entities/optimisticAllowance/types.ts (1)
1-5: LGTM! Well-defined interface for optimistic allowances.The interface provides a clean structure for tracking optimistic allowances with amount, block number, and timestamp. The timestamp field is particularly useful for cache invalidation of stale optimistic data.
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
90-90: LGTM! Tests correctly updated for new hook return shape.All test cases have been consistently updated to access the
reasonproperty from the hook's return value, aligning with the change to return{ reason, currentAllowance }instead of justApproveRequiredReason.Also applies to: 101-101, 112-112, 126-126, 138-138, 150-150, 160-160, 170-170, 178-178, 186-186, 196-196, 204-204, 216-216, 228-228, 240-240, 252-252, 266-266, 278-278, 290-290, 299-299, 303-303, 307-307, 311-311, 327-327, 342-342, 357-357, 369-369
apps/cowswap-frontend/src/entities/optimisticAllowance/optimisticAllowancesAtom.ts (1)
1-5: LGTM! Simple and effective global state store.The atom provides a clean centralized store for optimistic allowances, replacing the previous per-token block-number tracking approach.
apps/cowswap-frontend/src/modules/erc20Approve/pure/TradeAllowanceDisplay/index.tsx (1)
13-24: LGTM! Clean presentational component.The component correctly converts the bigint allowance to a CurrencyAmount for display. The implementation is straightforward and focused on rendering the current allowance information.
apps/cowswap-frontend/src/modules/trade/hooks/useAmountsToSignFromQuote.ts (3)
9-9: LGTM! Well-defined threshold constant.The 1% threshold is clearly defined and aligns with the PR objective to handle price/gas fluctuations for buy orders.
37-37: Correct dependency array updates.The addition of
isSellandafterSlippageto the dependency array is correct, as both are used in the memoized computation.
30-36: Confirm 1% buffer covers all buy orders
The 1% threshold is correctly applied to all buy orders (both quote-based and non-quote-based). No additional buy-order code paths require adjustment.apps/cowswap-frontend/src/modules/erc20Approve/utils/getIsTradeApproveResult.ts (1)
6-13: Confirm discriminator property
Verify thatSafeMultisigTransactionResponsedoes not include atxResponsefield—this type guard assumes onlyTradeApproveResulthastxResponse, so it’s sound only if no safe response defines that property.apps/cowswap-frontend/src/modules/erc20Approve/pure/TradeAllowanceDisplay/styled.ts (1)
5-30: LGTM – styling is clear and consistent with UI tokensNo issues found; components are well-scoped and reusable.
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts (1)
7-14: Return type broadened to include Safe transactions—confirm upstream hook alignsThe union makes sense with Safe flows. Please confirm useTradeApproveCallback actually returns SafeMultisigTransactionResponse in Safe contexts, or adjust the union if not.
Based on learnings
Also applies to: 26-29
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (1)
34-38: Verify the 'unsupported' sentinel aligns with usePermitInfo/PermitType
const isPermitSupported = type !== 'unsupported'assumes a specific sentinel. Confirm usePermitInfo yields 'unsupported' for non-permittable tokens; otherwise this check may misclassify support.apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.test.ts (2)
1-97: Solid coverage of approval log extraction and failure pathsTests exercise success, zero-amount (revoke), mixed logs, and failure statuses. Looks good.
13-13: Gitleaks false positiveThe hex at Line 13 is an Ethereum address (USDC), not a secret/API key. Safe to ignore.
Based on static analysis hints
apps/cowswap-frontend/src/common/hooks/useTokenAllowance.ts (1)
40-58: LGTM: Optimistic allowance overlay with safe SWR keying
- Correct composite keying and TTL cleanup.
- Avoiding erc20Contract in SWR key prevents extra RPC churn.
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts
Outdated
Show resolved
Hide resolved
…nto fix/partial-approve-buy-order
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
49-374: Consider adding test coverage for the newcurrentAllowanceproperty.The hook now exposes
currentAllowancein its return value, but no tests verify this property. Consider adding assertions to ensurecurrentAllowanceis correctly propagated fromuseApproveState.Example test additions:
it('should return currentAllowance from useApproveState', () => { const expectedAllowance = BigInt(1000000000000000000) mockUseApproveState.mockReturnValue({ state: ApprovalState.APPROVED, currentAllowance: expectedAllowance, }) const { result } = renderHook(() => useIsApprovalOrPermitRequired()) expect(result.current.currentAllowance).toBe(expectedAllowance) }) it('should handle null currentAllowance', () => { mockUseApproveState.mockReturnValue({ state: ApprovalState.UNKNOWN, currentAllowance: null, }) const { result } = renderHook(() => useIsApprovalOrPermitRequired()) expect(result.current.currentAllowance).toBeNull() })apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (2)
51-56: Avoid unhandled promise rejections on LegacyApproveButton clickWrap the async call to prevent unhandledrejection in the browser.
- onClick={() => handleApprove(MAX_APPROVE_AMOUNT)} + onClick={() => + handleApprove(MAX_APPROVE_AMOUNT).catch(() => { + /* handled via modal state elsewhere */ + }) + }
64-69: Avoid unhandled promise rejections on partial-approve button clickSame here—wrap the async callback to swallow errors already surfaced via state.
- onClick={approveWithPreventedDoubleExecution} + onClick={() => + approveWithPreventedDoubleExecution().catch(() => { + /* handled via modal state elsewhere */ + }) + }apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useHandleApprovalError.ts (1)
15-24: Guard console logging in production; include token symbolReduce noisy/PII logs in prod and add context for debugging.
- console.error('Error setting the allowance for token', error) + if (process.env.NODE_ENV !== 'production') { + // Include token symbol for context in dev logs + console.error('Allowance error', { symbol, error }) + }apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts (1)
138-167: Assert approveUtils receives correct params (chainId/owner/spender/token)Strengthen this success-path test by verifying the payload into processApprovalTransaction.
await waitFor(() => { - expect(mockProcessApprovalTransaction).toHaveBeenCalled() + expect(mockProcessApprovalTransaction).toHaveBeenCalledWith( + expect.objectContaining({ + chainId: mockChainId, + account: mockAccount, + spender: mockSpenderAddress, + currency: mockToken, + }), + expect.any(Object) + ) expect(mockSetOptimisticAllowance).toHaveBeenCalledWith({apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
59-75: Always surface “insufficient approved amount” even when onApproveConfirm is absentCurrently the error state is only set when onApproveConfirm exists. Surface the error regardless to avoid silent failures.
- if (tx && onApproveConfirm) { - if (getIsTradeApproveResult(tx)) { + if (!tx) return + + if (getIsTradeApproveResult(tx)) { const approvedAmount = tx.approvedAmount const isApprovedAmountSufficient = Boolean(approvedAmount && approvedAmount >= amountToApproveBig) - if (isApprovedAmountSufficient) { - const hash = - (tx.txResponse as TransactionReceipt).transactionHash || (tx.txResponse as TransactionResponse).hash - - onApproveConfirm(hash) - } else { - updateTradeApproveState({ error: 'Approved amount is not sufficient!' }) - } - } else { - onApproveConfirm(tx.transactionHash) - } - } + if (!isApprovedAmountSufficient) { + updateTradeApproveState({ error: 'Approved amount is not sufficient!' }) + return + } + const hash = + (tx.txResponse as TransactionReceipt).transactionHash || (tx.txResponse as TransactionResponse).hash + onApproveConfirm?.(hash) + } else { + onApproveConfirm?.(tx.transactionHash) + }apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.ts (1)
305-369: Add coverage for Safe/multisig (non-TradeApproveResult) pathInclude a case where handleApprove resolves to a SafeMultisigTransactionResponse-like object (no txResponse), and assert onApproveConfirm is called with transactionHash.
describe('no transaction or confirmSwap', () => { + it('should call onApproveConfirm with Safe tx hash for multisig submissions', async () => { + const safeTx = { transactionHash: '0xsafehash123' } as any + mockHandleApprove.mockResolvedValue(safeTx) + + const { result } = renderHook(() => + useApproveAndSwap({ + amountToApprove: mockAmountToApprove, + onApproveConfirm: mockOnApproveConfirm, + ignorePermit: false, + useModals: true, + }), + ) + + await result.current() + + await waitFor(() => { + expect(mockOnApproveConfirm).toHaveBeenCalledWith('0xsafehash123') + }) + })apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
114-120: Consider clarifying the comment to cover both conditions.The comment on line 114 only mentions the feature flag case, but the condition on line 115 also handles the case where
responseis undefined (which can happen ifapproveCallbackreturns undefined). While the logic is correct, the comment could be more explicit.Consider updating the comment:
- // if ff is disabled - use old flow, hide modal when tx is sent + // If approval failed/cancelled (no response) or feature flag disabled - use old flow, hide modal if (!response || !isPartialApproveEnabled) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx(2 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useHandleApprovalError.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts(3 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts(20 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
PR: cowprotocol/cowswap#6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.
Applied to files:
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useHandleApprovalError.tsapps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.tsapps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
PR: cowprotocol/cowswap#6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts
🧬 Code graph analysis (7)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (1)
useIsApprovalOrPermitRequired(22-48)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useHandleApprovalError.ts (4)
apps/cowswap-frontend/src/modules/erc20Approve/state/useUpdateApproveProgressModalState.ts (1)
useUpdateApproveProgressModalState(5-9)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useApprovalAnalytics.ts (1)
useApprovalAnalytics(7-21)libs/common-utils/src/misc.ts (1)
isRejectRequestProviderError(168-187)libs/common-utils/src/errorToString.ts (1)
errorToString(3-7)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.test.ts (7)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts (1)
useApproveCurrency(10-30)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.ts (1)
useGeneratePermitInAdvanceToTrade(12-51)apps/cowswap-frontend/src/modules/erc20Approve/state/useIsPartialApproveSelectedByUser.ts (1)
useIsPartialApproveSelectedByUser(5-7)apps/cowswap-frontend/src/modules/erc20Approve/state/useUpdateApproveProgressModalState.ts (1)
useUpdateApproveProgressModalState(5-9)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
useApproveAndSwap(22-84)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
TradeApproveResult(68-68)apps/cowswap-frontend/src/modules/erc20Approve/constants/maxApproveAmount.ts (1)
MAX_APPROVE_AMOUNT(3-3)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
useApproveAndSwap(22-84)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (6)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
processApprovalTransaction(21-51)libs/wallet/src/api/hooks.ts (1)
useWalletInfo(24-26)apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)
useSetOptimisticAllowance(16-35)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCallback.tsx (1)
useApproveCallback(43-79)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useApprovalAnalytics.ts (1)
useApprovalAnalytics(7-21)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useHandleApprovalError.ts (1)
useHandleApprovalError(9-28)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (6)
apps/cowswap-frontend/src/modules/erc20Approve/state/useIsPartialApproveSelectedByUser.ts (1)
useIsPartialApproveSelectedByUser(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.ts (1)
useApproveCurrency(10-30)apps/cowswap-frontend/src/modules/erc20Approve/state/useUpdateApproveProgressModalState.ts (1)
useUpdateApproveProgressModalState(5-9)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.ts (1)
useGeneratePermitInAdvanceToTrade(12-51)apps/cowswap-frontend/src/modules/erc20Approve/constants/maxApproveAmount.ts (1)
MAX_APPROVE_AMOUNT(3-3)apps/cowswap-frontend/src/modules/erc20Approve/utils/getIsTradeApproveResult.ts (1)
getIsTradeApproveResult(6-13)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.test.ts (8)
libs/common-hooks/src/useFeatureFlags.ts (1)
useFeatureFlags(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCallback.tsx (1)
useApproveCallback(43-79)apps/cowswap-frontend/src/modules/erc20Approve/state/useUpdateApproveProgressModalState.ts (1)
useUpdateApproveProgressModalState(5-9)apps/cowswap-frontend/src/modules/erc20Approve/state/useResetApproveProgressModalState.ts (1)
useResetApproveProgressModalState(5-7)libs/wallet/src/api/hooks.ts (1)
useWalletInfo(24-26)apps/cowswap-frontend/src/entities/optimisticAllowance/useSetOptimisticAllowance.ts (1)
useSetOptimisticAllowance(16-35)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/approveUtils.ts (1)
processApprovalTransaction(21-51)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (1)
useTradeApproveCallback(88-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (6)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
92-371: LGTM! Test assertions updated consistently.All test assertions have been correctly updated to access the nested
reasonproperty following the hook's API change from returningApproveRequiredReasondirectly to returning{ reason, currentAllowance }.apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
72-74: Safe/multisig: confirm navigation before execution? Verify intended UXFor non-TradeApproveResult (Safe) you invoke onApproveConfirm with the submission hash immediately. Confirm this won’t navigate to confirm/swap screens before the Safe tx is executed/mined. If execution confirmation is required, consider deferring the callback until executed or surfacing a “pending in Safe” state. Based on learnings
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts (4)
1-16: LGTM!The new imports are correctly added to support the optimistic allowance flow and refactored error handling.
63-66: LGTM!The default parameters are well-defined and used correctly throughout the implementation (line 103).
148-162: LGTM!The dependencies array is complete and correctly includes all external values used within the callback. The type assertion to
TradeApproveCallbackis necessary to preserve the overloaded signature.
37-39: No issues found – this is intentional defensive programming.The early return when
chainIdis undefined is correct. After successfully waiting for transaction confirmation (line 35), ifchainIdbecomes unavailable, the function gracefully returns withapprovedAmount: undefinedinstead of attempting to process the approval details. This mirrors the same behavior whenwaitForTxConfirmationis false (line 134). While this scenario is unusual—sincechainIdshould remain available if the transaction was initiated—it represents proper handling of a rare wallet or network state change. The code doesn't mask an error; it simply skips the optimistic allowance optimization when the required context is missing.
...ap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts
Show resolved
Hide resolved
|
@elena-zh could you pls check again? |
… fix/partial-approve-buy-order # Conflicts: # apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsx # apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts # apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts # apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
86-97: Consider adding test coverage for the newcurrentAllowanceproperty.All test assertions verify only the
reasonproperty of the returned object. Since the hook now also returnscurrentAllowance(which is part of the API change described in the PR objectives), consider adding at least one test case that verifies this property is present and has the expected value.Example test addition:
it('should return currentAllowance along with reason', () => { const expectedAllowance = BigInt(1000000000000000000) mockUseApproveState.mockReturnValue({ state: ApprovalState.APPROVED, currentAllowance: expectedAllowance, }) const { result } = renderHook(() => useIsApprovalOrPermitRequired({ isBundlingSupportedOrEnabledForContext: null }), ) expect(result.current.reason).toBe(ApproveRequiredReason.NotRequired) expect(result.current.currentAllowance).toBe(expectedAllowance) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsx(2 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts(32 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveWithAffectedOrderList/TradeApproveWithAffectedOrderList.tsx
- apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
PR: cowprotocol/cowswap#6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.
Applied to files:
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.tsapps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts
🧬 Code graph analysis (2)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (5)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx (1)
useGetAmountToSignApprove(21-45)libs/common-hooks/src/useFeatureFlags.ts (1)
useFeatureFlags(5-7)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveState.ts (1)
useApproveState(22-38)apps/cowswap-frontend/src/modules/trade/hooks/useDerivedTradeState.ts (1)
useDerivedTradeState(7-9)apps/cowswap-frontend/src/modules/permit/hooks/usePermitInfo.ts (1)
usePermitInfo(46-119)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.test.ts (1)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (1)
useIsApprovalOrPermitRequired(31-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (5)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useIsApprovalOrPermitRequired.ts (5)
1-1: LGTM: React import added for memoization.The
useMemoimport is correctly added to support the new memoized return value.
31-34: LGTM: API enriched to include current allowance.The return type change from a bare
ApproveRequiredReasonto an object containing bothreasonandcurrentAllowancealigns with the PR objective to display current allowance in the UI. This is a breaking change, but all consumers have been updated accordingly.
37-37: LGTM: Current allowance extracted from approval state.The
currentAllowanceis correctly extracted fromuseApproveStatefor inclusion in the hook's return value.
41-61: LGTM: Reason computation logic preserved.The approval/permit requirement logic has been correctly wrapped in an IIFE to compute the
reasonvalue. All decision branches are preserved and the logic remains unchanged from the previous implementation.
63-63: LGTM: Return value properly memoized.The
useMemohook correctly memoizes the return object based onreasonandcurrentAllowance. Since both dependencies are primitive or primitive-like values (enum and bigint), the memoization will work effectively to prevent unnecessary re-renders in consuming components.
|
Hey @shoom3301 , still there are some issues:
It should be only Swap for EOAs
Could you please fix it? |
|
@elena-zh fixed both, thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (1)
65-95: Fix label/tooltip gating: show “Swap” for ETH and already‑approved tokens.Current logic keys off permit cache only, which causes:
- ETH flow for EOAs to show “Approve and Swap” (should be “Swap”).
- Already‑approved permittable tokens to still show “Approve and Swap”.
Gate on on‑chain approval need instead: native tokens never need approval; approved tokens don’t need another approval. Tooltip should also appear only when an on‑chain approval is required.
import { MAX_APPROVE_AMOUNT } from '../../constants' -import { useApprovalStateForSpender, useApproveCurrency } from '../../hooks' +import { useApprovalStateForSpender, useApproveCurrency } from '../../hooks' import { useApproveAndSwap } from '../../hooks/useApproveAndSwap' import { LegacyApproveButton } from '../../pure/LegacyApproveButton' import { ApprovalState } from '../../types' +import { getIsNativeToken } from '@cowprotocol/common-utils' @@ const isPending = approvalState === ApprovalState.PENDING - const noCachedPermit = !cachedPermitLoading && !cachedPermit - - const label = - props.label || (noCachedPermit ? (isCurrentTradeBridging ? 'Approve, Swap & Bridge' : 'Approve and Swap') : 'Swap') + // On‑chain approval is needed only for non‑native tokens that are not already approved + const isNative = getIsNativeToken(amountToApprove.currency) + const needsOnchainApproval = !isNative && approvalState !== ApprovalState.APPROVED + const label = + props.label || + (needsOnchainApproval + ? isCurrentTradeBridging + ? 'Approve, Swap & Bridge' + : 'Approve and Swap' + : isCurrentTradeBridging + ? 'Swap & Bridge' + : 'Swap') @@ - {noCachedPermit ? ( + {needsOnchainApproval ? ( <HoverTooltip wrapInContainer content={ <Trans> You must give the CoW Protocol smart contracts permission to use your{' '} <TokenSymbol token={amountToApprove.currency} />. If you approve the default amount, you will only have to do this once per token. </Trans> } > {isPending ? <styledEl.StyledLoader /> : <styledEl.StyledAlert size={24} />} </HoverTooltip> ) : null}Please also remove any now‑unused variables (e.g., noCachedPermit) if present after the change.
🧹 Nitpick comments (3)
apps/cowswap-frontend/src/modules/swap/containers/TradeButtons/index.tsx (1)
61-61: LGTM: Bridging-aware button text improves UX clarity.The conditional text logic correctly displays "Swap and Bridge" for cross-chain operations and "Swap" otherwise, making the operation type explicit to users. This aligns with the broader approval and bridging flow improvements in the PR.
Optional: Consider i18n support for user-facing text.
The hardcoded strings "Swap and Bridge" and "Swap" could benefit from internationalization. However, this is consistent with the existing pattern in the codebase, so it can be addressed separately if/when i18n is implemented more broadly.
apps/cowswap-frontend/src/modules/permit/hooks/useHasCachedPermit.ts (1)
9-18: Skip fetching for native currency and zero amounts to reduce noise.Guard the SWR key so we don’t query cache for ETH/native or 0 approvals; it also prevents misleading consumers that equate “no cached permit” with “needs approval”.
import { getWrappedToken } from '@cowprotocol/common-utils' import { PermitHookData } from '@cowprotocol/permit-utils' import { Currency, CurrencyAmount } from '@uniswap/sdk-core' import useSWR, { SWRResponse } from 'swr' import { useGetCachedPermit } from './useGetCachedPermit' export function useHasCachedPermit(amountToApprove: CurrencyAmount<Currency>): SWRResponse<PermitHookData | undefined> { const getCachedPermit = useGetCachedPermit() - const token = getWrappedToken(amountToApprove.currency) + const token = getWrappedToken(amountToApprove.currency) const tokenAddress = token.address const amount = BigInt(amountToApprove.quotient.toString()) - return useSWR([tokenAddress, amount, token.chainId, 'useHasCachedPermit'], ([tokenAddress, amount]) => { - return getCachedPermit(tokenAddress, amount) - }) + // Skip fetching for native currency (no permit/approve) or zero amounts + const isNative = amountToApprove.currency.isNative === true + const key = isNative || amount === 0n ? null : [tokenAddress, amount, token.chainId, 'useHasCachedPermit'] as const + + return useSWR(key, ([tokenAddress, amount]) => getCachedPermit(tokenAddress, amount)) }apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (1)
71-77: Optional: disable while permit state is loading to prevent label flicker.When cachedPermitLoading is true, consider disabling the button or freezing the label until resolved to avoid transient “Approve…” text.
- <ButtonWrapper - disabled={isPending || isDisabled} + <ButtonWrapper + disabled={isPending || isDisabled || cachedPermitLoading} buttonSize={buttonSize} onClick={approveWithPreventedDoubleExecution} altDisabledStyle={isPending} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/cowswap-frontend/src/modules/account/containers/OrderPartialApprove/OrderPartialApprove.tsx(0 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx(5 hunks)apps/cowswap-frontend/src/modules/permit/hooks/useGetCachedPermit.ts(1 hunks)apps/cowswap-frontend/src/modules/permit/hooks/useHasCachedPermit.ts(1 hunks)apps/cowswap-frontend/src/modules/permit/index.ts(1 hunks)apps/cowswap-frontend/src/modules/permit/state/permitCacheAtom.ts(1 hunks)apps/cowswap-frontend/src/modules/swap/containers/TradeButtons/index.tsx(2 hunks)apps/cowswap-frontend/src/modules/swap/hooks/useGetConfirmButtonLabel.ts(0 hunks)apps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormButtons/tradeButtonsMap.tsx(0 hunks)
💤 Files with no reviewable changes (3)
- apps/cowswap-frontend/src/modules/account/containers/OrderPartialApprove/OrderPartialApprove.tsx
- apps/cowswap-frontend/src/modules/swap/hooks/useGetConfirmButtonLabel.ts
- apps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormButtons/tradeButtonsMap.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/cowswap-frontend/src/modules/permit/hooks/useGetCachedPermit.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
PR: cowprotocol/cowswap#6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/swap/containers/TradeButtons/index.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
PR: cowprotocol/cowswap#6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.
Applied to files:
apps/cowswap-frontend/src/modules/permit/hooks/useHasCachedPermit.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
PR: cowprotocol/cowswap#6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
apps/cowswap-frontend/src/modules/permit/state/permitCacheAtom.ts
🧬 Code graph analysis (4)
apps/cowswap-frontend/src/modules/swap/containers/TradeButtons/index.tsx (1)
apps/cowswap-frontend/src/modules/trade/hooks/useIsCurrentTradeBridging.ts (1)
useIsCurrentTradeBridging(5-9)
apps/cowswap-frontend/src/modules/permit/hooks/useHasCachedPermit.ts (2)
apps/cowswap-frontend/src/modules/permit/hooks/useGetCachedPermit.ts (1)
useGetCachedPermit(13-48)apps/cowswap-frontend/src/modules/permit/index.ts (1)
useGetCachedPermit(7-7)
apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveButton/TradeApproveButton.tsx (7)
apps/cowswap-frontend/src/modules/trade/hooks/useIsCurrentTradeBridging.ts (1)
useIsCurrentTradeBridging(5-9)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproval.ts (1)
useApprovalStateForSpender(18-32)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveAndSwap.ts (1)
useApproveAndSwap(22-84)libs/common-hooks/src/usePreventDoubleExecution.ts (1)
usePreventDoubleExecution(3-17)apps/cowswap-frontend/src/modules/permit/hooks/useHasCachedPermit.ts (1)
useHasCachedPermit(9-18)libs/ui/src/pure/Tooltip/index.tsx (1)
HoverTooltip(61-177)libs/ui/src/pure/TokenSymbol/index.tsx (1)
TokenSymbol(33-44)
apps/cowswap-frontend/src/modules/permit/state/permitCacheAtom.ts (1)
apps/cowswap-frontend/src/modules/permit/types.ts (1)
PermitCache(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (4)
apps/cowswap-frontend/src/modules/swap/containers/TradeButtons/index.tsx (2)
7-12: LGTM: Import addition is appropriate.The
useIsCurrentTradeBridgingimport is correctly added to support the bridging-aware UI text update.
57-57: LGTM: Hook usage is correct.The
useIsCurrentTradeBridginghook is properly invoked to detect cross-chain operations.apps/cowswap-frontend/src/modules/permit/state/permitCacheAtom.ts (1)
17-19: Correct use of unstable_getOnInit; please confirm SSR safety.Adding unstable_getOnInit avoids hydration flicker and matches our Jotai usage elsewhere. Confirm these atoms aren’t read during SSR or that storage access is safely guarded to avoid “localStorage is not defined” in server builds.
Based on learnings.
Also applies to: 26-28
apps/cowswap-frontend/src/modules/permit/index.ts (1)
6-6: Public re‑export looks good.No conflicts detected; aligns the permit API surface.
3 participants
Summary
Fixes https://www.notion.so/cownation/Handle-edge-case-for-Buy-orders-with-EOAs-2578da5f04ca8083a97de4eb393b4ac5
https://www.notion.so/cownation/Wrong-navigation-when-approval-TX-fails-2878da5f04ca801982fede0f21a79bc7
https://www.notion.so/cownation/Don-t-navigate-to-confirm-moda-when-approved-less-than-needed-2878da5f04ca80028f02e0ca16445316
demo-appr.mov
To Test
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements
Tests