feat: CoW Swap cash out after permit claim (#386)#457
feat: CoW Swap cash out after permit claim (#386)#457energypantry wants to merge 11 commits intoubiquity:developmentfrom
Conversation
|
PR marked ready for review. Recent updates to better match the original Issue #386 spec:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a65d26961
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { orderId } = await postCowSwapOrder({ | ||
| tokenIn, | ||
| tokenOut, | ||
| amountIn, | ||
| owner: address, | ||
| receiver, |
There was a problem hiding this comment.
Here owner is always the connected address, but earlier you set receiver from permit.beneficiary. If a permit’s beneficiary isn’t the connected wallet, the claimed tokens are sent to the beneficiary while the order is signed and funded by address, so the order will be unfillable (and the allowance check above also uses address). This means users claiming on behalf of someone else will see “submitted” swaps that can never settle. Consider skipping CoW submissions unless permit.beneficiary matches the connected wallet (or use the beneficiary as signer, which isn’t possible here).
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughReplaced single aggregated claimable total with per-token totals ( 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 1
🧹 Nitpick comments (6)
src/utils/cowswap-utils.ts (2)
120-148: Significant duplication betweenpostCowSwapOrderandgetCowSwapQuote.Lines 128-148 replicate the appData building and quote-fetching logic from lines 76-96. If slippage, appCode, or partner-fee logic changes, both must be updated in lockstep.
Consider extracting a shared helper (e.g.
buildQuoteRequest) that both functions call.
159-165: Doubleas unknowncast on the quote message is fragile.
quoteResponse.quote as unknown as Record<string, unknown>bypasses all type safety. If the CoW SDK changes the quote shape, this will silently produce an invalid EIP-712 signature.Consider typing the message more precisely or at least adding a runtime guard for required fields (
sellToken,buyToken,sellAmount,buyAmount,validTo, etc.) before signing.src/hooks/use-permit-claiming.ts (2)
226-233:MAX_UINT256unlimited approval — intentional?Granting unlimited allowance to the CoW vault relayer is common in DeFi but means the relayer can spend all of the user's tokens of that type indefinitely. Consider approving only the needed
amountInto limit exposure.Proposed change
- const MAX_UINT256 = (1n << 256n) - 1n; const approveTx = await walletClient.writeContract({ address: tokenIn, abi: erc20Abi, functionName: "approve", - args: [spender, MAX_UINT256], + args: [spender, amountIn], account: address, chain, });
191-202: Grouping key is normalized but status key (line 207) is not.Line 199 builds the group key with
.toLowerCase()on all addresses, but line 207 builds the status key without normalization. This inconsistency makes correlating group → status unreliable during debugging.Proposed fix
- const key = `${chainId}:${tokenIn}->${tokenOut}:${receiver}`; + const key = `${chainId}:${tokenIn.toLowerCase()}->${tokenOut.toLowerCase()}:${receiver.toLowerCase()}`;src/components/dashboard-page.tsx (2)
532-550: Swap status messages persist indefinitely — no way to dismiss.Once a swap completes or errors, the status banners stay on screen forever. Consider adding a dismiss mechanism (like
TxBannerhasonDismiss) or auto-clearing after a timeout.
142-194: RedundantBigInt()wrapper onpermit.amount(line 173).
permit.amountis alreadybigint(used with>comparison at line 110). TheBigInt()call is a no-op here but adds noise and a try/catch for an impossible error.
|
@coderabbitai I added JSDoc for the CoW swap helpers and permit hooks to address the docstring coverage gate from the previous review. Could you please re-review when you have a moment? Tested locally:
|
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/utils/cowswap-utils.ts (2)
46-48:asSupportedChainIdperforms no validation.The cast blindly trusts upstream callers. If an unsupported chain slips through,
OrderBookApisilently targets the wrong endpoint. A runtime guard here is cheap insurance.Proposed fix
function asSupportedChainId(chainId: number): SupportedChainId { + const supported: number[] = Object.values(SupportedChainId).filter((v) => typeof v === "number"); + if (!supported.includes(chainId)) { + throw new Error(`Unsupported CoW Protocol chainId: ${chainId}`); + } return chainId as SupportedChainId; }
87-107: Duplicate appData construction and quote fetching betweengetCowSwapQuoteandpostCowSwapOrder.Both functions build identical
appDataInfoand callorderBookApi.getQuotewith the same shape. Extract a shared helper to reduce duplication and keep the two paths in sync.Also applies to: 139-159
src/hooks/use-permit-claiming.ts (4)
244-261:MAX_UINT256approval is a broad security posture.Infinite approval to the CoW vault relayer means a compromise of the relayer contract drains all user tokens. Consider approving only
amountInfor a tighter security profile, or at least document the trade-off.
203-283:maybeSubmitCowSwapswallows errors silently for individual groups.If the first group succeeds but the second fails (e.g., user rejects the second signing prompt), the user gets a generic "Swap failed" status string but no differentiation between user-rejection and infra errors. Consider checking
isUserRejectedRequesthere too and using a distinct status like"rejected".
674-682:swapSubmissionStatusis exposed but never cleared.Once statuses accumulate, they persist across claims. Consider resetting
swapSubmissionStatusat the start of each claim flow (or providing a reset function) so stale entries don't confuse the UI.
446-522: Function namehandleClaimSequentialis misleading—it executes permits concurrently viaPromise.allSettled, not sequentially.Either rename to reflect concurrent execution (e.g.,
handleClaimIndividual) if concurrent behavior is intentional, or implement true sequential processing with a serial loop if nonce conflicts or rate limiting requires sequential execution.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/utils/cowswap-utils.ts (3)
70-75:assertQuoteShapeonly checks 5 of the ~12 EIP-712 Order fields.Fields like
receiver,feeAmount,kind,partiallyFillable,sellTokenBalance,buyTokenBalanceare all required in the Order struct for a valid EIP-712 signature. If the quote response ever omits one of these, you'll get an opaque signing or submission error rather than a clear assertion failure.Consider validating the full set of Order-required fields, or better yet, explicitly picking the Order fields from the quote (which also addresses the concern below about extra fields leaking into the signature).
103-108:partnerFeeBpscondition inbuildCowAppDataInforelies on truthiness — will silently skip fee ifbpsis0.Currently safe because
getPartnerFeeBpsreturns eitherCOWSWAP_PARTNER_FEE_BPS(10) orundefined. But if someone later passes0, line 115'spartnerFeeBps ? ...would treat it as "no fee". Use an explicit!== undefinedcheck for robustness.Proposed fix
- ...(partnerFeeBps + ...(partnerFeeBps !== undefined
216-224:sendOrderspreadsquoteResponse.quotewhich may include fields the API doesn't expect.Same root cause as the signing concern — spreading the raw quote may send extraneous fields (
signingScheme,from, etc.) to the order submission endpoint. Prefer constructing the order payload explicitly from picked Order fields plus the additional metadata (appData,appDataHash,from,quoteId,signature,signingScheme).
|
@coderabbitai review Pushed a fix to approve the total required amount once per token before submitting multi-group orders so allowances are not overwritten across groups. |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review Pushed a follow-up to build the EIP-712 message strictly from Thanks! |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/hooks/use-permit-claiming.ts (1)
413-414:maybeSubmitCowSwapis fire-and-forget but has no top-level catch.The
voidcall here (and Lines 569, 668) relies onmaybeSubmitCowSwapnever throwing. As noted above, the unprotected approve call breaks that assumption. Once the approve is wrapped in try/catch, consider also adding a top-level.catchon these call sites as a safety net.- void maybeSubmitCowSwap([permit]); + void maybeSubmitCowSwap([permit]).catch((err) => console.warn("CoW swap submission failed", err));
|
@coderabbitai review Pushed fixes:
Commit: 2a88e98 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils/cowswap-utils.ts (1)
224-232: Theas unknown as SendOrderParamsdouble-cast disables all type safety on the request body.This is pragmatic but fragile — any field name drift between the SDK's
SendOrderParamstype and the EIP-712 Order struct will silently produce invalid requests. Consider adding a brief inline comment explaining why the cast is necessary (e.g., SDK type mismatch) so future maintainers don't remove it or miss regressions.
|
@coderabbitai review Addressed the latest Commit: 70dcff9 |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review Addressed the remaining actionable item about an unhandled throw from Commit: ea4af46 |
|
✅ Actions performedReview triggered.
✅ Actions performedReview triggered.
|
|
CodeRabbit approved and PR is mergeable (CLEAN). Latest commit: ea4af46. Could a maintainer please merge when you have a moment so the bounty can move to payout? Thanks. |
|
Quick ping (2026-02-08): this PR is still CodeRabbit-approved and mergeable (CLEAN). If you have a moment to merge, it would help move the bounty toward payout. Happy to adjust anything if needed. Thanks! |
|
Any QA to support this? |
|
@gentlementlegen QA notes from my side: On the PR head branch, I ran:
|
|
Quick merge follow-up: this PR is approved and shows mergeState=CLEAN. If nothing else is needed, could a maintainer merge it when convenient to unblock the bounty payout? Thanks! |
|
Quick follow-up: PR is still showing as MERGEABLE + APPROVED on my side. If there's anything else needed (extra QA, docs, or tweaks) to get this merged, tell me and I'll turn it around immediately. |
|
Thanks for the reviews and QA on this. For payout, please use my EVM address (USDT on Arbitrum preferred): If you need an off-chain rail instead, PayPal also works: Once merged, could you confirm the expected payout timeline / next step on your side? |
|
Friendly ping on this one when you have a moment. PR is ready to merge (approved, checks green, Thanks! |
|
Quick follow-up on this one. I have already pushed the latest requested updates and the PR is still clean/approved on my side. If there is any remaining blocker before merge, I can address it immediately. If everything looks good, could we proceed with merge and payout when convenient? |
|
@energypantry as long as there is not real E2E testing with proven results, I will not accept the merge. |
2 participants
Fixes #386
What
Notes
Testing
bun run lintbun run typecheckbun run build