-
-
Notifications
You must be signed in to change notification settings - Fork 255
[base-controller] Make allowed{Actions,Events} required parameters of getRestricted
#4035
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
[base-controller] Make allowed{Actions,Events} required parameters of getRestricted
#4035
Conversation
getRestricted not inferring omitted allowed{Actions,Events} as never[] when used with unrestrictedControllerMessenger pattern getRestricted not inferring omitted allowed{Actions,Events} as never[] when used with unrestrictedControllerMessenger pattern
e855785 to
522ece0
Compare
getRestricted not inferring omitted allowed{Actions,Events} as never[] when used with unrestrictedControllerMessenger pattern getRestricted not inferring omitted Allowed{Actions,Events} as never when used with unrestrictedControllerMessenger pattern
getRestricted not inferring omitted Allowed{Actions,Events} as never when used with unrestrictedControllerMessenger pattern getRestricted not inferring omitted allowlists as empty
getRestricted not inferring omitted allowlists as emptygetRestricted not inferring allowlists as empty, when they are omitted in both function and generic arguments
…and function params and assigning directly to `messenger` in controller constructor prevents `Allowed{Action,Event}` generic defaults (`never`) from being applied. Assigning restricted messenger to variable first fixes issue.
522ece0 to
142cc22
Compare
…vents}` are required params
getRestricted not inferring allowlists as empty, when they are omitted in both function and generic argumentsallowed{Actions,Events} required parameters of getRestricted
…ctions,Events}` were omitted
… param is omitted
0a9af45 to
8fcaf68
Compare
| name, | ||
| allowedActions: [], | ||
| allowedEvents: [], | ||
| }) as RateLimitMessenger<RateLimitedApis>; |
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.
Can this be removed now? I noticed you removed some type casts in other files.
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.
Good catch! I missed that one. Removed here: 9cdcbf1
8fcaf68 to
9cdcbf1
Compare
Gudahtt
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!
|
@metamaskbot publish-preview |
Explanation
This commit implements a fundamental solution to aligning runtime and type-level behavior for
getRestricted: removing the option to omit its function parameters altogether.Impact
The downsides of this approach are a bit more redundant code and inconvenience, but having to write an extra empty array or two doesn't seem like a terrible cost for getting unambiguous, explicit behavior.
Caveat
Note that this solution does not directly resolve the observed bug that motivated this ticket. It does make the bug irrelevant for
getRestricted, because passing in both allowlist params fixes the issue, but in general, there may still be cases where generic default arguments fail to apply.See #4033 for repro steps and more details on this bug.
Overview
References
getRestrictednot inferring allowlists as empty, when they are omitted in both function and generic arguments #4033Changelog
@metamask/base-controllerChanged
getRestrictedmethod of theControllerMessengerclass now expects bothallowedActionsandallowedEventsas required parameters.Checklist