feat: simplify permission log controller#3662
Conversation
b2b21c1 to
18c4e89
Compare
mikesposito
left a comment
There was a problem hiding this comment.
Nice to see that we reduced the methods exposed by the controller, but it would be helpful to document the reasoning behind these changes, as it will help review them and also understand how we imagine this controller to be used by clients after these changes are released.
I see that unit tests have been adapted to the new API: I guess that this PR also covers this issue?
Thanks for bringing this Michele I just updated the PR Explanation description. |
@mikesposito That's a separate ticket that's designed to bring the unit tests up to our current standards. If we take care of some of that in this PR because we need to, that's fine, but I'm not sure we should close that PR as a part of this, if that's what you were thinking. |
mcmire
left a comment
There was a problem hiding this comment.
Great job! I reviewed the extension and I think limiting the controller to just the two methods is the right call. A lot of the comments I had below were pretty minor.
packages/permission-log-controller/src/PermissionLogController.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/src/PermissionLogController.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
I definitely agree that they should be addressed separately: As tests have been partially refactored already, I was curious to know if there was additional refactoring planned. So I agree that we should keep #1827 open |
|
Looks like there is not yet any description of changes in the PR description, or in the changelog itself. Could you describe the changes in one of these two places? |
mcmire
left a comment
There was a problem hiding this comment.
Looks good! I agree with Mark. Perhaps best to update the PR description so it doesn't clear existing approvals.
…23182) ## **Description** Following the successful migration of PermissionLogController to the core monorepo (MetaMask/core#1826), this commit removes the redundant PermissionLogController logic from the extension. All future developments and maintenance will be concentrated on the core implementation to streamline efforts and enhance functionality coherence across the platform. [](https://codespaces.new/MetaMask/metamask-extension/pull/23182?quickstart=1) ## **Related issues** - Fixes: 23181 ## **Changes** The transition of this controller from the extension repo to the core monorepo unfolded in three phases: 1. The controller was integrated into Core, with more information available at MetaMask/core#1871 2. The logic of the controller was streamlined, with additional details at MetaMask/core#3662 3. The tests for the controller were overhauled, with further information at MetaMask/core#3937 ## **Manual testing steps** These instructions outline the process for conducting manual testing locally. 1. Launch the extension from the latest development branch. 2. Navigate to the [test-dapp](https://metamask.github.io/test-dapp/). 3. Initiate the REQUEST_PERMISSIONS action from the Permissions Actions menu. 4. Open the background.html inspect window. 5. Execute the script `chrome.storage.local.get(null, ({data}) => console.log(data.PermissionLogController))` in the console. 6. Record the output from the previous step. 7. Switch to the branch named `feature/23181-remove-Permissionlogcontroller`. 8. Repeat steps 2 through 6 for this branch. 9. Compare the outputs from step 6 for both the development and feature branches. Look for matching entries in `permissionHistory` and `permissionActivityLog` from the initial run in the second run's output. Note that the log history is limited to 100 entries. ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Explanation
Initially, I took the PermissionLogController from the extension and converted it to TypeScript. During my work on this simplification task, I realized that the primary functions in use are
createMiddleware, for logging permission activities and history, andupdateAccountsHistory, which is triggered on account changes. The controller also tracks two state-level properties:permissionHistory, used on the extension's permissions page, andpermissionActivityLog, which is currently not in use but retained for an upcoming feature. Additionally, the rest of the functions in the PermissionLogController are private, serving as helper functions.Changes
Kept public
createMiddleware.updateAccountsHistory.Changed to private
logRequest: helper function only used within the controller.logResponse: helper function only used within the controller.logPermissionsHistory: helper function only used within the controller.commitNewHistory: helper function only used within the controller.getRequestedMethods: helper function only used within the controller.getAccountsFromPermission: helper function only used within the controller.getAccountToTimeMap: included in the controller instead of outside controller class as a private helper function.Removed
sinondependency: removed in favor of builtin jest fake timers.getActivityLog: originally returns the state of permissionActivityLog, but now state can be accessed with state property directly.updateActivityLog: originally updates the state for permissionActivityLog, replaced by this.update already available on the controller.getHistory: originally returns the state of PermissionHistory, but now state can be accessed with state property directlyupdateHistory: originally updates the state for PermissionHistory, replaced by this.update already available on the controller.References
Checklist