feat: remove PermissionLogController in favor of core implementation #23182
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
|
@metamaskbot update-policies |
|
Policies updated |
There was a problem hiding this comment.
Is it safe to do this? Could we remove this file in a later PR so that we can first verify whether the core controller passes all existing extension-side tests, and if not, which tests break?
There was a problem hiding this comment.
I did manual testing I also explained how to test it on Manual testing steps, let me know if this is enough
There was a problem hiding this comment.
I see that this test file was migrated to core as part of the first step: MetaMask/core#1871. Looks like my concerns weren't warranted. I'm good with moving forward on this.
|
Could we put together a list of breaking changes caused by replacing the extension implementation with the core implementation? Extension users need to be able to quickly reference the API changes introduced by this PR. Here's an example I'm working on: #22928 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #23182 +/- ##
===========================================
+ Coverage 68.64% 68.65% +0.01%
===========================================
Files 1099 1098 -1
Lines 43362 43185 -177
Branches 11575 11532 -43
===========================================
- Hits 29764 29647 -117
+ Misses 13598 13538 -60 ☔ View full report in Codecov by Sentry. |
I updated the PR Changes with all the work that has been done on this subject, let me know if that is okay regarding your remark ? |
Builds ready [1b10c72]
Page Load Metrics (1028 ± 410 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [2253a0c]
Page Load Metrics (1075 ± 433 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [406b162]
Page Load Metrics (1439 ± 441 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
@metamaskbot update-policies |
|
Policies updated |
Builds ready [95418a8]
Page Load Metrics (931 ± 402 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [95418a8]
Page Load Metrics (1201 ± 413 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [f312b53]
Page Load Metrics (725 ± 434 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
@cryptodev-2s I think we should have an extension user-facing changelog here that leaves out implementation details and only describes the API changes between Looks like the |
yes exactly the PR MetaMask/core#3662. covers all the changes made on the controller that the extension team should be aware of. |
Builds ready [ad80c30]
Page Load Metrics (929 ± 506 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [2255058]
Page Load Metrics (1033 ± 417 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
I'm not sure what's going on with CI. I don't think it's related to these changes. I think it's being fixed by #23249. |
Builds ready [5cd6baa]
Page Load Metrics (1252 ± 375 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-11.13.0
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.
Related issues
Changes
The transition of this controller from the extension repo to the core monorepo unfolded in three phases:
Manual testing steps
These instructions outline the process for conducting manual testing locally.
chrome.storage.local.get(null, ({data}) => console.log(data.PermissionLogController))in the console.feature/23181-remove-Permissionlogcontroller.permissionHistoryandpermissionActivityLogfrom the initial run in the second run's output. Note that the log history is limited to 100 entries.Pre-merge author checklist
Pre-merge reviewer checklist