Improve declarative delegation interface #63

hanzel98 · 2025-08-20T20:17:05Z

📝 Description

Improves the declarative delegation interface developer experience by making caveats optional, removing unnecessary parameters, and adding calldata support to native token scopes.

🔄 What Changed?

List the specific changes made:

Made caveats parameter optional in createDelegation and createOpenDelegation functions
Removed allowInsecureFullAuthorizationDelegation parameter from createDelegation and createOpenDelegation (kept in signDelegation and CaveatBuilder)
Added calldata support to native token scopes (nativeTokenTransfer, nativeTokenStreaming, nativeTokenPeriodic)
Added exactCalldata and allowedCalldata parameters to native token scope configurations
Updated resolveCaveats function to handle optional caveats parameter
Created comprehensive E2E tests for calldata functionality with new PayableReceiver smart contract
Added helper functions for deploying and interacting with test contracts

🚀 Why?

Explain the motivation behind these changes:

Better Developer Experience: Developers no longer need to pass empty arrays when using scopes without additional caveats
Cleaner API: Removed redundant parameter that's no longer relevant since delegations always have caveats from scopes
Enhanced Flexibility: Developers can now specify calldata restrictions when transferring native tokens, enabling more precise control over delegated operations
Improved Testing: E2E tests now use real contract interactions instead of hardcoded calldata, providing more realistic validation

🧪 How to Test?

Describe how to test these changes:

Manual testing steps:
Create a delegation using createDelegation without specifying caveats parameter
Create a delegation with native token scope including exactCalldata or allowedCalldata
Verify delegation creation works with scope-only caveats
Automated tests added/updated
All existing tests pass

⚠️ Breaking Changes

List any breaking changes:

Breaking changes (describe below):
Removed allowInsecureFullAuthorizationDelegation parameter from createDelegation and createOpenDelegation functions

Updated native token scope type definitions to include optional calldata parameters

📋 Checklist

Check off completed items:

🔗 Related Issues

Closes # https://app.zenhub.com/workspaces/readable-permissions-67982ce51eb4360029b2c1a1/issues/gh/metamask/delegator-readable-permissions/345

📚 Additional Notes

The PayableReceiver.sol contract was created specifically for testing calldata restrictions in E2E tests, providing a realistic target for delegated contract calls
Helper functions in test/utils/helpers.ts now return Promise directly to avoid type casting in tests
All native token scope tests now perform proper end-to-end validation by creating, signing, and redeeming delegations on-chain
Error message assertions updated to match actual smart contract revert messages (invalid-calldata instead of previous placeholder messages)

hanzel98 · 2025-08-20T20:26:54Z

Key Documentation Updates Needed:

createDelegation and createOpenDelegation API Changes:

Update examples to show caveats parameter is now optional
Remove references to allowInsecureFullAuthorizationDelegation parameter
Add examples showing scope-only delegations

Native Token Scope Enhancements:

Document new exactCalldata and allowedCalldata parameters for native token scopes
Update examples in delegation-core/README.md to show calldata usage
Add examples of using calldata restrictions with native token transfers

Breaking Changes Documentation:

Document the removal of allowInsecureFullAuthorizationDelegation from creation functions
Update any migration guides or changelog entries

External Documentation:

The external docs sites (docs.gator.metamask.io, docs.metamask.io/delegation-toolkit/) will need updates to reflect:
- New optional caveats parameter
- Calldata support in native token scopes
- Updated API signatures

MoMannn · 2025-08-26T13:55:33Z

packages/delegation-toolkit/src/caveatBuilder/scope/nativeTokenPeriodicScope.ts

+    allowedCalldata.forEach((calldataConfig) => {
+      caveatBuilder.addCaveat('allowedCalldata', calldataConfig);
+    });
+  } else if (exactCalldata) {


So here the user can specify either allowedCalldata or exactCalldata. But the function accepts both - this could confuse the developer. I would set it to either:

calldataRestriction?: AllowedCalldataBuilderConfig[] | ExactCalldataBuilderConfig

Or make it that if both parameters are defined and error is thrown to protect the user from unforeseen behavior.

Same with other scopes.

sure implemented the protection when both are passed

packages/delegation-toolkit/test/delegation.test.ts

jeffsmale90 · 2025-09-01T23:10:30Z

packages/delegation-toolkit/src/caveatBuilder/scope/functionCallScope.ts

+  if (allowedCalldata && allowedCalldata.length > 0 && exactCalldata) {
+    throw new Error(
+      'Cannot specify both allowedCalldata and exactCalldata. Please use only one calldata restriction type.',
+    );
+  }
+
  const caveatBuilder = createCaveatBuilder(environment)
    .addCaveat('allowedTargets', { targets })
    .addCaveat('allowedMethods', { selectors });

-  if (allowedCalldata) {
+  if (allowedCalldata && allowedCalldata.length > 0) {


nit: these two allowedCalldata && allowedCalldata.length > 0 checks could be simplified to allowedCalldata?.length > 0 because this will never be true if allowedCalldata is undefined.

If I change it I get this issue

ah my bad - because the operator is not an equality operator, this doesn't work (can't do > with undefined).

You could coalesce and keep the optional chaining, or you could change to an equality operator. Or you could leave it as it is.

if (allowedCalldata?.length !== 0) { ... } // actually, you'd probably want to switch the two cases and make this an `===` comparison if (allowedCalldata?.length ?? 0 > 0) { ... }

Hey, I tried to change it using those cool ideas but I got lint errors.

jeffsmale90

lgtm! one minor nit, duplicated across a couple files.

I don't know if it's worthwhile deduplicating the scope logic now, might be worthwhile deferring to a separate change.

jeffsmale90 · 2025-09-01T23:11:57Z

packages/delegation-toolkit/src/caveatBuilder/scope/nativeTokenPeriodicScope.ts

+    exactCalldata,
+  } = config;
+
+  if (allowedCalldata && allowedCalldata.length > 0 && exactCalldata) {


nit: same simplification as above - allowedCalldata?.length > 0

I get an issue if I change it

jeffsmale90 · 2025-09-01T23:12:18Z

packages/delegation-toolkit/src/caveatBuilder/scope/nativeTokenPeriodicScope.ts

+  const caveatBuilder = createCaveatBuilder(environment);
+
+  // Add calldata restrictions
+  if (allowedCalldata && allowedCalldata.length > 0) {


nit: same simplification as above - allowedCalldata?.length > 0

I get an issue if I change it

jeffsmale90 · 2025-09-01T23:15:27Z

packages/delegation-toolkit/src/caveatBuilder/scope/nativeTokenStreamingScope.ts

+  if (allowedCalldata && allowedCalldata.length > 0 && exactCalldata) {
+    throw new Error(
+      'Cannot specify both allowedCalldata and exactCalldata. Please use only one calldata restriction type.',
+    );
+  }
+
+  const caveatBuilder = createCaveatBuilder(environment);
+
+  // Add calldata restrictions
+  if (allowedCalldata && allowedCalldata.length > 0) {


nit: same simplification as above - allowedCalldata?.length > 0

I wonder if there's value in deduplicating this logic. Maybe a concern for a separate PR.

I get an issue if I change it

jeffsmale90 · 2025-09-01T23:15:59Z

packages/delegation-toolkit/src/caveatBuilder/scope/nativeTokenTransferScope.ts

+  if (allowedCalldata && allowedCalldata.length > 0 && exactCalldata) {
+    throw new Error(
+      'Cannot specify both allowedCalldata and exactCalldata. Please use only one calldata restriction type.',
+    );
+  }
+
+  const caveatBuilder = createCaveatBuilder(environment);
+
+  // Add calldata restrictions
+  if (allowedCalldata && allowedCalldata.length > 0) {


nit: same simplification as above - allowedCalldata?.length > 0

I get an issue if I change it

refactor: improve declarative delegation interface

22597f3

hanzel98 requested a review from a team as a code owner August 20, 2025 20:17

This comment was marked as outdated.

Sign in to view

fix: empty calldata array bypasses security checks

677075e

hanzel98 requested a review from jeffsmale90 August 20, 2025 20:54

hanzel98 self-assigned this Aug 20, 2025

hanzel98 requested a review from AyushBherwani1998 August 20, 2025 20:54

MoMannn requested changes Aug 26, 2025

View reviewed changes

chore: prevent doble calldata

11441de

hanzel98 requested a review from MoMannn August 27, 2025 20:33

MoMannn previously approved these changes Aug 28, 2025

View reviewed changes

jeffsmale90 reviewed Sep 1, 2025

View reviewed changes

jeffsmale90 previously approved these changes Sep 1, 2025

View reviewed changes

refactor: modernize allowedCalldata checks with optional chaining

3a8ab8b

hanzel98 dismissed stale reviews from jeffsmale90 and MoMannn via 3a8ab8b September 4, 2025 16:27

hanzel98 added 2 commits September 5, 2025 13:23

chore: reverted validation changes

967410b

test: fix deployment in test

63c03c1

jeffsmale90 approved these changes Sep 8, 2025

View reviewed changes

hanzel98 merged commit c70e131 into main Sep 8, 2025
17 checks passed

hanzel98 deleted the refactor/improve-delegation-devex branch September 8, 2025 20:34

Uh oh!

Improve declarative delegation interface #63

Improve declarative delegation interface #63

Uh oh!

Conversation

hanzel98 commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description

🔄 What Changed?

🚀 Why?

🧪 How to Test?

⚠️ Breaking Changes

📋 Checklist

🔗 Related Issues

📚 Additional Notes

Uh oh!

This comment was marked as outdated.

Uh oh!

hanzel98 commented Aug 20, 2025

Key Documentation Updates Needed:

createDelegation and createOpenDelegation API Changes:

Native Token Scope Enhancements:

Breaking Changes Documentation:

External Documentation:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffsmale90 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hanzel98 commented Aug 20, 2025 •

edited

Loading