Skip to content

Conversation

@danjm
Copy link
Contributor

@danjm danjm commented Feb 16, 2024

fix: fix selectedAddress and nftController instantiation (#22856)

Noticed that when you freshly import the extension, usse testDapp to deploy and mint an NFT, then manually import the NFT. you will be able to see the NFT. Then when you switch to another account and go back to the account that has the NFT;
1- you wont be able to see the NFTs you imported earlier 2- When you try to import them again you will get an error (fired from core when trying to verify ownership)

Noticed that this was due to core having the wrong userAddress to check ownership for, and then in metamask.js, the selectedAddress retrieved does not match the user's selected Address

This behavior is also reported to be on v11.10.0, on this issue: #22796

Fixes: #22796
Fixes: #22798

  1. Remove and reimport the extension
  2. Use testDapp to deploy and mint the NFT
  3. Add the NFT manually by clicking "import NFT"
  4. You should be able to see your newly import NFT
  5. Create new account
  6. Go back to the account that has the NFT
  7. You should be able to see the NFT you imported
devBefore.mov
DevAfter.mov
  • I’ve followed MetaMask Coding Standards.

  • I've clearly explained what problem this PR is solving and how it is solved.

  • I've linked related issues

  • 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 format if applicable

  • I’ve applied the right labels on the PR (see labeling guidelines). 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".

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


Description

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • 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 format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

@danjm danjm requested a review from a team as a code owner February 16, 2024 18:57
@github-actions
Copy link
Contributor

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.

@danjm
Copy link
Contributor Author

danjm commented Feb 16, 2024

I QA'd and confirmed that the bugs this commit fixed are still fixed on this cherry-pick

@danjm danjm force-pushed the cherry-pick-167036f branch from b16f180 to d11b190 Compare February 16, 2024 21:25
fix: fix selectedAddress and nftController instantiation (#22856)

Noticed that when you freshly import the extension, usse testDapp to
deploy and mint an NFT, then manually import the NFT.
you will be able to see the NFT. Then when you switch to another account
and go back to the account that has the NFT;
1- you wont be able to see the NFTs you imported earlier
2- When you try to import them again you will get an error (fired from
core when trying to verify ownership)

Noticed that this was due to core having the wrong userAddress to check
ownership for, and then in metamask.js, the selectedAddress retrieved
does not match the user's selected Address

This behavior is also reported to be on v11.10.0, on this issue:
#22796

Fixes: #22796
Fixes: #22798

1. Remove and reimport the extension
2. Use testDapp to deploy and mint the NFT
3. Add the NFT manually by clicking "import NFT"
4. You should be able to see your newly import NFT
5. Create new account
6. Go back to the account that has the NFT
7. You should be able to see the NFT you imported

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

https://github.com/MetaMask/metamask-extension/assets/10994169/a74cbf0b-014b-4e39-82ac-e8452941fc6e

https://github.com/MetaMask/metamask-extension/assets/10994169/987dbb27-b938-4bce-9b43-4269d714ee33

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] 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".

- [ ] 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: Monte Lai <monte.lai@consensys.net>
Co-authored-by: Dan J Miller <danjm.com@gmail.com>
@danjm danjm force-pushed the cherry-pick-167036f branch from d11b190 to ef0deaf Compare February 16, 2024 22:09
@codecov
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c2d8a9b) 68.03% compared to head (ef0deaf) 68.03%.

Files Patch % Lines
app/scripts/metamask-controller.js 50.00% 4 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           Version-v11.10.0   #23023      +/-   ##
====================================================
- Coverage             68.03%   68.03%   -0.01%     
====================================================
  Files                  1087     1087              
  Lines                 42892    42910      +18     
  Branches              11406    11411       +5     
====================================================
+ Hits                  29181    29190       +9     
- Misses                13711    13720       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danjm danjm merged commit dcaec4d into Version-v11.10.0 Feb 16, 2024
@danjm danjm deleted the cherry-pick-167036f branch February 16, 2024 22:36
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [ef0deaf]
Page Load Metrics (796 ± 22 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89154112168
domContentLoaded9501894
load7489457964722
domInteractive9501894

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants