Conversation
|
No top level dependency changes detected. Learn more about Socket for GitHub ↗︎ |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
5a84f3e to
41c71c3
Compare
|
I made a small PR with type fixes! #1908 |
735aec1 to
5e3950f
Compare
mcmire
left a comment
There was a problem hiding this comment.
It's definitely not easy to fit JavaScript code into TypeScript so nice job on converting this! We definitely have our work cut out for us when it comes time to revise the way this controller works.
I reviewed the implementation file but didn't get all the way through the test file. That said I think I've probably given you enough for now 😅
There was a problem hiding this comment.
Hey, I finally managed to come back to this PR and finish a review 😅 It looks like there were some leftover comments from my previous review that got washed away with a rebase. I copied those back again, and left some new ones.
One thing I noticed (and I am marking this as "request changes" to make sure we do this) is that this package doesn't have an index.ts file, so no one will be able to use the package until that's added (for context, this is the file that Node will load when you import the package). Can you add this? You may wish to add a test for the index to make sure that it exports the packages that you expect. eth-json-rpc-provider has one here: https://github.com/MetaMask/core/blob/8d5cd478c5cbdc699a9061643c5137054708506c/packages/eth-json-rpc-provider/src/index.test.ts
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/src/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/src/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/src/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
mcmire
left a comment
There was a problem hiding this comment.
A couple more things. We're getting close!
packages/permission-log-controller/tests/PermissionLogController.test.ts
Show resolved
Hide resolved
|
Ah, looks like there are merge conflicts now. Sorry, I will try to get to this faster 😞 |
mcmire
left a comment
There was a problem hiding this comment.
Okay! This looks good to me!
…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>
Description
This PR adds the
permission-log-controllerfrom metamask-extension.Changes
permission-log-controllercontrollerReferences
Checklist