-
-
Notifications
You must be signed in to change notification settings - Fork 24
chore: scope types #133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: scope types #133
Conversation
jeffsmale90
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I've opened this PR #137 targeting this branch to introduce backwards compatibility. Have a look at the "Why" in there - I think we can get the benefits of this change, without breaking the established viem pattern of passing string union literals.
Lmk wyt.
Of note, this change will also need to be applied to the caveat configs. I've opened a ticket in our backlog.
jeffsmale90
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I left a suggestion to change the CHANGELOG and updated the description to describe the change to ScopeConfig to allow string values.
AyushBherwani1998
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let'ss go! LGTM.
| export const createCaveatBuilderFromScope = ( | ||
| environment: SmartAccountsEnvironment, | ||
| scopeConfig: ScopeConfig, | ||
| ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing return type on function eslint@typescript-eslint/explicit-function-return-type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
jeffsmale90
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
mj-kiwi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go!
📝 Description
Introduces a centralized
ScopeTypeenum to replace scattered string literal constants used for scope type discrimination. This improves type safety and discoverability when configuring delegation permission scopes.🔄 What Changed?
ScopeTypeenum toconstants.tswith all scope type values (Erc20TransferAmount,Erc20Streaming,Erc20PeriodTransfer,NativeTokenTransferAmount,NativeTokenStreaming,NativeTokenPeriodTransfer,Erc721Transfer,OwnershipTransfer,FunctionCall)ScopeTypefrom the package's public API viaindex.tsScopeType.Xinstead oftypeof builderConstantcreateCaveatBuilderFromScopeswitch cases to useScopeTypeenum valuesScopeTypeenumScopeConfigtype to be union ofScopeTypeenum andScopeTypeenum string values🚀 Why?
ScopeTypeenum) rather than hunting through individual builder filesScopeType.Erc20TransferAmountetc.Implementation,TransferWindow)By changing
ScopeConfigto be a union of ScopeType enum and string literal values, the interface aligns with established viem conventions. viem commonly uses string literal unions for parameters, and developers in that ecosystem generally expect to pass string values directly. Supporting both the enum reference and its string value preserves backwards compatibility, reduces friction for existing users, and adds flexibility without detracting from the value of the enum-based API. See #137 for more details.Effectively, this allows a caller to:
OR
🧪 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
Low Risk
Low risk: primarily type-system/API ergonomics changes around scope type discrimination, with only a small runtime cast/normalization in
createCaveatBuilderFromScopethat could affect behavior if invalid strings slip through.Overview
Adds a centralized
ScopeTypeenum (exported from the public API) and updates all scope config types to useScopeType.*instead of scattered string/builder constants.createCaveatBuilderFromScopenow normalizesscopeConfig.typeso callers can pass either the enum value or its string literal, while the internal switch consistently dispatches onScopeType. Tests and the changelog are updated accordingly, including coverage for passing a string scope type.Written by Cursor Bugbot for commit e65c06d. This will update automatically on new commits. Configure here.