Tokens controller approve reject refactor full #1268

bergarces · 2023-04-26T09:50:02Z

Description

Stops triggering the acceptance or rejection of the approval from within TokensController. That'll be the responsibility of the caller.

This is part of a series of tickets that aim to refactor and simplify how we await for the confirmation promise.

Changes

BREAKING: Awaits approval request in TokensController instead of relaying on acceptWatchAsset and rejectWatchAsset, which are being removed.
BREAKING: Changes return value for watchAsset.

References

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

bergarces · 2023-04-26T14:56:47Z

packages/assets-controllers/src/TokensController.ts

@@ -667,144 +614,48 @@ export class TokensController extends BaseController<
    asset: Token,
    type: string,
    interactingAddress?: string,
-  ): Promise<AssetSuggestionResult> {
+  ): Promise<void> {


If this wasn't already a breaking change I'd have avoided doing this, but the changes are already breaking.

When this wasn't awaiting for the request to be approved or declined it was returning both the pending asset and the promise to await, but now that watchAsset handles the whole process there's no need to return the pending asset (which was not being used anyway) nor the address of the token (which was not being used either).

This makes more sense when looking at the before/after of how it's handled in the clients

PR for extension https://github.com/MetaMask/metamask-extension/pull/18829/files#diff-f3e25ebd138f61f51874e39dcbfcc24b8aff46248bf5e7deebfc9561a88d3824R39

Mobile is similar, no result is expected

For reference, the counter argument is that it could be good practice to provide useful return data in all our API methods for the sake of future-proofing. Though given the level of complexity we've reached, I'd agree to simplify where we can as we can add this back when needed.

Naturally this wouldn't be possible if the return value ever reached the dApp, but currently the only result from the middleware is a boolean.

The thing is that I'm not entirely sure what would be useful to return in this case. The suggestedAssetMeta or the Token from addToken.

If we do the second, and then we end up adding more types, it's going to turn into a bigger breaking change.

I think leaving it to return void is safest, as we probably won't need to update anything in every client if we eventually return something (even though it still counts as breaking change).

bergarces · 2023-04-26T14:57:33Z

packages/assets-controllers/src/TokensController.ts

    const { selectedAddress } = this.config;

-    const suggestedAssetMeta: SuggestedAssetMeta & {
-      interactingAddress: string;


We don't need to specify that interactingAddress is not optional, because selectedAddress isn't.

…controller-approve-reject-refactor-full

bergarces · 2023-05-04T19:30:23Z

packages/assets-controllers/src/TokensController.ts

@@ -120,7 +85,6 @@ export interface TokensState extends BaseState {
  allTokens: { [key: string]: { [key: string]: Token[] } };
  allIgnoredTokens: { [key: string]: { [key: string]: string[] } };
  allDetectedTokens: { [key: string]: { [key: string]: Token[] } };
-  suggestedAssets: SuggestedAssetMeta[];


No longer need for status if we don't want to persist them after restarts.

As this was previously persisted, does this warrant a migration to delete this data on upgrade?
Not sure what the best practice is and if we prioritise fewer migrations over dormant data in the user state.

How was this decision come to? Not saying I disagree with it. I'm just curious what the thinking was here.

The ability to persist pending wallet_watchAsset after a restart had no tests to cover that case and it was inadvertently broken in a previous PR.

When we found out about the regression, we reached out, asked if that feature was intentional, accidental or important, so that we could decide between fixing it first. The decision was to not restore that feature and have it behave the same way as other confirmations such as the ones that ask specific permissions (which are also not persisted).

cc @cryptotavares

@adonesky1 I have discussed this with @danjm and @bschorchit and we didn't thought of any relevant use case. As so we have decided to standardise the pending wallet_watchAsset confirmation behaviour (i.e no persisting confirmations after a restart... just like permissions for example. The only exception are transactions). But is there a use case that you can think for persisting pending approvals to add suggested assets?

I guess the one reason I can think of to keep suggestedAsset state (though we can probably accomplish this by looking at the approval controller state) is that to avoid duplicate suggestions. This is obviously not about persisting state across reloads. We do not currently prevent duplicate suggestions:

But I was thinking maybe we ought to?

It'd be easier to rate limit approvals from wallet_watchAsset type from ApprovalController.

bergarces · 2023-05-04T19:43:32Z

packages/assets-controllers/src/TokensController.ts

-      }
-    } catch (error) {
-      this.failSuggestedAsset(suggestedAssetMeta, error);
-      return Promise.reject(error);


This error return is maintained by:

Throwing with what is now the default clause if the type is not ERC20 (this should probably be changed to throw using ethErrors from eth-rpc-errors).

Letting validateTokenToWatch throw freely.

bergarces · 2023-05-04T19:46:53Z

packages/assets-controllers/src/TokensController.ts

-              return reject(new Error(meta.error.message));
-            /* istanbul ignore next */
-            default:
-              return reject(new Error(`Unknown status: ${meta.status}`));


All the cases in this switch are maintained (except the return value when it resolves):

case 1: When it resolves, the return value is now void for the reasons explained in the watchAsset signature.

case 2: When it's rejected, instead of throwing an error with a string we will define a userRejectedRequest in the same way it is done for permissions from the FE (https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/permissions-connect/permissions-connect.component.js#L383)

case 3: If it fails for any other reason, we are no longer catching the error so it will throw freely.

default: This cannot happen.

bergarces · 2023-05-04T19:54:24Z

packages/assets-controllers/src/TokensController.ts

+  ): Promise<void> {
+    if (type !== 'ERC20') {
+      throw new Error(`Asset of type ${type} not supported`);
+    }


I have added this validation because I don't think there's any point on keeping the switch given that this is only for ERC20. By keeping the switch we would have to either:

Add a switch before validation and another one after awaiting for request approval (like it currently does in the acceptWatchAsset), which in turn generates a default case that is unreachable.

Another option would be to define a variable that holds a callback and is populated in the first switch, something like:

let addToken: () => void; switch (type) { case 'ERC20': validateTokenToWatch(asset); addToken = () => { this.addToken(/* stuff */) }; break; default: throw new Error(`Asset of type ${type} not supported`); }

That way, if there were multiple cases, we could avoid needing a second switch after waiting for approval. This would be my choice if there was more than one type or plans to add another type here, but that's not the case and doing it will be more difficult to read for no reason (NFTs are being handled elsewhere).

Suggestions welcome though.

This sounds good to me, an if is cleaner if we don't expect to ever have any more cases.

bergarces · 2023-05-04T19:55:50Z

packages/assets-controllers/src/TokensController.ts

This file has undergone chunky refactors. I suggest to review it by looking both at the diff and the final product.

bergarces · 2023-05-04T19:56:53Z

packages/assets-controllers/src/TokensController.ts

-   *
-   * @param suggestedAssetID - The ID of the suggestedAsset to accept.
-   */
-  rejectWatchAsset(suggestedAssetID: string) {


Nothing needs to happen now when rejecting since there's no longer status for suggestedAssets, so this is dead code.

bergarces · 2023-05-11T16:05:02Z

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

The reason so many tests have been removed, is because some of them were testing the watchAsset part and others were testing approval/rejection.

That all happens within watchAsset now, so they can be simplified maintaining coverage.

packages/assets-controllers/src/TokensController.ts

matthewwalsh0 · 2023-05-15T21:43:24Z

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

-        .stub(tokensController, '_generateRandomId')
-        .callsFake(() => requestId);
-      type = 'ERC20';
+    it('stores token correctly under interacting address if user confirms', async function () {


I appreciate it's a negative test as it's verifying we don't have any error handling logic, but should we have a test to verify that the method throws if the promise from the messenger call rejects?

I can't see what value that'll add as part of a unit test. It's a promise that is being awaited and that is not wrapped in a try/catch, so yes, it will throw.

In an e2e test we can check that, when rejected, it indeed throws a userRejectedRequest RPC error.

The value would be it would assert our intention that it's not catching the approval errors.

What you've said concerning the code is correct but the unit test would programmatically enforce that requirement going forward, and ensure it's not captured and handled in future without us being aware and explicitly changing our expectations.

adonesky1 · 2023-05-15T21:44:21Z

packages/assets-controllers/src/TokensController.ts


-    this._requestApproval(suggestedAssetMeta);
+    await this._requestApproval(suggestedAssetMeta);


Should we wrap this in a try/catch and handle a failure state gracefully since the catch within the method is removed?

I don't think we should after these changes.

If, for example, the user rejects to add the token from the front end, we now rely on them sending the userRejectedRequest error, which we want to throw as it is.

adonesky1 · 2023-05-18T20:19:15Z

Just want to highlight this PR I've opened with NFT side changes that mirror this refactor: #1173

* refactor to wait for pending approval promise to resolve/reject

bergarces added 3 commits April 24, 2023 14:40

stops calling approve and reject from controller

c8d6b95

tests

1eb810b

await ApprovalController promise

b9ba7a7

bergarces force-pushed the tokens-controller-approve-reject-refactor-full branch from 8662958 to b9ba7a7 Compare April 26, 2023 12:06

bergarces added 2 commits April 26, 2023 14:53

further refactor

e9386ac

fix

5b6ba7a

bergarces force-pushed the tokens-controller-approve-reject-refactor-full branch from f808086 to 5b6ba7a Compare April 26, 2023 14:31

further refactor

ab5f738

bergarces commented Apr 26, 2023

View reviewed changes

coverage increased

3595a45

adonesky1 mentioned this pull request Apr 27, 2023

Add ERC721 & ERC1155 wallet_watchAsset support #1173

Merged

bergarces added 4 commits May 4, 2023 20:13

stops calling approve and reject from controller

78a9995

tests

ea6c428

remove unneeded actions

308856f

Merge branch 'tokens-controller-approve-reject-refactor' into tokens-…

3145b04

…controller-approve-reject-refactor-full

bergarces commented May 4, 2023

View reviewed changes

remove suggestedAssets from status

38e2ebf

bergarces force-pushed the tokens-controller-approve-reject-refactor-full branch from 3b37ef1 to 38e2ebf Compare May 4, 2023 19:37

bergarces commented May 4, 2023

View reviewed changes

revert coverage changes

a3ad19e

bergarces marked this pull request as ready for review May 4, 2023 20:38

bergarces requested a review from a team as a code owner May 4, 2023 20:38

bergarces marked this pull request as draft May 5, 2023 08:33

Merge branch 'main' into tokens-controller-approve-reject-refactor-full

4066934

bergarces commented May 11, 2023

View reviewed changes

bergarces marked this pull request as ready for review May 11, 2023 16:10

Merge branch 'main' into tokens-controller-approve-reject-refactor-full

727e421

matthewwalsh0 reviewed May 15, 2023

View reviewed changes

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

matthewwalsh0 reviewed May 15, 2023

View reviewed changes

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

matthewwalsh0 reviewed May 15, 2023

View reviewed changes

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

matthewwalsh0 reviewed May 15, 2023

View reviewed changes

adonesky1 reviewed May 15, 2023

View reviewed changes

use constant

3a60b96

matthewwalsh0 previously approved these changes May 19, 2023

View reviewed changes

unit test for pending approval failure

b7f317e

bergarces dismissed matthewwalsh0’s stale review via b7f317e May 19, 2023 10:50

Merge branch 'main' into tokens-controller-approve-reject-refactor-full

948a632

matthewwalsh0 approved these changes May 19, 2023

View reviewed changes

OGPoyraz approved these changes May 19, 2023

View reviewed changes

bergarces merged commit 09385b1 into main May 19, 2023

bergarces deleted the tokens-controller-approve-reject-refactor-full branch May 19, 2023 12:14

mcmire mentioned this pull request May 25, 2023

Release 53.0.0 #1385

Merged

Gudahtt added a commit that referenced this pull request May 26, 2023

Add more detail about #1268 changes

1ca5bc1

Gudahtt added a commit that referenced this pull request May 26, 2023

Add more detail about #1268 changes

93ded98

Gudahtt added a commit that referenced this pull request May 26, 2023

Add more detail about #1268 changes

84a3d31

Gudahtt added a commit that referenced this pull request May 26, 2023

Add more detail about #1268 changes

95a57aa

Gudahtt added a commit that referenced this pull request May 26, 2023

Add more detail about #1268 changes

045ee29

Gudahtt added a commit that referenced this pull request May 26, 2023

Add more detail about #1268 changes

cb17520

brad-decker pushed a commit to dragana8/core that referenced this pull request Jun 1, 2023

Tokens controller approve reject refactor full (MetaMask#1268)

a27af72

* refactor to wait for pending approval promise to resolve/reject

mcmire mentioned this pull request Jun 5, 2023

Handle watch asset accept and reject using ApprovalController only MetaMask/metamask-extension#18829

Merged

8 tasks

Gudahtt mentioned this pull request Jul 31, 2023

refactor: Update @metamask/assets-controllers patch MetaMask/metamask-mobile#6898

Merged

3 tasks

legobeat mentioned this pull request Aug 28, 2023

fix: error during persisted transaction approval MetaMask/metamask-extension#20626

Merged

8 tasks

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Tokens controller approve reject refactor full (#1268)

9c417a5

* refactor to wait for pending approval promise to resolve/reject

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Tokens controller approve reject refactor full (#1268)

571cfc0

* refactor to wait for pending approval promise to resolve/reject

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tokens controller approve reject refactor full #1268

Tokens controller approve reject refactor full #1268

bergarces commented Apr 26, 2023 •

edited

Loading

bergarces Apr 26, 2023 •

edited

Loading

bergarces May 11, 2023

matthewwalsh0 May 15, 2023

bergarces May 16, 2023

bergarces Apr 26, 2023

bergarces May 4, 2023

matthewwalsh0 May 15, 2023

adonesky1 May 15, 2023

bergarces May 16, 2023 •

edited

Loading

cryptotavares May 16, 2023

adonesky1 May 16, 2023

bergarces May 17, 2023

bergarces May 4, 2023

bergarces May 4, 2023

bergarces May 4, 2023 •

edited

Loading

matthewwalsh0 May 15, 2023

bergarces May 4, 2023

bergarces May 4, 2023

bergarces May 11, 2023

matthewwalsh0 May 15, 2023

bergarces May 17, 2023

matthewwalsh0 May 19, 2023 •

edited

Loading

adonesky1 May 15, 2023

bergarces May 16, 2023

adonesky1 commented May 18, 2023 •

edited

Loading


		this._requestApproval(suggestedAssetMeta);
		await this._requestApproval(suggestedAssetMeta);

Tokens controller approve reject refactor full #1268

Tokens controller approve reject refactor full #1268

Conversation

bergarces commented Apr 26, 2023 • edited Loading

Description

Changes

References

Checklist

bergarces Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergarces May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergarces May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewwalsh0 May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 commented May 18, 2023 • edited Loading

bergarces commented Apr 26, 2023 •

edited

Loading

bergarces Apr 26, 2023 •

edited

Loading

bergarces May 16, 2023 •

edited

Loading

bergarces May 4, 2023 •

edited

Loading

matthewwalsh0 May 19, 2023 •

edited

Loading

adonesky1 commented May 18, 2023 •

edited

Loading