Skip to content

Import permission log controller#1871

Merged
cryptodev-2s merged 38 commits intomainfrom
import-permission-log-controller
Nov 7, 2023
Merged

Import permission log controller#1871
cryptodev-2s merged 38 commits intomainfrom
import-permission-log-controller

Conversation

@cryptodev-2s
Copy link
Copy Markdown
Contributor

Description

This PR adds the permission-log-controller from metamask-extension.

Changes

  • ADDED: permission-log-controller controller

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@cryptodev-2s cryptodev-2s requested a review from a team as a code owner October 19, 2023 15:08
@cryptodev-2s cryptodev-2s marked this pull request as draft October 19, 2023 15:08
@socket-security
Copy link
Copy Markdown

socket-security bot commented Oct 19, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@socket-security
Copy link
Copy Markdown

socket-security bot commented Oct 19, 2023

👍 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.

@cryptodev-2s cryptodev-2s marked this pull request as ready for review October 24, 2023 16:46
@cryptodev-2s cryptodev-2s force-pushed the import-permission-log-controller branch from 5a84f3e to 41c71c3 Compare October 24, 2023 17:58
@MajorLift
Copy link
Copy Markdown
Contributor

I made a small PR with type fixes! #1908

@cryptodev-2s cryptodev-2s force-pushed the import-permission-log-controller branch from 735aec1 to 5e3950f Compare October 24, 2023 22:06
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cryptodev-2s cryptodev-2s requested a review from mcmire November 7, 2023 14:54
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more things. We're getting close!

@mcmire
Copy link
Copy Markdown
Contributor

mcmire commented Nov 7, 2023

Ah, looks like there are merge conflicts now. Sorry, I will try to get to this faster 😞

@cryptodev-2s cryptodev-2s requested a review from mcmire November 7, 2023 19:37
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay! This looks good to me!

@cryptodev-2s cryptodev-2s merged commit e56b975 into main Nov 7, 2023
@cryptodev-2s cryptodev-2s deleted the import-permission-log-controller branch November 7, 2023 21:56
cryptodev-2s added a commit to MetaMask/metamask-extension that referenced this pull request Mar 1, 2024
…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.


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-wallet-framework Deprecated: Please use `team-core-platform` instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copy TypeScript version of PermissionLogController (from extension) along with tests into this repo

3 participants