Skip to content

feat: simplify permission log controller#3662

Merged
cryptodev-2s merged 8 commits intomainfrom
simplify-permission-log-controller
Dec 20, 2023
Merged

feat: simplify permission log controller#3662
cryptodev-2s merged 8 commits intomainfrom
simplify-permission-log-controller

Conversation

@cryptodev-2s
Copy link
Copy Markdown
Contributor

@cryptodev-2s cryptodev-2s commented Dec 13, 2023

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, and updateAccountsHistory, which is triggered on account changes. The controller also tracks two state-level properties: permissionHistory, used on the extension's permissions page, and permissionActivityLog, 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

  • sinon dependency: 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 directly
  • updateHistory: originally updates the state for PermissionHistory, replaced by this.update already available on the controller.

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@cryptodev-2s cryptodev-2s added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Dec 13, 2023
@cryptodev-2s cryptodev-2s self-assigned this Dec 13, 2023
@cryptodev-2s cryptodev-2s requested a review from a team as a code owner December 13, 2023 18:51
@cryptodev-2s cryptodev-2s force-pushed the simplify-permission-log-controller branch from b2b21c1 to 18c4e89 Compare December 14, 2023 10:28
Copy link
Copy Markdown
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

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?

@cryptodev-2s
Copy link
Copy Markdown
Contributor Author

cryptodev-2s commented Dec 14, 2023

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.

@cryptodev-2s cryptodev-2s reopened this Dec 14, 2023
@mcmire
Copy link
Copy Markdown
Contributor

mcmire commented Dec 15, 2023

I see that unit tests have been adapted to the new API: I guess that this PR also covers this issue?

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

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.

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.

@mikesposito
Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Dec 19, 2023

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?

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.

Looks good! I agree with Mark. Perhaps best to update the PR description so it doesn't clear existing approvals.

@cryptodev-2s cryptodev-2s merged commit 509e580 into main Dec 20, 2023
@cryptodev-2s cryptodev-2s deleted the simplify-permission-log-controller branch December 20, 2023 09:55
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.

Simplify PermissionLogController API

4 participants