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

[token-detection-controller] Refactor detectTokens method #3938

Merged
merged 17 commits into from
Mar 4, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Feb 20, 2024

Explanation

Adds refactors and cosmetic, typing fixes to #3923.

References

Changelog

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift requested a review from a team as a code owner February 20, 2024 23:53
@MajorLift MajorLift changed the title Add missing method return types [token-detection-controller] Refactor detectTokens method Feb 20, 2024
@MajorLift MajorLift changed the base branch from main to 240215-TokenDetectionController-sync-with-extension February 20, 2024 23:53
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch 2 times, most recently from 0aed206 to 6fc4688 Compare February 20, 2024 23:57
@MajorLift MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from a955a1c to d4581a9 Compare February 21, 2024 00:17
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch from 6fc4688 to f2faf4f Compare February 21, 2024 00:18
@MajorLift MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from 8aaeb9c to aee4d13 Compare February 21, 2024 00:41
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch from 4fdd841 to 5fee94d Compare February 21, 2024 00:42
@MajorLift MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch from aee4d13 to cf2d53f Compare February 21, 2024 00:46
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch from 5fee94d to c91ff00 Compare February 21, 2024 00:48
@MajorLift MajorLift self-assigned this Feb 21, 2024
Comment on lines 163 to 167
#chainIdAgainstWhichToDetect: Hex;

#addressAgainstWhichToDetect: string;

#networkClientIdAgainstWhichToDetect: NetworkClientId;
Copy link
Contributor Author

@MajorLift MajorLift Feb 22, 2024

Choose a reason for hiding this comment

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

Should these be maintained separately from #chainId, #selectedAddress, #networkClientId? My assumption is that we want the detectTokens method to be callable independently of the polling/passive detection performed by the controller and messenger. At least, that seems to be what the current implementation's avoidance of class fields in detectTokens suggests.

Copy link
Contributor

@mcmire mcmire Mar 1, 2024

Choose a reason for hiding this comment

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

You're right that consumers should be able to call detectTokens independently, and it should just work.

It makes sense that we would cache the currently selected network as well as the currently selected account. I don't think we need to cache the chain ID, because we can always ask the network client. (But removing that is for another PR.)

As for these three variables, though, it seems to me that we've created them so that we can break up detectTokens into pieces. But, why not pass along these values, then? It seems that this would make it easier for someone to read the code because there would be fewer things for them to track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that a pure style that avoids using class fields might be more readable here.

Copy link
Contributor Author

@MajorLift MajorLift Mar 2, 2024

Choose a reason for hiding this comment

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

Removed the three unnecessary class fields here: ab0e3eb (#3938)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removed #chainId: 12ba664 (#3938)

Comment on lines -67 to -84
Token,
'aggregators' | 'image' | 'balanceError' | 'isERC721'
> & {
type LegacyToken = {
name: string;
logo: string;
logo: `${string}.svg`;
symbol: string;
decimals: number;
erc20?: boolean;
erc721?: boolean;
};

type TokenDetectionMap = {
[P in keyof TokenListMap]: Omit<TokenListMap[P], 'occurrences'>;
};

export const STATIC_MAINNET_TOKEN_LIST = Object.entries<LegacyToken>(
contractMap,
).reduce<
Record<
string,
Partial<TokenListToken> & Pick<Token, 'address' | 'symbol' | 'decimals'>
>
>((acc, [base, contract]) => {
const { logo, ...tokenMetadata } = contract;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better typing and handling of legacy token list.

Comment on lines 575 to 623
await safelyExecute(async () => {
const balances = await this.#getBalancesInSingleCall(
this.#addressAgainstWhichToDetect,
tokensSlice,
this.#networkClientIdAgainstWhichToDetect,
);

const tokensWithBalance: Token[] = [];
const eventTokensDetails: string[] = [];
for (const nonZeroTokenAddress of Object.keys(balances)) {
const { decimals, symbol, aggregators, iconUrl, name } =
this.#tokenList[nonZeroTokenAddress];
eventTokensDetails.push(`${symbol} - ${nonZeroTokenAddress}`);
tokensWithBalance.push({
address: nonZeroTokenAddress,
decimals,
symbol,
aggregators,
image: iconUrl,
isERC721: false,
name,
});
}

await safelyExecute(async () => {
const balances = await this.#getBalancesInSingleCall(
addressAgainstWhichToDetect,
tokensSlice,
networkClientIdAgainstWhichToDetect,
if (tokensWithBalance.length) {
this.#trackMetaMetricsEvent({
event: 'Token Detected',
category: 'Wallet',
properties: {
tokens: eventTokensDetails,
token_standard: 'ERC20',
asset_type: 'TOKEN',
},
});

await this.messagingSystem.call(
'TokensController:addDetectedTokens',
tokensWithBalance,
{
selectedAddress: this.#addressAgainstWhichToDetect,
chainId: this.#chainIdAgainstWhichToDetect,
},
);
const tokensWithBalance: Token[] = [];
const eventTokensDetails: string[] = [];
for (const nonZeroTokenAddress of Object.keys(balances)) {
const { decimals, symbol, aggregators, iconUrl, name } =
tokenListUsed[nonZeroTokenAddress];
eventTokensDetails.push(`${symbol} - ${nonZeroTokenAddress}`);
tokensWithBalance.push({
address: nonZeroTokenAddress,
decimals,
symbol,
aggregators,
image: iconUrl,
isERC721: false,
name,
});
}
if (tokensWithBalance.length) {
this.#trackMetaMetricsEvent({
event: 'Token Detected',
category: 'Wallet',
properties: {
tokens: eventTokensDetails,
token_standard: 'ERC20',
asset_type: 'TOKEN',
},
});
await this.messagingSystem.call(
'TokensController:addDetectedTokens',
tokensWithBalance,
{
selectedAddress: addressAgainstWhichToDetect,
chainId: chainIdAgainstWhichToDetect,
},
);
}
});
}
}
});
Copy link
Contributor Author

@MajorLift MajorLift Feb 23, 2024

Choose a reason for hiding this comment

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

Large diff, but no actual changes here other than being moved to the new #addDetectedTokens method, and variable name updates (this.#{address,networkClientId,chainId}AgainstWhichToDetect, this.#tokenList).

@MajorLift MajorLift force-pushed the 240215-TokenDetectionController-sync-with-extension branch 2 times, most recently from 8a73dde to e9bbf1d Compare February 23, 2024 19:04
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch 2 times, most recently from 606211e to 70f1a26 Compare February 26, 2024 17:32
Comment on lines +430 to +441
const { selectedNetworkClientId } = this.messagingSystem.call(
'NetworkController:getState',
);
const newNetworkClientId = this.messagingSystem.call(
'NetworkController:findNetworkClientIdByChainId',
chainId,
const {
configuration: { chainId },
} = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
selectedNetworkClientId,
);
return {
chainId,
networkClientId: newNetworkClientId,
networkClientId: selectedNetworkClientId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaces getProviderConfig, findNetworkClientIdByChainId actions with getState, getNetworkClientById

I used getNetworkClientById instead of getNetworkConfigurationByNetworkClientId to avoid having to use this.#chainId as a fallback value for the case where the returned network configuration is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bergeron @jiexi @adonesky1 Thoughts on this approach for retrieving networkClientId, chainId? c75fb3b

Copy link
Contributor

Choose a reason for hiding this comment

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

That approach looks okay to me, but wondering if you could get both in 1 call:

    const {
      selectedNetworkClientId,
      providerConfig: { chainId },
    } = this.messagingSystem.call('NetworkController:getState');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good to me as long as there's no edge case where the selected network client's chainId is inconsistent with providerConfig. @mcmire What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied here: d6caee2

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. We might as well do the same thing for lines 418-428, huh?

Copy link
Contributor Author

@MajorLift MajorLift Mar 2, 2024

Choose a reason for hiding this comment

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

I'm a little confused by that section. For detectTokens to work independently of the class state, shouldn't the user be able to pass in a network client ID that's not the currently selected network's and get the corresponding chain ID?

It seems like 418-428 is handling this case where the user-supplied network client ID is the source of truth, instead of the selectedNetworkClientId in NetworkState.

Otherwise, if the only network client ID ever used in detectTokens is the selectedNetworkClientId from NetworkState, shouldn't we remove the networkClientId params from detectTokens, restartTokenDetection, and getCorrect...?

Copy link
Contributor

@mcmire mcmire Mar 4, 2024

Choose a reason for hiding this comment

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

Oh, sorry. What I meant was that it looks like further up we are using getNetworkConfigurationByNetworkClientId to get the configuration for the given network client, then we are checking to see whether we have a configuration, then we are getting the chain ID from the configuration. But in your changes here you are using getNetworkClientById. So do we want to be consistent? I'm thinking this:

  #getCorrectChainIdAndNetworkClientId(networkClientId?: NetworkClientId): {
    chainId: Hex;
    networkClientId: NetworkClientId;
  } {
    if (networkClientId) {
      const {
        configuration: { chainId },
      } = this.messagingSystem.call(
        'NetworkController:getNetworkClientById',
        networkClientId,
      );
      return {
        chainId,
        networkClientId,
      };
    }

    const { selectedNetworkClientId } = this.messagingSystem.call(
      'NetworkController:getState',
    );
    const {
      configuration: { chainId },
    } = this.messagingSystem.call(
      'NetworkController:getNetworkClientById',
      selectedNetworkClientId,
    );
    return {
      chainId,
      networkClientId: selectedNetworkClientId,
    };
  }

And if we can do this, then we can flip it around:

  #getCorrectChainIdAndNetworkClientId(givenNetworkClientId?: NetworkClientId): {
    chainId: Hex;
    networkClientId: NetworkClientId;
  } {
    const networkClientId =
      givenNetworkClientId ??
      this.messagingSystem.call(
        'NetworkController:getState',
      ).selectedNetworkClientId;
    const {
      configuration: { chainId },
    } = this.messagingSystem.call(
      'NetworkController:getNetworkClientById',
      networkClientId,
    );
    return { chainId, networkClientId };
  }

Copy link
Contributor Author

@MajorLift MajorLift Mar 4, 2024

Choose a reason for hiding this comment

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

The reason I made that distinction was that getNetworkConfigurationByNetworkClientId returns undefined if there's no network configuration for the given network client ID.

So by using that first instead of getNetworkClientById (which never returns undefined), I thought we'd be able to rule out any user-supplied networkClientId that's not registered in the network state. If that's the case, we default to the selected network client ID.

But if that's not a meaningful edge case then the second option looks good I can apply it rn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see what you're saying. Okay, that makes sense!

Base automatically changed from 240215-TokenDetectionController-sync-with-extension to main February 27, 2024 00:13
MajorLift added a commit that referenced this pull request Feb 27, 2024
…dates (#3923)

## Explanation

As a preparatory step for fully replacing the extension
`DetectTokensController` with the consolidated core repo
`TokenDetectionController`, `TokenDetectionController` needs to be
updated with changes made to the extension `DetectTokensController`
since #1813 was closed.

#### Diff of `DetectTokensController` state
-
[MetaMask/metamask-extension@`5d285f7be5f7be981995dfa725aad97d81cc990a..85cd1c89039e900b452edb704ec37e9ccbd3e76a`#diff-323d0cf464](https://github.com/MetaMask/metamask-extension/compare/5d285f7be5f7be981995dfa725aad97d81cc990a..85cd1c89039e900b452edb704ec37e9ccbd3e76a#diff-323d0cf46498be3850b971474905354ea5ccf7fa13745ad1e6eba59c5b586830)

### Differences from extension `DetectTokensController`

- Refactors logic for retrieving `chainId`, `networkClientId` into
`this.#getCorrectChainIdAndNetworkClientId`
- Uses `getNetworkConfigurationByNetworkClientId` action instead of
`getNetworkClientById` to retrieve `chainId`.
- If `networkClientId` is not supplied to the method, or it's supplied
but `getNetworkConfigurationByNetworkClientId` returns `undefined`,
finds `chainId` from `providerConfig`.

- `detectTokens` replaces `detectNewTokens`
- `detectTokens` accepts options object `{ selectedAddress,
networkClientId }` instead of `{ selectedAddress, chainId,
networkClientId }`.
- Does not throw error if `getBalancesInSingleCall` fails. Also does not
exit early -- continues looping.
- Passes lists of full `Token` types to
`TokensController:addDetectedTokens` instead of objects containing only
`{ address, decimals, symbol }`.
  
- `#trackMetaMetricsEvents` is a private method instead of protected.
- Passes string literals instead of extension shared constants into
`_trackMetaMetricsEvent`.

## References

- Partially implements #3916
- Blocking #3918
- Changes adopted from:
  - MetaMask/metamask-extension#22898
  - MetaMask/metamask-extension#22814
  - #3914
  - MetaMask/metamask-extension#21437
- Blocking (Followed by) #3938

## Changelog

###
[`@metamask/assets-controllers`](https://github.com/MetaMask/core/pull/3923/files#diff-ee47d03d53776b8dd530799a8047f5e32e36e35765620aeb50b294adc3339fab)

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: jiexi <[email protected]>
…IdAndNetworkClientId`, `#getTokenListAndSlicesOfTokensToDetect`, `#addDetectedTokens`

- Fixes #1614
- Maintains distinction between private class fields `#{chainId,selectedAddress,networkClientId}{,AgainstWhichToDetect}`, so that `detectTokens` method can be used independently of polling/passive detection (convert to static method?). Is this expected behavior?
…yChainId` actions with `getState`, `getNetworkClientById`
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch from 70f1a26 to a09fcda Compare February 27, 2024 00:20
@MajorLift MajorLift force-pushed the 240216-TokenDetectionController-detectTokens-refactor branch from f5c3194 to f060ced Compare February 27, 2024 16:00
bergeron
bergeron previously approved these changes Feb 27, 2024
@MajorLift MajorLift mentioned this pull request Mar 1, 2024
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.

Makes sense overall, but I had some comments.

packages/assets-controllers/CHANGELOG.md Outdated Show resolved Hide resolved
NetworkClientId,
} from '@metamask/network-controller';
import { defaultState as defaultNetworkState } from '@metamask/network-controller';
import type { AutoManagedNetworkClient } from '@metamask/network-controller/src/create-auto-managed-network-client';
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up: #3998 will make it impossible to import subpaths, so this will need to be changed when that happens, but is okay for now.

Copy link
Contributor Author

@MajorLift MajorLift Mar 2, 2024

Choose a reason for hiding this comment

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

Yes I was surprised to learn that AutoManagedNetworkClient isn't a package-level export. Is this an intentional choice? Otherwise I could create a ticket for exporting this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't. I don't think I expected anyone to have to use this type, as it's low-level. But there's no harm in exporting it or anything.

Comment on lines 163 to 167
#chainIdAgainstWhichToDetect: Hex;

#addressAgainstWhichToDetect: string;

#networkClientIdAgainstWhichToDetect: NetworkClientId;
Copy link
Contributor

@mcmire mcmire Mar 1, 2024

Choose a reason for hiding this comment

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

You're right that consumers should be able to call detectTokens independently, and it should just work.

It makes sense that we would cache the currently selected network as well as the currently selected account. I don't think we need to cache the chain ID, because we can always ask the network client. (But removing that is for another PR.)

As for these three variables, though, it seems to me that we've created them so that we can break up detectTokens into pieces. But, why not pass along these values, then? It seems that this would make it easier for someone to read the code because there would be fewer things for them to track.

const { chainId } =
this.#addressAgainstWhichToDetect = this.#selectedAddress;

const { chainId, networkClientId: correctNetworkClientId } =
this.#getCorrectChainIdAndNetworkClientId(networkClientId);
Copy link
Contributor

Choose a reason for hiding this comment

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

With your changes to #getCorrectChainIdAndNetworkClientId, it will retrieve the currently selected network from NetworkController. So does the constructor need to take a networkClientId? It seems like we don't need to pass it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm looks like this is also related to #3938 (comment). If we allow #getCorrectChainIdAndNetworkClientId to accept a non-selected network client id and then return its chain id, then keeping the networkClientId constructor option would also allow token-detection-controller's internal state to diverge from network-controller's.

Removing this will be fairly disruptive for the tests, but it looks like it'll be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied here: d0f6f69 (#3938)

Comment on lines +430 to +441
const { selectedNetworkClientId } = this.messagingSystem.call(
'NetworkController:getState',
);
const newNetworkClientId = this.messagingSystem.call(
'NetworkController:findNetworkClientIdByChainId',
chainId,
const {
configuration: { chainId },
} = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
selectedNetworkClientId,
);
return {
chainId,
networkClientId: newNetworkClientId,
networkClientId: selectedNetworkClientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. We might as well do the same thing for lines 418-428, huh?

MajorLift and others added 4 commits March 1, 2024 17:44
… class field is always initialized with `selectedNetworkClientId` property of network-controller state
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.

This looks good to me!

@MajorLift MajorLift merged commit 34bf8f0 into main Mar 4, 2024
139 checks passed
@MajorLift MajorLift deleted the 240216-TokenDetectionController-detectTokens-refactor branch March 4, 2024 17:21
@MajorLift MajorLift restored the 240216-TokenDetectionController-detectTokens-refactor branch March 4, 2024 18:29
Gudahtt added a commit that referenced this pull request Mar 5, 2024
* origin/main:
  refactor(controller-utils): replace `any` with types for type safety (#3975)
  Release 123.0.0 (#4007)
  [token-detection-controller] Refactor `detectTokens` method (#3938)
  fix: update usage of OP goerli to OP Sepolia (#3999)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants