feat: add token approval revocation functionality#328
Conversation
- Add tests for token approval revocation context, content, and validation. - Create a new TokenApprovalRevocationForm component to handle user input for revocation methods. - Update the RedemptionForm to support various token approval revocation methods. - Refactor existing ERC20TokenRevocationForm references to the new TokenApprovalRevocationForm. - Modify the RPC handler and supported rule types to accommodate the new token approval revocation type. - Remove deprecated ERC20TokenRevocationForm component.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
jeffsmale90
left a comment
jeffsmale90
left a comment
There was a problem hiding this comment.
Generally I think this looks great - let's get some guidance on the UX of displaying the different primitives.
| "revokeTokenApprovalsLabel": { | ||
| "message": "Revoke token approvals" | ||
| }, | ||
| "approvalRevocationMechanismsLabel": { |
There was a problem hiding this comment.
nit; we've used the term Primitive to refer to the different ways that a permit can be revoked - at least in code I think it's a good idea to standardise that.
| ({ key }) => permissionData[key] === true, | ||
| ).map(({ labelKey }) => t(labelKey)); | ||
| const revokeValue = | ||
| revocationMechanisms.length > 0 |
There was a problem hiding this comment.
are we saying that if none of the booleans values in the data bag are true, then all primitives are allowed?
I had considered this an invalid request.
But maybe it would be a good idea, if all of the revocation primitives are true that we should say "All revocation primitives" or something instead of listing them all out. I suspect that would be the more common experience.
| const revokeValue = t('allTokens'); | ||
| const revocationMechanisms = TOKEN_APPROVAL_REVOCATION_PRIMITIVES.filter( | ||
| ({ key }) => permissionData[key] === true, | ||
| ).map(({ labelKey }) => t(labelKey)); |
There was a problem hiding this comment.
what does t() return if there's no match? should we default to the labelKey, or throw?
| if (contracts.approvalRevocationEnforcer === ZERO_ADDRESS) { | ||
| throw new InvalidInputError( | ||
| 'ApprovalRevocationEnforcer address is not configured.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
is this a temporary check until we add the revocation enforcer? I don't think we have this check on the other enforcers.
| {enabledMechanisms.map(({ key, labelKey }) => ( | ||
| <Text key={key}>{t(labelKey)}</Text> | ||
| ))} |
There was a problem hiding this comment.
I've raised with UX, but can we at least make this a bullet list? (I don't know if we can 😢)
There was a problem hiding this comment.
Maybe not for this PR, but we should probably just import @metamask/delegation-deployments instead of copying the contract addresses 🤔
2 participants
Description
Replaces the disabled
erc20-token-revocationpermission withtoken-approval-revocation.This adds support for common token approval revocation mechanisms:
approve(spender, 0)approve(address(0), tokenId)setApprovalForAll(operator, false)approve(token, spender, 0, 0)lockdowninvalidateNoncesThe PR wires the new permission through offer creation, validation, caveat terms, permission intro rendering, existing permission formatting, site request UI, and site redemption calldata generation.
Need to update the dependence when MetaMask/smart-accounts-kit#226 is ready.
Changelog entry: Not added in this branch.
Related issues
Fixes:
Manual testing steps
yarn start.http://localhost:8000/.token-approval-revocationpermission type.Revocation methodssection renders as a vertical checkbox list.Screenshots/Recordings
Before
N/A
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Introduces a new revocation permission and caveat encoding backed by a new enforcer address (currently configured as zero), which could block revocation flows or change on-chain call constraints if misconfigured.
Overview
Replaces the disabled
erc20-token-revocationpermission with a newtoken-approval-revocationpermission that supports multiple approval-revocation mechanisms (ERC-20, ERC-721, and Permit2) and surfaces the selected mechanisms in both confirmation and existing-permissions UIs.Adds a new
ApprovalRevocationEnforcercontract slot to chain metadata and switches revocation caveat construction from hard-coded calldata/value constraints to a single enforcer term bitmask (rejecting requests if the enforcer address is unset).Updates supported-permission wiring end-to-end (offers, handler factory, intro content, shared supported rule types) and updates the site demo to request mechanism flags and generate the correct target/calldata for the chosen revocation method.
Reviewed by Cursor Bugbot for commit b12594e. Bugbot is set up for automated code reviews on this repo. Configure here.