chore(cherry-pick):759b92e to 12.1.1 #26802
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. |
montelaidev
changed the title
|
👍 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. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
montelaidev
changed the title
montelaidev
changed the title
…Account (#26573) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> This PR bumps the `AccountsController` and introduces a new migration. The `updateAccounts` methods from the `AccountsController` now checks if the selectedAccount is undefined and recovers from this. The migration updates the selectedAccount values that are not defined. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/26573?quickstart=1) Fixes: #26377 1. Go to this page... 2. 3. <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- [screenshots/recordings] --> <!-- [screenshots/recordings] --> - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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 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 <[email protected]>
montelaidev
force-pushed
the
cherry-pick-b9173da-759b92e-to-12.1.1
branch
from
088a344 to
ab36239
Compare
|
@metamaskbot update-policies |
|
Policies updated |
|
@SocketSecurity ignore npm/@metamask/[email protected] |
Builds ready [337ffaa]
Page Load Metrics (408 ± 323 ms)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## Version-v12.1.1 #26802 +/- ##
================================================
Coverage 69.83% 69.84%
================================================
Files 1371 1371
Lines 48789 48791 +2
Branches 13452 13453 +1
================================================
+ Hits 34071 34074 +3
+ Misses 14718 14717 -1 ☔ View full report in Codecov by Sentry. |
| @@ -24,7 +25,7 @@ export const SnapAccountCard = ({ | |||
| const account = mergedAccounts.find( | |||
| (internalAccount: { address: string }) => | |||
| internalAccount.address === address, | |||
| ); | |||
| ) as MergedInternalAccount; | |||
There was a problem hiding this comment.
It looks like this is overriding a TypeScript error that is flagging the possibility of account being undefined. How do we know that undefined is impossible in this case?
There was a problem hiding this comment.
If we show the removal of a snap account, it would mean the account is defined.
There was a problem hiding this comment.
What would happen if the account was removed in another window while this was open?
There was a problem hiding this comment.
There might still be a crash here, but probably it would have a minimal impact because it doesn't really make sense to remove an account that doesn't exist. I'll approve for now, but it might be a good idea to follow up here to improve how this missing account scenario is handled
| const mergedAccounts = mergeAccounts( | ||
| connectedAccounts, | ||
| internalAccounts, | ||
| ) as AccountType[]; |
There was a problem hiding this comment.
Non-blocking comment: This type assertion does not appear to be necessary, but at least it's not overriding any valid type errors. Easiest to leave this as-is for the cherry-pick, but please avoid the use of type assertions in the future. See here for details on why, and about alternatives: https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md#type-assertions
There was a problem hiding this comment.
I think linter was complaining when the pr was made. This type comes from this line
There was a problem hiding this comment.
Right, but that line is using the wrong type (it's using AccountType, but this is a MergedAccountType). If the type is corrected, the error goes away. TypeScript is correct here.
There was a problem hiding this comment.
In general, we should never use type assertions like this. Even in the scenarios where TypeScript lacks the necessary type information and there is no good way to provide it, there are better ways to "override" the type (type guards, the satisfies operator, type annotations, etc.`). The linked document has plenty more details. I'd encourage you to review it, and follow up with any questions you have about it.
But in this specific scenario, I don't think any override is needed
](https://codespaces.new/MetaMask/metamask-extension/pull/26573?quickstart=1){kind=link}
Description
This pr cherry picks the 759b92e.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist