any type in asset-controller classes have been fixed with a proper types #3818

kanthesha · 2024-01-23T12:13:40Z

Explanation

Replace use of any with proper types (non-test files only) in asset-controller.

Changelog

`@metamask/assets-controllers`

BREAKING: Change type of provider property in AssetsContractController from any to Provider from @metamask/network-controller.
BREAKING: Change type of provider property in TokensController from any to Provider from @metamask/network-controller.

References

Fixes assets-controllers: Replace use of any with proper types (non-test files only) #3714

kanthesha · 2024-01-23T12:38:06Z

packages/assets-controllers/src/AssetsContractController.ts

@@ -177,7 +175,9 @@ export class AssetsContractController extends BaseControllerV1<
   */
  getProvider(networkClientId?: NetworkClientId): Web3Provider {
    const provider = networkClientId
-      ? this.getNetworkClientById(networkClientId).provider
+      ? (this.getNetworkClientById(networkClientId).provider as unknown as


It's little tricky here! Web3Provider constructor expects ExternalProvider | JsonRpcFetchFunc! but this.getNetworkClientById(networkClientId).provider type is ProxyWithAccessibleTarget<SafeEventEmitterProvider>!! For now, I can only think of this type assertion!!

I'm trying to find if there's a better way!

kanthesha · 2024-01-23T12:40:15Z

packages/assets-controllers/src/NftController.ts

          if (k === 'address') {
            return true; // address will always be present
          }
          // collection will always be an object, we need to check the internal values
          if (k === 'collection') {
-            return v?.name === null && v?.image_url === null;
+            const obj = v as {


The collection is {name: string; image_url: string} . Link to the declaration.

kanthesha · 2024-01-23T12:42:19Z

packages/assets-controllers/src/TokensController.ts

-        ? this.getNetworkClientById(networkClientId).provider
-        : this.config?.provider,
-    );
+    const provider = networkClientId


Web3Provider constructor expects ExternalProvider | JsonRpcFetchFunc! but this.getNetworkClientById(networkClientId).provider type is ProxyWithAccessibleTarget<SafeEventEmitterProvider>!! For now, I can only think of this type assertion!!

I'm trying to find if there's a better way!

MajorLift · 2024-01-24T05:51:14Z

Provider type incompatibility in prettified format:

Maybe adding id, jsonrpc to the network client provider's sendAsync request type might work?

kanthesha · 2024-01-25T14:55:20Z

@MajorLift
I think, it is other way around! network client provider's sendAsync has a req: JsonRpcRequest, JsonRpcRequest additionally contains id and jsonrpc (link) compared to sendAsync of JsonRpcFetchFunc & ExternalProvider's request: { method: string, params?: Array<any> }.

MajorLift · 2024-01-25T15:01:04Z

Oh you're right. In that case, I guess we'd want to remove id and jsonrpc from the provider before it's passed into Web3Provider.

kanthesha · 2024-01-25T15:21:18Z

hmm! id and jsonrpc are there in the request parameter of send and sendAync functions of provider object.

MajorLift · 2024-01-25T18:37:30Z

Here's the updated provider with working methods. Since ExternalProvider and Web3Provider don't use id and jsonrpc, the mock value for id shouldn't be an issue.

    const provider = {
      ...networkClientProvider,
      send: (
        _req: { method: string; params?: Json[] },
        _callback: (error: unknown, response: unknown) => void,
      ) =>
        networkClientProvider.send.bind(provider)(
          { ..._req, id: '', jsonrpc: '2.0' },
          _callback,
        ),
      sendAsync: (
        _req: { method: string; params?: Json[] },
        _callback: (error: unknown, response: unknown) => void,
      ) =>
        networkClientProvider.sendAsync.bind(provider)(
          { ..._req, id: '', jsonrpc: '2.0' },
          _callback,
        ),
    };

packages/assets-controllers/src/AssetsContractController.ts

packages/assets-controllers/src/NftController.ts

packages/assets-controllers/src/AssetsContractController.ts

mcmire

Almost there! Just two more things.

packages/assets-controllers/src/TokensController.ts

MajorLift · 2024-02-07T15:27:17Z

packages/assets-controllers/src/TokensController.ts

-    );
+    const provider = networkClientId
+      ? this.getNetworkClientById(networkClientId).provider
+      : this.config?.provider;


nit:
internalConfig is always defined in BaseControllerV1

Suggested change

: this.config?.provider;

: this.config.provider;

MajorLift · 2024-02-07T15:32:02Z

Looks like the coverage thresholds can be increased.

-      branches: 88.36,
-      functions: 97.08,
-      lines: 97.23,
-      statements: 97.28,
+      branches: 89.35,
+      functions: 97.74,
+      lines: 97.64,
+      statements: 97.68,

…ore into asset-controller-fix-any-type

kanthesha · 2024-02-07T17:10:33Z

@MajorLift
I see that the branch % is 89.29 in my local. I've updated the jest.config accordingly.

   branches: 89.29,
   functions: 97.74,
   lines: 97.64,
   statements: 97.68,

mcmire

One final thing :)

packages/assets-controllers/src/TokensController.test.ts

mcmire

Left a non-blocking comment. Looks good.

mcmire · 2024-02-08T19:31:09Z

packages/assets-controllers/src/TokensController.test.ts

@@ -109,6 +110,8 @@ describe('TokensController', () => {
    tokenListStateChangeListener = listener;
  });

+  const fakeProvider = new FakeProvider();


Heads up — it's best to avoid setting variables outside of beforeEach/afterEach/it, because it means that changes to an object could persist across multiple tests, which could lead to tests that intermittently fail or that are difficult to debug. In this case it's okay since the fake provider isn't actually used for anything, it's just here to satisfy a type, but something to keep in mind for future tests. This is something we go into more detail about in the contributor docs, but we could certainly make this more clear if it's not clear enough.

Yeah, here it is intentional. The objects and variables which will be modified in the tests and needs reset after each test has to be included in beforeEach/afterEach/it. For example, if we want to write a test case where mainnet provider is expected and in an another test case, we are expecting testnet. But in this case we are not modifying the provider. This kind of initialisation can optionally moved to beforeAll. Adding unnecessary setup work into beforeEach or afterEach may slowdown the tests.

mcmire

Looks good!

There's one thing remaining in this PR: updating the Changelog section in the PR description. We use this section to populate CHANGELOG.md when we create release PRs. You can see an example of that in this PR: #3749. In this case I believe that some of the changes to AssetsContractController and TokensController are breaking.

kanthesha · 2024-02-09T18:29:00Z

The changelog section has been added.

kanthesha added 2 commits January 23, 2024 12:06

any type in asset-controller classes have been fixed with a proper types

ec48f57

fix lint error

bed3ed0

kanthesha commented Jan 23, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

provider type updated

9a086c3

mcmire reviewed Jan 25, 2024

View reviewed changes

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

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

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

kanthesha added 3 commits January 25, 2024 21:19

improve test coverage

1d4c503

type assertion replaced

ae7fadc

revert to type assertion

178f659

kanthesha marked this pull request as ready for review February 6, 2024 12:50

kanthesha requested a review from a team as a code owner February 6, 2024 12:50

mcmire reviewed Feb 6, 2024

View reviewed changes

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

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

kanthesha added 3 commits February 6, 2024 19:51

add ts-expect-error instead of wrapper

9da3da8

fix tests failing

7a99dcb

Merge branch 'main' into asset-controller-fix-any-type

520f0fc

MajorLift reviewed Feb 7, 2024

View reviewed changes

kanthesha added 2 commits February 7, 2024 17:08

updated coverage thresholds in jest.config

cb49602

Merge branch 'asset-controller-fix-any-type' of github.com:MetaMask/c…

800a5c1

…ore into asset-controller-fix-any-type

mcmire reviewed Feb 7, 2024

View reviewed changes

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

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

kanthesha added 2 commits February 8, 2024 10:00

replace real provider with fake provider in unit tests

ff29d8e

Merge branch 'main' into asset-controller-fix-any-type

a07a1e6

mcmire previously approved these changes Feb 8, 2024

View reviewed changes

Resolved merge conflict by incorporating both suggestions

d88da8a

kanthesha dismissed mcmire’s stale review via d88da8a February 9, 2024 08:54

kanthesha added 2 commits February 9, 2024 09:04

fix tests failing

9b9dee7

Merge branch 'main' into asset-controller-fix-any-type

dfba038

mcmire approved these changes Feb 9, 2024

View reviewed changes

kanthesha merged commit 8282d9c into main Feb 9, 2024
136 checks passed

kanthesha deleted the asset-controller-fix-any-type branch February 9, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

any type in asset-controller classes have been fixed with a proper types #3818

any type in asset-controller classes have been fixed with a proper types #3818

kanthesha commented Jan 23, 2024 •

edited

Loading

kanthesha Jan 23, 2024

kanthesha Jan 23, 2024

kanthesha Jan 23, 2024

MajorLift commented Jan 24, 2024

kanthesha commented Jan 25, 2024

MajorLift commented Jan 25, 2024

kanthesha commented Jan 25, 2024

This comment was marked as outdated.

MajorLift commented Jan 25, 2024 •

edited

Loading

mcmire left a comment

MajorLift Feb 7, 2024

MajorLift commented Feb 7, 2024

kanthesha commented Feb 7, 2024

mcmire left a comment

mcmire left a comment

mcmire Feb 8, 2024

kanthesha Feb 9, 2024 •

edited

Loading

mcmire left a comment

kanthesha commented Feb 9, 2024

any type in asset-controller classes have been fixed with a proper types #3818

any type in asset-controller classes have been fixed with a proper types #3818

Conversation

kanthesha commented Jan 23, 2024 • edited Loading

Explanation

Changelog

@metamask/assets-controllers

References

kanthesha Jan 23, 2024

Choose a reason for hiding this comment

kanthesha Jan 23, 2024

Choose a reason for hiding this comment

kanthesha Jan 23, 2024

Choose a reason for hiding this comment

MajorLift commented Jan 24, 2024

kanthesha commented Jan 25, 2024

MajorLift commented Jan 25, 2024

kanthesha commented Jan 25, 2024

This comment was marked as outdated.

MajorLift commented Jan 25, 2024 • edited Loading

mcmire left a comment

Choose a reason for hiding this comment

MajorLift Feb 7, 2024

Choose a reason for hiding this comment

MajorLift commented Feb 7, 2024

kanthesha commented Feb 7, 2024

mcmire left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

mcmire Feb 8, 2024

Choose a reason for hiding this comment

kanthesha Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

kanthesha commented Feb 9, 2024

kanthesha commented Jan 23, 2024 •

edited

Loading

`@metamask/assets-controllers`

MajorLift commented Jan 25, 2024 •

edited

Loading

kanthesha Feb 9, 2024 •

edited

Loading