Add makePermissionDecoderConfigs to resolve the PermissionDecoderConfigs to be used with @metamask/gator-permissions-controller#259
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
92f2173 to
cbda271
Compare
cbda271 to
e37bee7
Compare
makePermissionDecoderConfigs to resolve the PermissionDecoderConfigs to be used with @metamask/gator-permissions-controller
…ing returned data is appropriately typed.
7b75406 to
1930bd9
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1930bd9. Configure here.
| periodDuration?: number; | ||
| startDate?: number; | ||
| } = {}): Hex => | ||
| createERC20TokenPeriodTransferTerms( |
There was a problem hiding this comment.
makeTerms currently uses createERC20TokenPeriodTransferTerms, which validates inputs before producing the encoded terms. That means the zero periodAmount / zero periodDuration tests are exercising delegation-core validation rather than this decoder's validation branches.
Can we construct the raw hex terms manually here, like nativeTokenPeriodic.test.ts does, so the test actually reaches the decoder checks in this package?
There was a problem hiding this comment.
Good call - I've updated this.
|
|
||
| if (matchingCaveats.length === 0) { | ||
| if (throwIfNotFound ?? true) { | ||
| throw new Error('Invalid caveats'); |
There was a problem hiding this comment.
The missing/duplicate caveat error from getTermsByEnforcer is currently just Invalid caveats, which makes debugging pretty hard. Could we include the enforcer address and distinguish missing vs duplicate matches?
Something like:
Invalid caveats: missing terms for enforcer ${enforcer}
Invalid caveats: multiple caveats found for enforcer ${enforcer}
There was a problem hiding this comment.
I've refactored this function to improve readability, and updated the error messages as you suggested.
| import type { ChecksumCaveat } from '../../../src/permissions/types'; | ||
| import { getChecksumEnforcersByChainId } from '../../../src/permissions/utils'; | ||
|
|
||
| describe('expiryRule', () => { |
There was a problem hiding this comment.
The expiry rule has several explicit error paths that are not covered by tests yet:
- invalid terms length
timestampBeforeThreshold <= 0timestampAfterThreshold !== 0
Can we add tests for those branches so the validation behavior is locked down?
There was a problem hiding this comment.
Added tests to cover these cases
| } | ||
|
|
||
| const [caveat] = matchingCaveats; | ||
| if (!caveat) { |
There was a problem hiding this comment.
This if (!caveat) branch appears unreachable: the code already returned when there were zero matches and already throws when there is more than one match. Also, the error message here says “multiple” even though this branch would imply no caveat.
Can we remove this dead branch?
There was a problem hiding this comment.
It's actually reimplementing the functionality in getTermsByEnforcer, so I've updated it to use that utility.
…tamask/delegation-core encoders
Update payeeRuleDecoder to use getTermsByEnforcer utility
mj-kiwi
left a comment
There was a problem hiding this comment.
Some nits may need to be addressed before merging.
| "vitest": "^3.2.4" | ||
| }, | ||
| "dependencies": { | ||
| "@metamask/delegation-core": "^2.2.1", |
There was a problem hiding this comment.
Move @metamask/delegation-deployments to devDependencies. I think it’s only used for tests.
There was a problem hiding this comment.
👍
I think we could potentially remove the dependency entirely - we don't strictly need the dependency, as we could make up environment values, but it's easier to import 🤷
There was a problem hiding this comment.
makePermissionDecoderConfigs returns PermissionDecoderConfig[] and accepts DeployedContractsByName, but neither type is exported from src/index.ts. Consumers who want to annotate a variable or write a helper that accepts/returns these types have no way to reference them. Please export both.
There was a problem hiding this comment.
Good call! These are the types that I mentioned this morning :)
Updated!
| throw new Error('Invalid native-token-allowance terms: expected 96 bytes'); | ||
| } | ||
|
|
||
| const [allowanceAmount, periodDurationRaw, startTimeRaw] = splitHex( |
There was a problem hiding this comment.
splitHex always prepends 0x to every segment, so every returned value is truthy — the if (!tokenAddress || !allowanceAmount || ...) guard can never fire. The byte-length check above it is the real defence. Please remove this dead guard
There was a problem hiding this comment.
splitHex returns Hex[] of indeterminate length, meaning each of the destructured values is Hex | undefined.
i've updated splitHex to return a Hex[] of the same length as the specified lengths array, meaning we can remove the guard as you suggest 👍
|
|
||
| const [tokenAddress, allowanceAmount, periodDurationRaw, startTimeRaw] = | ||
| splitHex(terms, [20, 32, 32, 32]); | ||
| if ( |
There was a problem hiding this comment.
splitHex always prepends 0x to every segment, so every returned value is truthy — the if (!tokenAddress || !allowanceAmount || ...) guard can never fire. The byte-length check above it is the real defence. Please remove this dead guard
There was a problem hiding this comment.
as above - fixed with an update to splitHex 👍

📝 Description
This PR relocates the
PermissionDecoderConfigs from @metamask/gator-permissions-controller into @metamask/7715-permission-types.🔄 What Changed?
Adds the following to @metamask/7715-permission-types:
makePermissionDecoderConfigsfunction to resolve thePermissionDecodeConfigsExpiryRule,PayeeRule, andRedeemerRuletypesNote: erc20-token-allowance and native-token-allowance still have bespoke implementations as they use periodic caveats with
UINT256_MAXas the period duration, which is decoded as a number. These will be replaced with basic allowance caveats, at which point these bespoke decodes can be discarded.🚀 Why?
Presently in order to add a new permission type, changes must be made to @metamask/smart-accounts-kit, @metamask/7715-permission-types, @metamask/gator-permissions-snap, @metamask/gator-permissions-controller.
By moving all of the logic to define the permission types into @metamask/7715-permission-types, and importing these into the various packages that need these definitions, the bulk of the changes will be made in this one place.
🧪 How to Test?
Describe how to test these changes:
List any breaking changes:
📋 Checklist
Check off completed items:
🔗 Related Issues
Link to related issues:
Closes #
Related to #
📚 Additional Notes
Any additional information, concerns, or context:
Note
Medium Risk
Relocates delegation caveat decoding and validation used to interpret execution permissions; incorrect decoding could misread limits, payees, or redeemers, though behavior is heavily tested and mostly delegated to delegation-core.
Overview
Moves permission decoder configuration from
@metamask/gator-permissions-controllerinto@metamask/7715-permission-types, so consumers can build chain-specific decoders from one shared package.makePermissionDecoderConfigstakes deployed delegation contracts, resolves checksummed enforcer addresses, and returns sevenPermissionDecoderConfigentries (native/ERC-20 stream, periodic, allowance, plus token-approval-revocation). The package now exportsExpiryRule,PayeeRule, andRedeemerRulealongside that factory.Each config wires required/optional enforcers, rule decoders (expiry, redeemer, ERC-20 or native payee), and
validateAndDecodeDatathat turns on-chain caveats into typed permission payloads. Most types use@metamask/delegation-coreterm decoders; erc20/native-token-allowance still use custom parsing for periodic enforcer terms withUINT256_MAXperiod duration.Adds runtime dependencies on
delegation-core,delegation-deployments, andutils, Vitest coverage for decoders and rules, and test-focused ESLint/tsconfig updates.Reviewed by Cursor Bugbot for commit 1930bd9. Bugbot is set up for automated code reviews on this repo. Configure here.