Skip to content

Implement resetConnection method in network controller - #1131

Merged
cryptodev-2s merged 4 commits into
mainfrom
feature/reset-connection
Apr 6, 2023
Merged

Implement resetConnection method in network controller#1131
cryptodev-2s merged 4 commits into
mainfrom
feature/reset-connection

Conversation

@cryptodev-2s

@cryptodev-2scryptodev-2s commented Mar 15, 2023

Copy link
Copy Markdown
Contributor

This PR adds a resetConnection method.
Related issue #1023

@cryptodev-2s
cryptodev-2s requested a review from a team as a code ownerMarch 15, 2023 18:57
@cryptodev-2scryptodev-2s changed the title [draft]: implement reset connection in network controller[draft]: implement resetConnection method in network controllerMar 15, 2023
mikesposito
mikesposito reviewed Mar 16, 2023
View reviewed changes
Comment threadpackages/network-controller/src/NetworkController.ts Outdated
@cryptodev-2s
cryptodev-2sforce-pushed the feature/reset-connection branch from 5b8b7bc to 00ed357CompareMarch 17, 2023 23:42
mcmire
mcmire requested changes Mar 21, 2023
View reviewed changes

@mcmiremcmire left a comment •
edited
Loading

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.

We should add tests for resetConnection. We have existing tests for this on the extension side here: https://github.com/MetaMask/metamask-extension/blob/b9f00ba5365cf92429c45f4ad05ced7373f726f5/app/scripts/controllers/network/network-controller.test.js#L3535. Note, however, that due to the differences in implementation between the extension and core versions of the network controller as well as the network controller tests, you will probably have to make some modifications to these tests when bringing them over.

@mcmire

mcmire commented Mar 21, 2023

Copy link
Copy Markdown
Collaborator

By the way, @cryptodev-2s, if you use "Fixes" instead of "Related issue" in your PR description, it will associate this PR with the original issue and when you merge this PR it will close the associated issue automatically. Otherwise we have to go back later and manually close issues.

@mcmiremcmire linked an issue Mar 21, 2023 that may be closed by this pull request
NetworkController API normalization: Add resetConnection method #1023
Closed
4 tasks
@cryptodev-2scryptodev-2s changed the title [draft]: implement resetConnection method in network controllerimplement resetConnection method in network controllerMar 30, 2023
@cryptodev-2scryptodev-2s changed the title implement resetConnection method in network controllerImplement resetConnection method in network controllerMar 30, 2023
Gudahtt
Gudahtt previously approved these changes Mar 30, 2023
View reviewed changes

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@mcmire

mcmire commented Mar 30, 2023•
edited
Loading

Copy link
Copy Markdown
Collaborator

We still ought to have tests for the resetConnection method, as there are none at this time. Also, there seem to be extra changes borrowed from #1133.

@Gudahtt
Gudahtt dismissed their stale reviewMarch 30, 2023 22:26

Missing tests

@cryptodev-2s
cryptodev-2sforce-pushed the feature/reset-connection branch from c35f242 to 5edb863CompareApril 3, 2023 15:08
mcmire
mcmire reviewed Apr 3, 2023
View reviewed changes

@mcmiremcmire left a comment

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.

This is getting there! Just a few more changes needed.

Comment threadpackages/network-controller/tests/NetworkController.test.ts
Comment threadpackages/network-controller/tests/NetworkController.test.ts
Comment threadpackages/network-controller/tests/NetworkController.test.ts Outdated
Comment threadpackages/network-controller/tests/NetworkController.test.ts
@cryptodev-2s
cryptodev-2sforce-pushed the feature/reset-connection branch 2 times, most recently from 95d242e to 53fab33CompareApril 6, 2023 15:16
@cryptodev-2s
cryptodev-2sforce-pushed the feature/reset-connection branch from 078de49 to 4104f40CompareApril 6, 2023 17:00
mcmire
mcmire previously approved these changes Apr 6, 2023
View reviewed changes

@mcmiremcmire left a comment

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.

Two nits, but I'm happy with this!

Comment threadpackages/network-controller/tests/NetworkController.test.ts Outdated
Comment threadpackages/network-controller/tests/NetworkController.test.ts Outdated
@cryptodev-2s
cryptodev-2s requested a review from mcmireApril 6, 2023 17:36
mcmire
mcmire approved these changes Apr 6, 2023
View reviewed changes

@mcmiremcmire left a comment

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.

Looks great!

@cryptodev-2s
cryptodev-2s merged commit cb43d9e into mainApr 6, 2023
@cryptodev-2s
cryptodev-2s deleted the feature/reset-connection branch April 6, 2023 17:42
@legobeatlegobeat mentioned this pull request Apr 25, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcmiremcmiremcmire approved these changes
@mikespositomikespositoAwaiting requested review from mikesposito
@GudahttGudahttAwaiting requested review from Gudahtt

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NetworkController API normalization: Add resetConnection method

4 participants

@cryptodev-2s@mcmire@Gudahtt@mikesposito