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.

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

Disallow loading as metamaskNetworkId #5924

Merged
merged 2 commits into from
Dec 13, 2018
Merged

Conversation

frankiebee
Copy link
Contributor

No description provided.

@frankiebee
Copy link
Contributor Author

in a state where we aren't able to get the net id we should just fail those transactions because the txController needs some guarantee about the network

@metamaskbot
Copy link
Collaborator

Builds ready [7e4886b]: mascara, chrome, firefox, edge, opera

@danjm
Copy link
Contributor

danjm commented Dec 13, 2018

I think this works as an immediate solution to the bug.

However, it seems that there is a broader architectural issue with the network controller. The network state can either be an id of a network, or "loading". Then, we handle getNetworkState () returns of "loading" in various ad hoc ways throughout the controller and even in the UI.

We may be able to make the code clearer and less fragile if we store two different values in the network controller state: selectedNetwork for storing an id of the network that the user has most recently selected, and a boolean isNetworkLoading to indicate whether or not we are connected to the selectedNetwork

Something like this may also be required if we want to provide better offline support.

Of course, this is beyond the scope of this PR and should be treated as a separate issue.

@frankiebee frankiebee merged commit b5d6452 into develop Dec 13, 2018
@frankiebee frankiebee deleted the disallow-loading-as-netId branch December 13, 2018 19:14
Sign up for free to 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