feat: introduce CaveatType enum and normalize caveat type handling in builders#170
feat: introduce CaveatType enum and normalize caveat type handling in builders#170
Conversation
packages/smart-accounts-kit/src/caveatBuilder/resolveCaveats.ts
Outdated
Show resolved
Hide resolved
jeffsmale90
left a comment
There was a problem hiding this comment.
This looks super nice!
Would it make sense to add test coverage? I think for the most part behaviour hasn't changed (as the enum is a string anyways), but maybe it would be nice to have tests showing how to pass an enum in each of the cases.
I hope typescript errors in the tests would cause the tests to fail, so we can at least assert the type resolutions work as expected.
packages/smart-accounts-kit/src/caveatBuilder/resolveCaveats.ts
Outdated
Show resolved
Hide resolved
…ad of returning a value, allowing TypeScript to narrow the type and eliminating the need for variable reassignment - Remove unnecessary 'as string' type assertion in resolveCaveats.ts - the type is already correctly inferred - Move CaveatType enum from constants.ts to coreCaveatBuilder.ts to better organize the caveat-specific type definition - Remove ConvertCaveatConfigsToInputs and CoreCaveatConfiguration from public exports - only expose CaveatType and essential builder types - Update imports across the codebase to reflect the new locations
packages/smart-accounts-kit/src/caveatBuilder/resolveCaveats.ts
Outdated
Show resolved
Hide resolved
packages/smart-accounts-kit/src/caveatBuilder/coreCaveatBuilder.ts
Outdated
Show resolved
Hide resolved
…lders - Updated caveat builders to use the CaveatType enum instead of string literals for better type safety and maintainability. - Removed the validateCaveatType function from utils as it is no longer necessary. - Adjusted tests to reflect the changes in caveat type usage.
…gToInputs in scope types
jeffsmale90
left a comment
There was a problem hiding this comment.
Looking really good! It's definitely a struggle when changes touch every caveat type!
A couple minor comments - mostly I think we need to be able to:
caveatBuilder.add(CaveatType.AllowedCaveats, { ... }); // works
caveatBuilder.add("allowedCaveats", { ...}); // doesn't work
packages/smart-accounts-kit/src/caveatBuilder/resolveCaveats.ts
Outdated
Show resolved
Hide resolved
packages/smart-accounts-kit/test/caveatBuilder/createCaveatBuilder.test.ts
Show resolved
Hide resolved
packages/smart-accounts-kit/src/caveatBuilder/coreCaveatBuilder.ts
Outdated
Show resolved
Hide resolved
…oInputs for improved clarity and consistency
…s and support CaveatTypeParam
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.
…l support and update tests for compile-time validation
jeffsmale90
left a comment
There was a problem hiding this comment.
I think generally this looks great.
There's something about the generics of the CaveatBuilder that's not quite sitting right. I've left a couple minor comments on the topic, but I'm going to have to spend a minute tomorrow getting my head around it properly. Maybe we can chat in the morning.
| export type CoreCaveatBuilder = CaveatBuilder<CoreCaveatMap> & { | ||
| addCaveat<TKey extends CaveatTypeParam>( | ||
| name: TKey, | ||
| config: CoreCaveatParamToConfig[TKey], | ||
| ): CoreCaveatBuilder; | ||
| }; |
There was a problem hiding this comment.
Why do we need to extend the CaveatBuilder type here with the shadowing addCaveat function? Does that indicate a deficiency with the CaveatBuilder type that might bite users who consume the CaveatBuilder directly (not the CoreCaveatBuilder)?
| * Supports both enum and string literal when the builder includes that enforcer, e.g.: | ||
| * - caveatBuilder.addCaveat(CaveatType.AllowedMethods, { ... }) | ||
| * - caveatBuilder.addCaveat('allowedMethods', { ... }) |
There was a problem hiding this comment.
is this true of the CaveatBuilder itself? Or is this only true if TCaveatBuilderMap specifies keys that are a union of the enum and string values?
|
A better solution to add the CaveatType enum #179 |
📝 Description
Apply the ScopeType enum pattern from PR #133 to caveat type parameters, allowing both enum references and string literals when defining caveats.
🔄 What Changed?
CaveatTypeenum toconstants.tswith all 26 caveat enforcer function namesConvertCaveatConfigsToInputsgeneric type to support flexible caveat type specification (enum or string)CaveatConfigurationtype to use the flexible type conversion patternCaveatBuilder.addCaveat()to normalize and accept both enum and string caveat typesresolveCaveats()function to handle flexible caveat type normalizationCaveatType,CoreCaveatConfiguration,ConvertCaveatConfigsToInputs) in public API🚀 Why?
🧪 How to Test?
CaveatType.AllowedMethodsenum reference incaveatBuilder.addCaveat()'allowedMethods'createDelegationcaveats array📋 Checklist
🔗 Related Issues
Related to #145
Related to PR #133 (ScopeType enum pattern)
Related to PR #137 (enum flexibility pattern)
📚 Additional Notes
This implementation follows the same flexible type pattern established in PR #133 for ScopeType. Both enum references and string literals are supported for maximum flexibility:
Note
Medium Risk
Medium risk because it changes the public caveat-construction API surface (new
CaveatTypeexport, widenedaddCaveat/resolveCaveatstyping, and updated error messaging), which could impact downstream TypeScript inference and caveat resolution if any mapping is incorrect.Overview
Adds a new
CaveatTypeenum (plusCaveatTypeParam) and reworks core caveat builder typing soaddCaveat()andCaveatConfigurationaccept eitherCaveatType.*or the existing string literal caveat names while preserving per-caveat config type checking.Updates all core caveat builder modules and scope helpers to use
CaveatTypeconstants internally, exportsCaveatTypevia the public kit +utils, and adjustsresolveCaveats()to accept enum/stringtypevalues while improving errors to include the failing caveat index.Refreshes e2e and unit tests to use
CaveatType, and adds new coverage proving enum and string forms behave identically (including explicit string-literal cases inallowedCalldatatests).Written by Cursor Bugbot for commit 0c71b08. This will update automatically on new commits. Configure here.