feat: Add origin throttling modal #29656
Conversation
| } | ||
|
|
||
| export default function createOriginThrottlingMiddleware({ | ||
| appStateController, |
There was a problem hiding this comment.
Can we try and encapsulate as much logic as possible inside this middleware for the sake of maintainability and testing?
As this is constructed from the MetamaskController, can we define callbacks for anything we need such as:
onApprovalAcceptonApprovalRejectupdateThrottledOriginsgetThrottledOrigins
| return; | ||
| } | ||
|
|
||
| next(); |
There was a problem hiding this comment.
What do we gain by subscribing to the approval controller events if we already have the means to check all the request responses via a callback provided to next similar to what we do in app/scripts/lib/createRPCMethodTrackingMiddleware.js?
Could we just check the result error and update the throttled state via our callbacks?
It would reduce coupling between this middleware and the controllers and further modularise all this throttling functionality?
There was a problem hiding this comment.
Removed approval controller events, fixed all in middleware
| export function validateOriginThrottling({ | ||
| req, | ||
| end, | ||
| appStateController, |
There was a problem hiding this comment.
Is it worth a more specific callback to avoid coupling to the controller directly?
| 'Request blocked due to spam filter.', | ||
| ); | ||
|
|
||
| export function validateOriginThrottling({ |
There was a problem hiding this comment.
Is this function necessary if it's only used internally and encapsulates a call to a single function? Would it add readability to the main middleware to move it inline?
There was a problem hiding this comment.
If we're able to encapsulate all the logic in the middleware, should these constants be moved there if there's no other usages of them?
There was a problem hiding this comment.
These constants still be shared between UI and background. Not all of them(BLOCKABLE_METHODS used in background only) but is it make sense to have them together under shared?
|
|
||
| return ( | ||
| <PageFooter className="confirm-footer_page-footer"> | ||
| <OriginThrottleModal | ||
| isOpen={showOriginThrottleModal} | ||
| onConfirmationCancel={onFooterCancel} |
There was a problem hiding this comment.
Rather than relying on forceCancel, could we instead just use onCancel here or a wrapper to avoid duplicating the location property?
There was a problem hiding this comment.
I am not sure if I follow, can you elaborate on this?
| <ModalFooter | ||
| paddingTop={4} | ||
| onSubmit={() => { | ||
| // Order of operations is important here to ensure the origin is reset after the confirmation is cancelled |
There was a problem hiding this comment.
Minor, is it worth storybooks for this component?
| return { | ||
| origin, | ||
| resetOrigin, | ||
| willNextRejectionReachThreshold: |
There was a problem hiding this comment.
Minor, could this be defined above in a variable to avoid the inline logic?
| return { | ||
| origin, | ||
| resetOrigin, | ||
| willNextRejectionReachThreshold: |
There was a problem hiding this comment.
Should we call this something like shouldThrottleOrigin to be explicit and also include some extra context for the external usages?
| return callback(); | ||
| } | ||
|
|
||
| if ('error' in res && res.error && isUserRejectedError(res.error)) { |
There was a problem hiding this comment.
in operator here is to satisfy type check on response
Description
This PR brings origin throttling modal functionality which is done in the mobile MetaMask/metamask-mobile#10082
The PR also needs an
ApprovalControllerto be updated in order to subscribe theacceptedandrejectedapprovals to handle user rejected approvals.Core PR: MetaMask/core#5127
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3344
First mobile task: https://github.com/MetaMask/MetaMask-planning/issues/1822
Manual testing steps
Screenshots/Recordings
origin.throttling.modal.mov
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist