[01/03] DeepSec approvals: permit cache and token identity#7837
Open
fairlighteth wants to merge 5 commits into
Open
[01/03] DeepSec approvals: permit cache and token identity#7837fairlighteth wants to merge 5 commits into
fairlighteth wants to merge 5 commits into
Conversation
Contributor
WalkthroughPermit handling is now scoped by token and spender across frontend state, hooks, and utility caches. Permit generation preserves explicit zero values and handles rejection errors more precisely. Native-token detection uses address equality, and a localization source reference was updated. ChangesFrontend permit scoping
Permit utility caching
Native-token detection
Localization reference
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant usePermitInfo
participant permittableTokensAtom
participant getTokenPermitInfo
usePermitInfo->>permittableTokensAtom: read token-spender cache
usePermitInfo->>getTokenPermitInfo: request permit info
getTokenPermitInfo-->>usePermitInfo: return permit result
usePermitInfo->>permittableTokensAtom: cache result with spender
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Deploying explorer-dev with
|
| Latest commit: |
3307c25
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://28c979d7.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://deepsec-approval-permit-01-c.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
3307c25
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3ef6be7b.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://deepsec-approval-permit-01-c.swap-dev-5u6.pages.dev |
6 tasks
fairlighteth
force-pushed
the
deepsec/approval-permit-01-cache-storage
branch
from
9a7cc91 to
5768811
Compare
fairlighteth
commented
Jul 10, 2026
fairlighteth
left a comment
fairlighteth
left a comment
Contributor
Author
There was a problem hiding this comment.
✅ AI Review (Codex GPT-5, worked 9m): follow-up addressed; no new findings
Rechecked
generatePermitHook.getCacheKeynow isolates requests by token, chain, owner, spender, amount, nonce, permit type, effective token name, and version.usePermitInfoandusePermitCompatibleTokenskeep custom-spender results separate from the default-relayer Gas-free badge.- The four focused suites pass: 20 tests covering cache isolation, transient-error eviction, spender-specific capability state, and default-spender presentation.
Result: Fixed. The original wrong-spender cache collision and custom-spender badge contamination paths no longer apply, and I found no new regression in the amended diff.
Review scope and related context
- Current head:
e0d08f908dc38c987ae518b146ca8e247a0f42cc. - Harness used: DeepSec PR mode plus code inspection and targeted unit tests.
- DeepSec status: fresh output inspected for the current 13-file PR scope; 8 analyses, exit code 0, no findings, and no comment artifact. Run:
20260710114946-058a9a29995b8463. - DeepSec helper:
run-deepsec-pr-mode.sh --pr 7837 --github-repo cowprotocol/cowswap; scope resolved throughgh pr diff 7837 --name-only. - CI: Test, Lint, Typecheck, and Agent Harness pass. Cypress has the same four
#do-trade-buttontimeouts as the exact base run. - Existing comments checked: no review threads or human findings; only deployment and skipped-draft bot comments.
- Confidence limit: real-wallet browser QA has not been performed. The PR body accurately leaves the fresh-profile, warm-storage, and custom-spender checklist open.
🤖 Verification notes for AI agents
The two prior findings were rechecked against current head e0d08f908:
1. Permit-generation requests with different spender, nonce, permit type,
effective token name, or version now have distinct in-flight cache keys.
2. Custom-spender permit capability entries cannot override or create the
regular token selector's default-relayer Gas-free state.
Focused regression tests and DeepSec PR mode pass. Do not reopen these findings
without a new concrete failure mode.
Generated using the pr-review and security-review skills from the CoW Protocol skills repo.
Contributor
Author
🧪 Scoped browser QA passed: fresh/warm permit paths and on-chain approval fallbackOutcome
Run details
Artifacts
How to retest
Generated using the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/permit-utils/src/lib/generatePermitHook.ts (1)
21-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
typeofguard overas numberassertion.After
'code' in error,error.codeis stillunknown. Usingtypeof error.code === 'number'would narrow the type without an assertion, making the code safer and theascast unnecessary.♻️ Proposed refactor
function hasUserRejectionCode(error: unknown): boolean { return ( typeof error === 'object' && error !== null && 'code' in error && - USER_REJECTION_CODES.includes(error.code as number) + typeof error.code === 'number' && + USER_REJECTION_CODES.includes(error.code) ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/permit-utils/src/lib/generatePermitHook.ts` around lines 21 - 28, In hasUserRejectionCode, replace the error.code as number assertion with a typeof error.code === 'number' guard before calling USER_REJECTION_CODES.includes, preserving the existing object and property checks while relying on type narrowing.libs/permit-utils/src/lib/generatePermitHook.test.ts (1)
36-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for rejection and non-rejection error handling.
The new
isUserRejectionErrorlogic and the.catchhandler ingeneratePermitHook(re-throwing rejections, swallowing other errors) are not covered by tests. Consider adding cases for: (1) user rejection errors propagate, (2) non-rejection errors resolve toundefined, (3) cache entry is evicted after either error type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/permit-utils/src/lib/generatePermitHook.test.ts` around lines 36 - 128, Add tests covering the error handling in generatePermitHook: mock the permit calldata builder to reject with a user-rejection error and assert the rejection propagates, then reject with a non-rejection error and assert the result is undefined. For both cases, verify the in-flight cache entry is evicted by invoking generatePermitHook again and asserting the builder is called again; use the existing request-cache test setup and identify the relevant isUserRejectionError and generatePermitHook catch behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/permit-utils/src/lib/generatePermitHook.test.ts`:
- Around line 36-128: Add tests covering the error handling in
generatePermitHook: mock the permit calldata builder to reject with a
user-rejection error and assert the rejection propagates, then reject with a
non-rejection error and assert the result is undefined. For both cases, verify
the in-flight cache entry is evicted by invoking generatePermitHook again and
asserting the builder is called again; use the existing request-cache test setup
and identify the relevant isUserRejectionError and generatePermitHook catch
behavior.
In `@libs/permit-utils/src/lib/generatePermitHook.ts`:
- Around line 21-28: In hasUserRejectionCode, replace the error.code as number
assertion with a typeof error.code === 'number' guard before calling
USER_REJECTION_CODES.includes, preserving the existing object and property
checks while relying on type narrowing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb5421a2-d334-4448-b8d3-37b7218dd1df
📒 Files selected for processing (14)
apps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/modules/permit/hooks/usePermitCompatibleTokens.test.tsapps/cowswap-frontend/src/modules/permit/hooks/usePermitCompatibleTokens.tsapps/cowswap-frontend/src/modules/permit/hooks/usePermitInfo.test.tsapps/cowswap-frontend/src/modules/permit/hooks/usePermitInfo.tsapps/cowswap-frontend/src/modules/permit/state/permittableTokensAtom.tsapps/cowswap-frontend/src/modules/permit/types.tslibs/common-utils/src/getIsNativeToken.test.tslibs/common-utils/src/getIsNativeToken.tslibs/permit-utils/src/index.tslibs/permit-utils/src/lib/generatePermitHook.test.tslibs/permit-utils/src/lib/generatePermitHook.tslibs/permit-utils/src/lib/getTokenPermitInfo.test.tslibs/permit-utils/src/lib/getTokenPermitInfo.ts
💤 Files with no reviewable changes (1)
- apps/cowswap-frontend/src/locales/en-US.po
10 tasks
Contributor
Author
|
Cross-PR coordination heads-up: #7697 adds The review on #7697 also found that the approval hook can run before the persistent cached-permit lookup, so cached reuse may still prompt the integrator. That issue is outside this PR's direct diff, but the cache contract here is foundational to fixing it consistently across the stack. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
permittableTokens:v4, while the regular token selector derives its Gas-free badge only from the default vault relayer.Why
DeepSec findings addressed
Concurrent permit generation can reuse a permit for the wrong spender/In-flight permit cache omits spender from the cache key: concurrent signing requests are now separated by every input that affects the permit payload, including spender, owner, amount, nonce, type, effective name, and version.Permit-info cache permanently stores transient errors and ignores estimation parameters: temporary RPC or estimation failures no longer poison permit detection for the page session, and cached support results are separated by spender, effective amount, and minimum gas threshold.Review guide
develop <- #7837 <- #7838 <- #7839.QA Testing
Preview URL QA (not yet completed):
1. Fresh-profile permit flow (Chromium and Firefox)
Expected result: the wallet requests a permit signature rather than an on-chain approval transaction. After rejection, the app remains usable and no transaction or order is submitted.
2. Warm-storage repeat (Chromium and Firefox)
Expected result: the Gas-free badge and permit-signature path remain unchanged after reload; the app does not unexpectedly switch to an on-chain approval.
3. Custom-spender isolation
0x000000000000000000000000000000000000dEaDcan be used because the request will not be signed.Expected result: checking the custom spender does not remove, add, or change the regular token selector's Gas-free badge. A subsequent regular permit request must still show CoW Swap's spender, never the test spender.
4. Non-permit approval fallback
Expected result: the token has no Gas-free badge and the wallet requests an on-chain approval transaction, not a permit signature. Rejection returns to a usable trade screen.
Record the browser, wallet, chain, token, expected result, actual result, and pass/fail status for each scenario. Attach a screenshot or short recording of the badge and wallet request where policy allows.
Developer verification on
e0d08f908:cowswap-frontendtypecheck and directpermit-utilsTypeScript validation passed.20260710114946-058a9a29995b8463).#do-trade-buttontimeouts as the exactdevelopbase run ata2e8a457d; this does not appear introduced by this PR.Reviewer note:
Preview URLs
Summary by CodeRabbit
Improvements
Bug Fixes