Replace network/isCustomNetwork with networkStatus#1073
Closed
cryptodev-2s wants to merge 2 commits intomainfrom
Closed
Replace network/isCustomNetwork with networkStatus#1073cryptodev-2s wants to merge 2 commits intomainfrom
cryptodev-2s wants to merge 2 commits intomainfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
mcmire
reviewed
Jan 23, 2023
packages/transaction-controller/src/TransactionController.test.ts
Outdated
Show resolved
Hide resolved
Base automatically changed from
improve-network-controller-unit-tests
to
main
January 24, 2023 16:25
The network controller contains two related state properties: `network` and `isCustomNetwork`. The first property is strange: it can either be a number, which represents the version of the network as obtained via `net_version`, or "loading", which is a sort of null state that either means we don't have a network to connect to, or we do but it's not accessible. In any case, the `net_version` RPC method, much less the idea of a network version, is no longer standard, so we don't need to store it. The second property is unnecessary, as we can already obtain whether a network is "custom" (i.e. not one of the ones we know about by default) by looking at the `type` within the provider config object. Therefore, we can remove these state properties and replace them with a simple property that merely tracks whether we have an accessible network or not. Co-authored-by: Salah <salah-eddine.saakoun@consensys.net>
mcmire
force-pushed
the
replace-network-and-iscustomnetwork
branch
from
2db91e4 to
f98e942
Compare
mcmire
changed the title
Gudahtt
reviewed
Feb 27, 2023
Comment on lines
+53
to
+57
| * created for the currently selected network and the network is provably | ||
| * accessible. | ||
| * @property loading - Represents the state in which a provider object does not | ||
| * exist for the currently selected network, or it does exist but the network is | ||
| * provably inaccessible. |
Member
There was a problem hiding this comment.
Nit: Maybe better to use softer language here since we only check periodically, and since the way we check today is a bit questionable and might not be a guarantee that the network is accessible (e.g. if a network doesn't implement net_version, or uses a cache for static methods like this, it might not reflect whether the network is accessible or not).
Suggested change
| * created for the currently selected network and the network is provably | |
| * accessible. | |
| * @property loading - Represents the state in which a provider object does not | |
| * exist for the currently selected network, or it does exist but the network is | |
| * provably inaccessible. | |
| * created for the currently selected network and the network is seems | |
| * accessible. | |
| * @property loading - Represents the state in which a provider object does not | |
| * exist for the currently selected network, or it does exist but the network is | |
| * seems inaccessible. |
Gudahtt
reviewed
Feb 27, 2023
Member
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
Looks great! There seems to be a conflict to resolve though.
Also, could we compile a list of all of the breaking changes? Lots on the transaction controller here too.
Contributor
|
I need to revisit this. I'll try to focus on this after my current task on the extension side. |
Contributor
|
Placing this back in draft until MetaMask/metamask-extension#17556 is merged. |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The network controller contains two related state properties:
networkand
isCustomNetwork.The first property is strange: it can either be a number, which
represents the version of the network as obtained via
net_version, or"loading", which is a sort of null state that either means we don't have
a network to connect to, or we do but it's not accessible. In any case,
the
net_versionRPC method, much less the idea of a network version,is no longer standard, so we don't need to store it.
The second property is unnecessary, as we can already obtain whether a
network is "custom" (i.e. not one of the ones we know about by default)
by looking at the
typewithin the provider config object.Therefore, we can remove these state properties and replace them with a
simple property that merely tracks whether we have an accessible network
or not.
Fixes #1020.