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

replace web3-provider-engine #996

Closed
wants to merge 3 commits into from
Closed

replace web3-provider-engine #996

wants to merge 3 commits into from

Conversation

BelfordZ
Copy link
Contributor

@BelfordZ BelfordZ commented Dec 1, 2022

Replace web3-provider-engine

Use the network clients from extension.

Description

  • networkControllers.provider is now typed better than any

  • BREAKING:

  • FIXED:

    • Describe the fix/bug addressed
  • CHANGED:

    • Describe the change you have made to existing functionality
  • REMOVED:

    • Describe functionality removed and why
  • ADDED:

    • Provider change event - fired immediately after networkController.provider is sete
  • DEPRECATED:

    • Describe what was deprecated and why
  • SECURITY:

    • These should not be in a standard PR and addressed using the Security Advisory process

Checklist

  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@BelfordZ BelfordZ requested a review from a team as a code owner December 1, 2022 17:05
@BelfordZ BelfordZ marked this pull request as draft December 1, 2022 17:05
@BelfordZ BelfordZ changed the title Added network clients replace web3-provider-engine Dec 13, 2022
@BelfordZ BelfordZ marked this pull request as ready for review December 15, 2022 18:32
setTimeout(() => {
provider?.stop();
provider?.removeAllListeners();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

provider.stop doesn't exist anymore - open to suggestions over what we should do here.

@@ -226,6 +226,14 @@ export class TokenDetectionController extends BaseController<
!isDetectionEnabledForNetwork ||
!isDetectionEnabledFromPreferences
) {
console.log('early returned');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these logs needed?

Comment on lines +258 to 261
if (this.config.provider === undefined) {
return undefined;
}
return new Web3Provider(this.config?.provider);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that it shouldn't be any other falsey value than undefined here but can we still control for it, either by handling it as undefined (this makes sense to me) or throw an error?
Also config not being an object is handled below so should be here too - and better so since new Web3Provider doesn't play with undefined provider anyway.

Suggested change
if (this.config.provider === undefined) {
return undefined;
}
return new Web3Provider(this.config?.provider);
if (!this.config?.provider) {
return undefined;
}
return new Web3Provider(this.config.provider);

const networkMiddleware = mergeMiddleware([
...scaffolded,
createBlockRefRewriteMiddleware({
blockTracker: blockTracker as any,
Copy link
Contributor

@legobeat legobeat Dec 15, 2022

Choose a reason for hiding this comment

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

Why is as any used here? Types look like they should line up after https://github.com/MetaMask/controllers/pull/996/files#r1047869969?

@mcmire
Copy link
Contributor

mcmire commented Dec 16, 2022

@BelfordZ We should chat tomorrow about this. I am worried that there are slight differences in web3-provider-engine here and the json-rpc-engine middleware in the extension which we haven't properly assessed. My thought at the beginning was that we could do an exploration by writing the same kinds of tests that we have for the extension (the network client tests) here, then if there is behavior we want to keep, we could adjust web3-provider-engine to match. That would allow us to replace web3-provider-engine with json-rpc-engine with zero changes in user- or developer-facing functionality. There may, of course, be a faster way of making this assessment without having to write tests, and perhaps we don't really need to make adjustments to web3-provider-engine. But I worry that if we just replace one middleware stack with another, we may introduce breakages that we can't account for. But, perhaps we can convene on this tomorrow and figure out a good plan.

EDIT: Even so, I will for sure do a proper review of this.

@mcmire
Copy link
Contributor

mcmire commented Mar 20, 2023

Can this be closed in favor of #1116, or what's the relation of this PR to that one?

@mcmire
Copy link
Contributor

mcmire commented Mar 23, 2023

@BelfordZ See above comment ^

@BelfordZ
Copy link
Contributor Author

superceded by #1116

@BelfordZ BelfordZ closed this Mar 23, 2023
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