Add eth-query types #1266

Gudahtt · 2023-04-25T21:27:50Z

Description

Type definitions have been added for the eth-query package. These were copied from the type definitions used in the extension.

The addition of these types revealed various invalid assumptions we've made throughout the codebase, requiring the addition of runtime checks. Mostly this consists of throwing an error whenever someone tries to use the ethQuery instance before it has been set. In most cases, this would only introduce an error in code paths that would already throw an error.

The one exception to that is the gas fee controller, which used to get ethQuery upon construction. It has been updated to wait until the providerConfigChange event before attempting to fetch that, since it's never defined upon construction in practice anyway.

There are two places in the network controller where these types revealed that we're not validating the network response to the net_version and eth_getBlockByNumber calls made during network lookup. A comment has been left about validating this in a future PR.

Changes

`@metamask/assets-controllers`

BREAKING: The controller configuration now uses our EthQuery type definition for the provider config property

`@metamask/controller-utils`

CHANGED: The query function has improved type checks for the ethQuery argument
- This type change could be breaking, but only if you were passing in an invalid ethQuery parameter. In that circumstance this would have thrown an error at runtime anyway. Effectively this should be non-breaking for any usage that isn't already broken.

`@metamask/gas-fee-controller`

BREAKING: NetworkController:getEthQuery is no longer called in the constructor
- The internal ethQuery instance gets updated upon the providerConfigChange event. This means that the ethQuery instance won't get set until after provider initialization or the next network switch.
- This should have no impact upon extension or mobile, but might affect projects that were constructing this controller after the network controller was initialized.

`@metamask/network-controller`

BREAKING: The exported Provider type has been updated to better reflect the provider type returned by the network controller
- Previously this was set to any. Now it returns a type that mostly matches the provider returned (some semi-internal properties are omitted)
- This affects the exported ProviderProxy type as well, which wraps the Provider type
BREAKING: The action NetworkController:getEthQuery will now throw an error if ethQuery is not set, rather than returning undefined

References

Closes #1204

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

Type definitions have been added for the `eth-query` package. These were copied from the type definitions used in the extension. The addition of these types revealed various invalid assumptions we've made throughout the codebase, requiring the addition of runtime checks. Mostly this consists of throwing an error whenever someone tries to use the `ethQuery` instance before it has been set. In most cases, this would only introduce an error in code paths that would already throw an error. The one exception to that is the gas fee controller, which used to get `ethQuery` upon construction. It has been updated to wait until the `providerConfigChange` event before attempting to fetch that, since it's never defined upon construction in practice anyway. There are two places in the network controller where these types revealed that we're not validating the network response to the `net_version` and `eth_getBlockByNumber` calls made during network lookup. A comment has been left about validating this in a future PR. Closes #1204

packages/assets-controllers/src/AccountTrackerController.ts

packages/controller-utils/src/util.test.ts

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

packages/controller-utils/package.json

packages/assets-controllers/src/AccountTrackerController.ts

packages/network-controller/src/NetworkController.ts

packages/network-controller/tests/provider-api-tests/helpers.ts

packages/assets-controllers/src/AccountTrackerController.ts

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

Gudahtt · 2023-04-27T20:29:20Z

packages/gas-fee-controller/src/GasFeeController.ts

@@ -346,9 +346,6 @@ export class GasFeeController extends BaseControllerV2<
        'NetworkController:getProviderConfig',
      );
      this.currentChainId = providerConfig.chainId;
-      this.ethQuery = this.messagingSystem.call(


In retrospect, my assumption here wasn't entirely true; in practice this likely was set upon construction. Just not in the tests. So this is breaking after all. We'll need to fix this before the next release.

Maybe the simplest fix would be to update getEthQuery to return EthQuery | undefined, and restore this line.

Okay, fixed via #1284.

Gudahtt · 2023-06-23T17:57:20Z

packages/controller-utils/src/util.ts

      if (error) {
        reject(error);
        return;
      }
      resolve(result);
    };

-    if (typeof ethQuery[method] === 'function') {
+    if (
+      hasProperty(ethQuery, method) &&


This seems to have introduced a bug; the methods in question are on the ethQuery prototype, so this is returning false when this condition used to resolve to true

Right, that makes sense.

The `query` function was updated in #1266 to use `hasProperty` has part of an effect to improve the types being used. Unfortunately this had the unexpected affected of breaking this condition because `hasProperty` doesn't look up the prototype chain, but the method we were checking for was on the prototype. `hasProperty` is only meant to be used with plain objects. The condition has been updated to use `in`, and the tests have been updated to use a more realistic EthQuery mock that has methods as prototypes.

The `query` function was updated in #1266 to use `hasProperty` has part of an effect to improve the types being used. Unfortunately this had the unexpected affected of breaking this condition because `hasProperty` doesn't look up the prototype chain, but the method we were checking for was on the prototype. `hasProperty` is only meant to be used with plain objects. The condition has been updated to use `in`, and the tests have been updated to use a more realistic EthQuery mock that has methods as prototypes. To fix various type errors, an old inlined EthQueryLike type has been replaced by the real EthQuery type. This required adding `eth-query` as a dependency to the controller utils package.

Type definitions have been added for the `eth-query` package. These were copied from the type definitions used in the extension. The addition of these types revealed various invalid assumptions we've made throughout the codebase, requiring the addition of runtime checks. Mostly this consists of throwing an error whenever someone tries to use the `ethQuery` instance before it has been set. In most cases, this would only introduce an error in code paths that would already throw an error. The one exception to that is the gas fee controller, which used to get `ethQuery` upon construction. It has been updated to wait until the `providerConfigChange` event before attempting to fetch that, since it's never defined upon construction in practice anyway. There are two places in the network controller where these types revealed that we're not validating the network response to the `net_version` and `eth_getBlockByNumber` calls made during network lookup. A comment has been left about validating this in a future PR. Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

The `query` function was updated in #1266 to use `hasProperty` has part of an effect to improve the types being used. Unfortunately this had the unexpected affected of breaking this condition because `hasProperty` doesn't look up the prototype chain, but the method we were checking for was on the prototype. `hasProperty` is only meant to be used with plain objects. The condition has been updated to use `in`, and the tests have been updated to use a more realistic EthQuery mock that has methods as prototypes. To fix various type errors, an old inlined EthQueryLike type has been replaced by the real EthQuery type. This required adding `eth-query` as a dependency to the controller utils package.

Type definitions have been added for the `eth-query` package. These were copied from the type definitions used in the extension. The addition of these types revealed various invalid assumptions we've made throughout the codebase, requiring the addition of runtime checks. Mostly this consists of throwing an error whenever someone tries to use the `ethQuery` instance before it has been set. In most cases, this would only introduce an error in code paths that would already throw an error. The one exception to that is the gas fee controller, which used to get `ethQuery` upon construction. It has been updated to wait until the `providerConfigChange` event before attempting to fetch that, since it's never defined upon construction in practice anyway. There are two places in the network controller where these types revealed that we're not validating the network response to the `net_version` and `eth_getBlockByNumber` calls made during network lookup. A comment has been left about validating this in a future PR. Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

The `query` function was updated in #1266 to use `hasProperty` has part of an effect to improve the types being used. Unfortunately this had the unexpected affected of breaking this condition because `hasProperty` doesn't look up the prototype chain, but the method we were checking for was on the prototype. `hasProperty` is only meant to be used with plain objects. The condition has been updated to use `in`, and the tests have been updated to use a more realistic EthQuery mock that has methods as prototypes. To fix various type errors, an old inlined EthQueryLike type has been replaced by the real EthQuery type. This required adding `eth-query` as a dependency to the controller utils package.

Gudahtt force-pushed the add-eth-query-types branch 2 times, most recently from c8fa5bc to 4881a29 Compare April 25, 2023 21:31

Gudahtt changed the title ~~Add eth-query types~~ Add eth-query types [WIP] Apr 25, 2023

Gudahtt force-pushed the add-eth-query-types branch 13 times, most recently from 621e59f to e9c594c Compare April 26, 2023 15:51

Gudahtt changed the base branch from main to remove-has-property April 26, 2023 15:52

Gudahtt force-pushed the add-eth-query-types branch from e9c594c to 617611d Compare April 26, 2023 15:53

Base automatically changed from remove-has-property to main April 26, 2023 15:58

Gudahtt force-pushed the add-eth-query-types branch 4 times, most recently from 8d8a93b to 3fc91b6 Compare April 26, 2023 16:15

Gudahtt changed the title ~~Add eth-query types [WIP]~~ Add eth-query types Apr 26, 2023

Gudahtt force-pushed the add-eth-query-types branch from 3fc91b6 to 96b6e78 Compare April 26, 2023 17:55

Gudahtt marked this pull request as ready for review April 26, 2023 17:55

Gudahtt requested a review from a team as a code owner April 26, 2023 17:55

Mrtenz reviewed Apr 26, 2023

View reviewed changes

packages/assets-controllers/src/AccountTrackerController.ts Outdated Show resolved Hide resolved

packages/controller-utils/src/util.test.ts Outdated Show resolved Hide resolved

Use assertion for verifying that provider is set

de6204c

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

Gudahtt added 2 commits April 26, 2023 16:39

Replace ts-ignore comments with ts-expect-error

d681068

Add assert import

d1cac88

legobeat reviewed Apr 26, 2023

View reviewed changes

packages/controller-utils/package.json Outdated Show resolved Hide resolved

Mrtenz requested changes Apr 27, 2023

View reviewed changes

Gudahtt marked this pull request as draft April 27, 2023 13:02

Remove eth-query dependency from controller-utils

601a61a

Gudahtt force-pushed the add-eth-query-types branch from cc2e4e9 to 601a61a Compare April 27, 2023 13:55

Gudahtt and others added 5 commits April 27, 2023 11:28

Use our assert function rather than Node.js builtin

c11c27e

Replace unnecessary if blocks

9dfb0eb

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

Fix lint error

26eb310

Remove unnecessary if block

8ee4d7c

Add assert import

1c61d47

Gudahtt marked this pull request as ready for review April 27, 2023 14:12

Gudahtt requested a review from Mrtenz April 27, 2023 14:12

Mrtenz approved these changes Apr 27, 2023

View reviewed changes

Gudahtt merged commit f52d891 into main Apr 27, 2023

Gudahtt deleted the add-eth-query-types branch April 27, 2023 14:24

Gudahtt commented Apr 27, 2023

View reviewed changes

This was referenced Apr 27, 2023

GasFeeController ethQuery property is undefined initially when it should not be #1283

Closed

Fix GasFeeController to assign ethQuery initially #1284

Merged

Gudahtt commented Jun 23, 2023

View reviewed changes

Gudahtt mentioned this pull request Jun 23, 2023

fix(controller-utils): Fix query function #1447

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eth-query types #1266

Add eth-query types #1266

Gudahtt commented Apr 25, 2023 •

edited

Loading

Gudahtt Apr 27, 2023

Gudahtt Apr 27, 2023

mcmire Apr 27, 2023

Gudahtt Jun 23, 2023

mcmire Jun 23, 2023

Add eth-query types #1266

Add eth-query types #1266

Conversation

Gudahtt commented Apr 25, 2023 • edited Loading

Description

Changes

@metamask/assets-controllers

@metamask/controller-utils

@metamask/gas-fee-controller

@metamask/network-controller

References

Checklist

Gudahtt Apr 27, 2023

Choose a reason for hiding this comment

Gudahtt Apr 27, 2023

Choose a reason for hiding this comment

mcmire Apr 27, 2023

Choose a reason for hiding this comment

Gudahtt Jun 23, 2023

Choose a reason for hiding this comment

mcmire Jun 23, 2023

Choose a reason for hiding this comment

Gudahtt commented Apr 25, 2023 •

edited

Loading

`@metamask/assets-controllers`

`@metamask/controller-utils`

`@metamask/gas-fee-controller`

`@metamask/network-controller`