Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Network Controller refactor to use the same selector #5529

Merged
merged 28 commits into from
Mar 1, 2023
Merged

Network Controller refactor to use the same selector #5529

merged 28 commits into from
Mar 1, 2023

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Jan 17, 2023 •
edited

Description
Network controller needs to be refactored for be easier to upgrade on the future.

The controller packages have been updated to use the latest versions of each package. The only breaking change that affects mobile was the rename of the provider property of the network controller to providerConfig. Most changes in this commit relate to that change.

Proposed Solution
Now we use the reselect package of redux to memoise and improve performance, and also we have a dedicated file to export the Network controller attributes.

Test Cases

  • Overall use of the app (navigate between screens, change network, added and remove custom networks)
  • Dapp Transactions on MetaMask test dapp
  • Swap Ethereum to DAI on mainnet
  • Send flow transaction of goerli
  • Imported tokens on avalanche
  • Imported nfts on mainnet

Code impact
High - Everything network related

Screenshots/Recordings
https://recordit.co/00LpJxmclu

Issue

Progresses #5509

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@tommasini tommasini requested a review from a team as a code owner January 17, 2023 10:45
@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.

@tommasini tommasini added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 17, 2023
@tommasini tommasini removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 17, 2023
@tommasini tommasini marked this pull request as draft January 17, 2023 11:52
@tommasini tommasini added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 17, 2023
@tommasini tommasini marked this pull request as ready for review January 17, 2023 13:02
Gudahtt
Gudahtt approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@Gudahtt Gudahtt mentioned this pull request Jan 17, 2023
Merged
3 tasks
@Gudahtt
Update controller packages (#5532)
c57f912 
The controller packages have been updated to use the latest versions of
each package. The only breaking change that affects mobile was the
rename of the `provider` property of the network controller to
`providerConfig`. Most changes in this commit relate to that change.
@wachunei wachunei mentioned this pull request Jan 18, 2023
Closed
3 tasks
blackdevelopa
blackdevelopa approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

This looks good to me. I can't think of network instance that was missed. Do you think we should run this by QA? What do you think?

@tommasini
Copy link
Contributor Author

Thanks for the review!
Since there were major versions updates on some controllers package it would be good to do QA testing before merging it on my opinion

@tommasini tommasini added Mobile QA board needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jan 19, 2023
@Gudahtt

This comment was marked as resolved.

Sign in to view
Cal-L
Cal-L reviewed Feb 8, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left some comments! Mostly just simplifying the selectors

app/components/Nav/Main/index.js Outdated Show resolved Hide resolved
app/components/UI/AccountRightButton/index.tsx Outdated Show resolved Hide resolved
app/components/UI/NetworkInfo/index.tsx Outdated Show resolved Hide resolved
app/components/Views/Wallet/index.tsx Outdated Show resolved Hide resolved
app/components/hooks/useAccounts/useAccounts.ts Show resolved Hide resolved
app/components/hooks/useAccounts/useAccounts.ts Outdated Show resolved Hide resolved
@socket-security
Copy link

socket-security bot commented Feb 17, 2023 •
edited

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Powered by socket.dev

@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 17, 2023
cortisiko
cortisiko approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

I completed a thorough regression on this PR. This is QA passed! 🌮 🌮

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Mar 1, 2023
@tommasini tommasini merged commit b1877f3 into main Mar 1, 2023
@tommasini tommasini deleted the refactor/5509-refactor-network-controller branch March 1, 2023 23:11
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Cal-L Cal-L Cal-L left review comments

@Gudahtt Gudahtt Gudahtt approved these changes

@cortisiko cortisiko cortisiko approved these changes

@blackdevelopa blackdevelopa blackdevelopa approved these changes

Assignees
No one assigned
Labels
QA Passed A successful QA run through has been done release-6.2.0 team-mobile-platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@tommasini @Gudahtt @Cal-L @cortisiko @blackdevelopa @vpintorico @gauthierpetetin @sethkfman