fix(approve): support partial approvals in Safe #6397
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMigrates partial approval state management from hook-based to Jotai atom approach. Introduces Changes
Sequence DiagramsequenceDiagram
participant SwapUpdaters
participant Erc20ApproveWidget
participant Jotai Atom Store
participant useGetAmountToSignApprove
participant SafeBundleContext
SwapUpdaters->>Erc20ApproveWidget: render with isPartialApprovalEnabled prop
Erc20ApproveWidget->>Jotai Atom Store: useSetAtom updates isPartialApproveEnabledAtom
rect rgb(230, 245, 255)
Note over useGetAmountToSignApprove,Jotai Atom Store: Atom-driven flow
useGetAmountToSignApprove->>Jotai Atom Store: useAtomValue reads isPartialApproveEnabledAtom
Jotai Atom Store-->>useGetAmountToSignApprove: returns boolean value
useGetAmountToSignApprove->>useGetAmountToSignApprove: compute amountToApprove
end
useGetAmountToSignApprove-->>SafeBundleContext: amountToApprove provided
SafeBundleContext->>SafeBundleContext: memoize with amountToApprove included
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/erc20Approve/containers/Erc20ApproveWidget/index.tsx (1)
10-18: LGTM! State synchronization component works correctly.The
Erc20ApproveWidgetcomponent effectively synchronizes theisPartialApprovalEnabledprop with the global atom state. The pattern of renderingnullwhile performing side effects viauseEffectis appropriate for this use case.Minor optimization: The
setIsPartialApproveEnabledfunction returned byuseSetAtomis stable across renders, so it's not strictly necessary in the dependency array. However, including it follows React's exhaustive-deps rule and doesn't cause any issues.If you prefer a minimal dependency array, you can apply this diff:
useEffect(() => { setIsPartialApproveEnabled(isPartialApprovalEnabled) - }, [setIsPartialApproveEnabled, isPartialApprovalEnabled]) + }, [isPartialApprovalEnabled])apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx (1)
34-34: Clarify the triple-condition logic for partial approvals.The partial approval logic requires three separate conditions to all be true:
isPartialApproveEnabled(feature flag)isPartialApprovalSelectedByUser(user selection)isPartialApprovalEnabledInSettings(from atom)The distinction between conditions 2 and 3 is not immediately clear. Please consider adding a comment explaining what each condition represents and why all three are necessary, or verify if this complexity can be simplified.
Consider adding a JSDoc comment above line 22 or inline comment above line 34 explaining the three-gate logic:
+ // Partial approval requires: + // 1. Feature flag enabled globally + // 2. User selected partial approval for this transaction + // 3. Partial approval enabled in swap settings if (isPartialApproveEnabled && isPartialApprovalSelectedByUser && isPartialApprovalEnabledInSettings) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/cowswap-frontend/src/modules/erc20Approve/containers/Erc20ApproveWidget/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/containers/index.ts(1 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.test.ts(20 hunks)apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx(3 hunks)apps/cowswap-frontend/src/modules/erc20Approve/state/isPartialApproveEnabledAtom.ts(1 hunks)apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/swap/hooks/useSwapFlowContext.ts(1 hunks)apps/cowswap-frontend/src/modules/swap/updaters/index.tsx(3 hunks)apps/cowswap-frontend/src/modules/tradeFlow/hooks/useHandleSwap.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts(2 hunks)apps/cowswap-frontend/src/modules/tradeFlow/types/TradeFlowContext.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.tsapps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsxapps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.tsapps/cowswap-frontend/src/modules/swap/updaters/index.tsx
📚 Learning: 2025-04-02T09:58:29.374Z
Learnt from: shoom3301
PR: cowprotocol/cowswap#5549
File: apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts:152-152
Timestamp: 2025-04-02T09:58:29.374Z
Learning: In the `safeBundleEthFlow` function, `account` is guaranteed to be truthy based on the type system (`PostOrderParams` defines it as a required string) and the context in which the function is called, so additional runtime checks are unnecessary.
Applied to files:
apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.tsapps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.ts
📚 Learning: 2025-02-20T15:59:33.749Z
Learnt from: shoom3301
PR: cowprotocol/cowswap#5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
Applied to files:
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx
📚 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/SwapWidget/index.tsx
🧬 Code graph analysis (7)
apps/cowswap-frontend/src/modules/erc20Approve/containers/Erc20ApproveWidget/index.tsx (1)
apps/cowswap-frontend/src/modules/erc20Approve/state/isPartialApproveEnabledAtom.ts (1)
isPartialApproveEnabledAtom(3-3)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx (1)
apps/cowswap-frontend/src/modules/erc20Approve/state/isPartialApproveEnabledAtom.ts (1)
isPartialApproveEnabledAtom(3-3)
apps/cowswap-frontend/src/modules/tradeFlow/hooks/useHandleSwap.ts (2)
apps/cowswap-frontend/src/modules/tradeFlow/hooks/useTradeFlowContext.ts (1)
TradeFlowParams(34-36)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/types.ts (1)
TradeWidgetActions(8-14)
apps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.ts (5)
apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx (1)
useGetAmountToSignApprove(22-46)libs/wallet/src/api/hooks/useSendBatchTransactions.ts (1)
useSendBatchTransactions(13-43)apps/cowswap-frontend/src/common/hooks/useContract.ts (2)
useWethContract(102-104)useTokenContract(98-100)apps/cowswap-frontend/src/common/hooks/useNeedsApproval.ts (1)
useNeedsApproval(23-37)libs/common-utils/src/getCurrencyAddress.ts (1)
getCurrencyAddress(6-8)
apps/cowswap-frontend/src/modules/swap/hooks/useSwapFlowContext.ts (3)
apps/cowswap-frontend/src/modules/tradeFlow/types/TradeFlowContext.ts (1)
TradeFlowContext(26-53)apps/cowswap-frontend/src/modules/swap/hooks/useSwapSettings.ts (1)
useSwapDeadlineState(14-22)apps/cowswap-frontend/src/modules/tradeFlow/hooks/useTradeFlowContext.ts (1)
useTradeFlowContext(41-235)
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx (1)
apps/cowswap-frontend/src/modules/tradeFlow/hooks/useHandleSwap.ts (1)
useHandleSwap(20-104)
apps/cowswap-frontend/src/modules/swap/updaters/index.tsx (2)
apps/cowswap-frontend/src/modules/swap/hooks/useSwapSettings.ts (2)
useSwapDeadlineState(14-22)useSwapSettings(10-12)apps/cowswap-frontend/src/modules/erc20Approve/containers/Erc20ApproveWidget/index.tsx (1)
Erc20ApproveWidget(10-18)
⏰ 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 (16)
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx (1)
89-89: Code change is correct; no performance issue exists.The concern about removing
useSafeMemoObjectis not valid.useTradeFlowContextdestructures thedeadlinevalue from the params object and only uses that scalar value. The SWR hook's dependency array includesvalidTo(derived from the deadline value), not the params object reference, so the new object reference on each render has no effect. The deadline value itself is stable since it comes from state.apps/cowswap-frontend/src/modules/erc20Approve/containers/index.ts (1)
10-10: LGTM!The export follows the established pattern in this file and properly exposes the new
Erc20ApproveWidgetcomponent.apps/cowswap-frontend/src/modules/erc20Approve/state/isPartialApproveEnabledAtom.ts (1)
1-3: LGTM!The atom definition is straightforward and follows Jotai best practices. The default value of
falseis appropriate for the partial approval feature.apps/cowswap-frontend/src/modules/tradeFlow/types/TradeFlowContext.ts (1)
61-61: LGTM! Interface change makes approval amount explicit.Replacing the optional boolean flag
isPartialApproveEnabled?with a requiredamountToApprove: CurrencyAmount<Currency>is a good design improvement. It makes the approval amount explicit and type-safe, eliminating ambiguity about what amount should be approved.This is a breaking change to the
SafeBundleFlowContextinterface, but the refactor appears comprehensive across the codebase.apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts (2)
50-50: LGTM! Consistent with the refactored approval flow.The destructuring of
amountToApprovefromsafeBundleContextaligns with the interface changes and mirrors the pattern used insafeBundleApprovalFlow.ts.
85-85: LGTM! Direct usage ofamountToApprovefrom context.The change to use
BigInt(amountToApprove.quotient.toString())directly is correct and consistent with the refactored flow where the amount is pre-computed and provided via context.apps/cowswap-frontend/src/modules/tradeFlow/hooks/useHandleSwap.ts (1)
20-23: LGTM! Explicit return type improves type safety.Adding an explicit return type to
useHandleSwapis a good practice that enhances type safety and makes the hook's contract clearer to consumers.apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.test.ts (3)
1-1: LGTM! Test setup correctly migrated to Jotai atom mocking.The test file has been properly updated to mock
useAtomValueinstead of the previous hook-based partial approval state. The mocking approach aligns with the new atom-based state management.Also applies to: 16-19, 37-37
60-61: LGTM! Default mock values are appropriate.The default mock configuration with
mockUseAtomValue.mockReturnValue(true)inbeforeEachsets up a reasonable baseline for most test cases.
85-90: LGTM! Test cases comprehensively cover partial approval scenarios.The test cases properly verify behavior across different combinations of:
isPartialApproveEnabled(feature flag)isPartialApprovalSelectedByUser(user selection)isPartialApprovalEnabledInSettings(settings from atom)All scenarios correctly mock
useAtomValueto simulate different settings states.Also applies to: 98-103, 109-114, 120-125
apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.ts (1)
45-62: UpstreamamountToApprovecomputation is correct—no issues found.Verification confirms that
useGetAmountToSignApprove()(called inuseSafeBundleFlowContextline 18) correctly computes the amount based on partial approval settings. The hook returns: zero if approval isn't needed; the partial amount if partial approval is enabled, user-selected, and configured in settings; otherwise the max/unlimited amount. All feature flags, user selections, and settings are properly checked and memoized.apps/cowswap-frontend/src/modules/tradeFlow/hooks/useSafeBundleFlowContext.ts (2)
7-7: LGTM: Clean migration from boolean flag to concrete amount.The migration from
isPartialApproveEnabledtoamountToApproveprovides consumers with the actual approval amount rather than just a boolean flag, enabling more precise decision-making in the approval flow.Also applies to: 18-18, 42-42
31-44: Verify the null guard onamountToApprove.The null guard on line 32 prevents the context from being created when
amountToApproveis null. However,safeBundleEthFlowonly usesamountToApprovewhenneedsApprovalis true (line 81 of safeBundleEthFlow), meaning this flow can execute withneedsApproval=falseand a nullamountToApprove. The current null guard unnecessarily blocks context creation in this scenario.Consider relaxing the null guard to only require
amountToApprovewhenneedsApprovalis true, or make its requirement conditional based on which flow will execute.apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGetAmountToSignApprove.tsx (1)
1-1: LGTM: Clean atom migration.The migration from hook-based to atom-based state reading for
isPartialApprovalEnabledInSettingsis straightforward and maintains the existing logic flow.Also applies to: 13-13, 27-27
apps/cowswap-frontend/src/modules/swap/hooks/useSwapFlowContext.ts (1)
1-9: LGTM: Excellent simplification.The removal of the
useSafeMemoObjectwrapper and direct pass-through touseTradeFlowContextimproves maintainability. Thedeadlinevalue comes fromuseSwapDeadlineState, which provides a stable reference that only changes when the user updates the setting, so the plain object at line 8 is appropriate.apps/cowswap-frontend/src/modules/swap/updaters/index.tsx (1)
6-6: LGTM: Clean integration of approval state synchronization.The
Erc20ApproveWidgetintegration properly establishes the uni-directional data flow from swap settings to the global atom. The placement inSwapUpdatersis appropriate for this side-effect component.Also applies to: 17-17, 24-24, 39-39
There was a problem hiding this comment.
Hey @shoom3301 , great job, thank you!
There is 1 issue related to the ETH-bundled-flow, but I opened another task to address it later
… feat/safe-partial-approvals
Summary
Fixes https://www.notion.so/cownation/Safe-partial-amount-is-sent-to-approve-when-the-setting-is-OFF-2878da5f04ca80b184a5d97b9c8c8a18
Added support of partial approvals in Safe, it should work the same as usual swap but with tx bundling.
To Test
See above
Summary by CodeRabbit
Refactor
Tests