[Bug]: NFT disappears after killing and relaunching the app #10057
Assignees
Labels
regression-RC-7.26.0
release-7.26.0
Issue or pull request that will be included in release 7.26.0
release-7.26.1
Issue or pull request that will be included in release 7.26.1
release-7.27.0
Issue or pull request that will be included in release 7.27.0
release-blocker
This bug is blocking the next release
Sev1-high
An issue that may have caused fund loss or access to wallet in the past & may still be ongoing
team-wallet-framework
type-bug
Something isn't working
Comments
cortisiko
added
type-bug
7 tasks
cortisiko
added
team-wallet-framework
Sev1-high
Gudahtt
added a commit
that referenced
this issue
Jun 27, 2024
We were failing to properly restore persisted state for some BaseControllerV1 controllers. This resulted in state being lost when the application was restarted. The only confirmed affected controller so far is the `NftController`. The context here is that for some older controllers, rather than passing the initial state into the controller directly when it is constructed, we call the `update` function directly. This has always been a bad practice (controllers should update their own state), and we're forced to fix this for controllers that we update to `BaseControllerV2` (because directly updating state like this is no longer possible. But this method of rehydrating controller state was still relied upon in some cases. This was broken recently in #9570 when a condition was added to fix a type error. The condition was meant to check that the controller had a `subscribe` function. Unfortunately this `hasProperty` check only looks at owned properties, not inherited properties, and the `subscribe` function was inherited from the base class. It has been updated to use the `in` operator instead, which does look up the entire prototype chain. Fixes #10057
7 tasks
Gudahtt
added a commit
that referenced
this issue
Jun 27, 2024
We were failing to properly restore persisted state for some BaseControllerV1 controllers. This resulted in state being lost when the application was restarted. The only confirmed affected controller so far is the `NftController`. The context here is that for some older controllers, rather than passing the initial state into the controller directly when it is constructed, we call the `update` function directly. This has always been a bad practice (controllers should update their own state), and we're forced to fix this for controllers that we update to `BaseControllerV2` (because directly updating state like this is no longer possible. But this method of rehydrating controller state was still relied upon in some cases. This was broken recently in #9570 when a condition was added to fix a type error. The condition was meant to check that the controller had a `subscribe` function. Unfortunately this `hasProperty` check only looks at owned properties, not inherited properties, and the `subscribe` function was inherited from the base class. It has been updated to use the `in` operator instead, which does look up the entire prototype chain. Fixes #10057
metamaskbot
added
the
release-7.27.0
metamaskbot
pushed a commit
that referenced
this issue
Jun 27, 2024
<!-- 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. --> ## **Description** We were failing to properly restore persisted state for some BaseControllerV1 controllers. This resulted in state being lost when the application was restarted. The only confirmed affected controller so far is the `NftController`. The context here is that for some older controllers, rather than passing the initial state into the controller directly when it is constructed, we call the `update` function directly. This has always been a bad practice (controllers should update their own state), and we're forced to fix this for controllers that we update to `BaseControllerV2` (because directly updating state like this is no longer possible. But this method of rehydrating controller state was still relied upon in some cases. This was broken recently in #9570 when a condition was added to fix a type error. The condition was meant to check that the controller had a `subscribe` function. Unfortunately this `hasProperty` check only looks at owned properties, not inherited properties, and the `subscribe` function was inherited from the base class. It has been updated to use the `in` operator instead, which does look up the entire prototype chain. ## **Related issues** Fixes #10057 ## **Manual testing steps** See #10057 ## **Screenshots/Recordings** ### **Before** See #10057 ### **After** https://github.com/MetaMask/metamask-mobile/assets/2459287/b7cf3e09-086b-4964-8180-e58195969e17 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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.
metamaskbot
added
the
release-7.26.0
|
Missing release label release-7.26.0 on issue. Adding release label release-7.26.0 on issue, as issue is linked to PR #10133 which has this release label. |
Jonathansoufer
pushed a commit
that referenced
this issue
Jun 27, 2024
<!-- 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. --> ## **Description** We were failing to properly restore persisted state for some BaseControllerV1 controllers. This resulted in state being lost when the application was restarted. The only confirmed affected controller so far is the `NftController`. The context here is that for some older controllers, rather than passing the initial state into the controller directly when it is constructed, we call the `update` function directly. This has always been a bad practice (controllers should update their own state), and we're forced to fix this for controllers that we update to `BaseControllerV2` (because directly updating state like this is no longer possible. But this method of rehydrating controller state was still relied upon in some cases. This was broken recently in #9570 when a condition was added to fix a type error. The condition was meant to check that the controller had a `subscribe` function. Unfortunately this `hasProperty` check only looks at owned properties, not inherited properties, and the `subscribe` function was inherited from the base class. It has been updated to use the `in` operator instead, which does look up the entire prototype chain. ## **Related issues** Fixes #10057 ## **Manual testing steps** See #10057 ## **Screenshots/Recordings** ### **Before** See #10057 ### **After** https://github.com/MetaMask/metamask-mobile/assets/2459287/b7cf3e09-086b-4964-8180-e58195969e17 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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.
metamaskbot
added
the
release-7.26.1
|
Missing release label release-7.26.1 on issue. Adding release label release-7.26.1 on issue, as issue is linked to PR #10133 which has this release label. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
There is currently a bug on main and the release relating to NFTs disappearing. The bug happens when you:
manually import an NFT
Kill and relaunch the app
Upon landing on the wallet, the NFT disappears.
Expected behavior
NFTs should not disappear from your account when you kill and relaunch the app
Screenshots/Recordings
Screen.Cast.2024-06-20.at.8.09.44.PM.mp4
Steps to reproduce
Import wallet that has NFTs
Manually import the NFT do not use NFT auto detection
Kill and relaunch the app
Notice the NFT is no longer in your wallet
Error messages or log output
No response
Version
7.26 RC
Build type
None
Device
iOS and Android
Operating system
Android
Additional context
Credits to Sahara for going back in the commit history to reproduce:
On commit: 2384af6 => I can reproduce this bug => PR: https://github.com/MetaMask/metamask-mobile/pull/9570/files
On commit 5db49f1 => I cannot reproduce this bug
Severity
No response
The text was updated successfully, but these errors were encountered: