Skip to content

refactor: update Nft Controllers to use selectedAccountId instead of selectedAddress - #4221

Merged
ccharly merged 10 commits into
mainfrom
fix/ap381/update-nft-controllers-to-use-internal-account
Jun 7, 2024
Merged

refactor: update Nft Controllers to use selectedAccountId instead of selectedAddress#4221
ccharly merged 10 commits into
mainfrom
fix/ap381/update-nft-controllers-to-use-internal-account

Conversation

@montelaidev

@montelaidevmontelaidev commented Apr 26, 2024

Copy link
Copy Markdown
Contributor

Explanation

This PR updates removes selectedAddress and uses the controller messenger to get InternalAccounts in the Nft Controllers

References

Fixeshttps://github.com/MetaMask/accounts-planning/issues/381

Changelog

@metamask/assets-controllers

  • BREAKING: NftController constructor argument selectedAddress has been removed.
  • BREAKING: NftController now requires AccountsControlelr:get{Account,SelectedAccount} messenger actions.
  • BREAKING: NftController now requires AccountsController:selectedEvmAccountChange event.
  • BREAKING: NftDetectionController now requires AccountsControlelr:getSelectedAccount messenger actions.

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

@montelaidev

Copy link
Copy Markdown
ContributorAuthor

@metamaskbot publish-preview

@github-actions

Copy link
Copy Markdown
Contributor

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
"@metamask-previews/accounts-controller": "13.0.0-preview-61ace68c",
"@metamask-previews/address-book-controller": "4.0.1-preview-61ace68c",
"@metamask-previews/announcement-controller": "6.1.0-preview-61ace68c",
"@metamask-previews/approval-controller": "6.0.1-preview-61ace68c",
"@metamask-previews/assets-controllers": "28.0.0-preview-61ace68c",
"@metamask-previews/base-controller": "5.0.1-preview-61ace68c",
"@metamask-previews/build-utils": "2.0.1-preview-61ace68c",
"@metamask-previews/composable-controller": "6.0.1-preview-61ace68c",
"@metamask-previews/controller-utils": "9.1.0-preview-61ace68c",
"@metamask-previews/ens-controller": "10.0.1-preview-61ace68c",
"@metamask-previews/eth-json-rpc-provider": "3.0.1-preview-61ace68c",
"@metamask-previews/gas-fee-controller": "15.0.0-preview-61ace68c",
"@metamask-previews/json-rpc-engine": "8.0.1-preview-61ace68c",
"@metamask-previews/json-rpc-middleware-stream": "7.0.1-preview-61ace68c",
"@metamask-previews/keyring-controller": "15.0.0-preview-61ace68c",
"@metamask-previews/logging-controller": "3.0.1-preview-61ace68c",
"@metamask-previews/message-manager": "8.0.1-preview-61ace68c",
"@metamask-previews/name-controller": "6.0.1-preview-61ace68c",
"@metamask-previews/network-controller": "18.1.0-preview-61ace68c",
"@metamask-previews/notification-controller": "5.0.1-preview-61ace68c",
"@metamask-previews/permission-controller": "9.0.2-preview-61ace68c",
"@metamask-previews/permission-log-controller": "2.0.1-preview-61ace68c",
"@metamask-previews/phishing-controller": "9.0.1-preview-61ace68c",
"@metamask-previews/polling-controller": "6.0.1-preview-61ace68c",
"@metamask-previews/preferences-controller": "10.0.0-preview-61ace68c",
"@metamask-previews/queued-request-controller": "0.9.0-preview-61ace68c",
"@metamask-previews/rate-limit-controller": "5.0.1-preview-61ace68c",
"@metamask-previews/selected-network-controller": "12.0.1-preview-61ace68c",
"@metamask-previews/signature-controller": "15.0.0-preview-61ace68c",
"@metamask-previews/transaction-controller": "28.1.0-preview-61ace68c",
"@metamask-previews/user-operation-controller": "8.0.1-preview-61ace68c"
}

@montelaidevmontelaidev changed the title fix: update Nft Controllers to use selectedAccountId instead of selectedAddressrefactor: update Nft Controllers to use selectedAccountId instead of selectedAddressJun 5, 2024
@montelaidev
montelaidevforce-pushed the fix/ap381/update-nft-controllers-to-use-internal-account branch from 61ace68 to dd6e3e8CompareJune 5, 2024 12:33
@montelaidev
montelaidevforce-pushed the fix/ap381/update-nft-controllers-to-use-internal-account branch from dd6e3e8 to 42bbf96CompareJune 5, 2024 12:42
@montelaidev
montelaidev marked this pull request as ready for review June 5, 2024 12:43
@montelaidev
montelaidev requested review from a teamJune 5, 2024 12:43
Comment threadpackages/assets-controllers/src/NftController.test.ts
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.ts Outdated
constructor({
chainId: initialChainId,
selectedAddress = '',
selectedAccountId = '',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similar comment as here — what are your thoughts on using the messenger to get this value from the AccountsController a bit later on in this constructor instead of having the constructor take this argument?

Comment threadpackages/assets-controllers/src/NftDetectionController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftDetectionController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftDetectionController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftDetectionController.test.ts Outdated
Comment threadpackages/assets-controllers/src/NftDetectionController.test.ts Outdated
@ccharly
ccharly merged commit 4149d47 into mainJun 7, 2024
@ccharly
ccharly deleted the fix/ap381/update-nft-controllers-to-use-internal-account branch June 7, 2024 14:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@montelaidev@mcmire@ccharly