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

Require Infura project ID #1276

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Require Infura project ID #1276

merged 4 commits into from
Apr 27, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 26, 2023

Description

The network controller now requires an Infura project ID to be provided upon construction. This brings the network controller more in-line with the extension. Runtime validation has been added for the sake of making the merge with the extension network controller easier.

Changes

@metamask/network-controller

  • BREAKING: The infuraProjectId constructor parameter is now required

References

Closes #1201

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@Gudahtt Gudahtt marked this pull request as ready for review April 26, 2023 18:06
@Gudahtt Gudahtt requested a review from a team as a code owner April 26, 2023 18:06
The network controller now requires an Infura project ID to be provided
upon construction. This brings the network controller more in-line with
the extension. Runtime validation has been added for the sake of making
the merge with the extension network controller easier.

Closes #1201
@legobeat
Copy link
Contributor

legobeat commented Apr 26, 2023

Is it possible to refrain from doing this and take it in the other direction? The goal should be that everything degrades gracefully without Infura connectivity or credentials as a first step, and long-term to make the codebase provider-agnostic and remove hardcoded references to specific providers on this level?.

@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 26, 2023

This constructor argument won't make a difference either way in terms of how baked-in the Infura networks are. This isn't meant to cement them further, just to improve validation and help us merge the two network controllers.

I'd like to remove the special handling of Infura providers as well but... that's not trivial (API changes, configuration, middleware differences, etc.). It'll be easier to tackle that when we have one network controller rather than two.

@@ -173,7 +173,7 @@ export type NetworkControllerMessenger = RestrictedControllerMessenger<
export type NetworkControllerOptions = {
messenger: NetworkControllerMessenger;
trackMetaMetricsEvent: () => void;
infuraProjectId?: string;
infuraProjectId: string;
Copy link
Contributor

@legobeat legobeat Apr 26, 2023

Choose a reason for hiding this comment

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

It's easy to get the impression that this does cement the provider.. Here or another place good to add a // TODO: or // FIXME: on that the intention is that the controller will be made provider-agnostic?

Copy link
Member Author

@Gudahtt Gudahtt Apr 26, 2023

Choose a reason for hiding this comment

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

Sorry I should elaborate; the path to removing these would be to enhance the capabilities of our custom RPC configuration, such that it can be used to support our Infura networks as well.

We've already started down this path; the localhost e2e test network used to be baked-in, but now we use it as a custom network instead. And when we recently added Linea as a default network, it was added as a custom network.

That is, I'd expect us to delete the built-in networks completely rather than make them optional. Making them optional isn't need not be a step along the path to removing them.

It's long past time to track this work in an issue; I'll set a reminder to create one tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue created: #1279

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

@Gudahtt Gudahtt merged commit 95c0d2d into main Apr 27, 2023
@Gudahtt Gudahtt deleted the require-infura-project-id branch April 27, 2023 17:50
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The network controller now requires an Infura project ID to be provided
upon construction. This brings the network controller more in-line with
the extension. Runtime validation has been added for the sake of making
the merge with the extension network controller easier.

Closes #1201
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The network controller now requires an Infura project ID to be provided
upon construction. This brings the network controller more in-line with
the extension. Runtime validation has been added for the sake of making
the merge with the extension network controller easier.

Closes #1201
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.

NetworkController constructor should throw if infuraProjectId is invalid
3 participants