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.

Sign up for GitHub

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

Fix detected tokens added to wrong network #22814

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Fix detected tokens added to wrong network #22814

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4c1cb8
.
bergeron Jan 28, 2024
7b066d6
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
bergeron Feb 5, 2024
6550633
undo batching change
bergeron Feb 5, 2024
2ad1f4f
Merge branch 'develop' into brian/token-detection-network-switch
bergeron Feb 8, 2024
5e6b428
removed unused code
bergeron Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 14 additions & 18 deletions app/scripts/controllers/detect-tokens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,7 @@ export default class DetectTokensController extends StaticIntervalPollingControl
this.useTokenDetection =
this.preferences?.store.getState().useTokenDetection;
this.selectedAddress = getCurrentSelectedAccount().address;
this.tokenAddresses = this.tokensController?.state.tokens.map((token) => {
return token.address;
});
this.setIntervalLength(interval);
this.hiddenTokens = this.tokensController?.state.ignoredTokens;
this.detectedTokens = this.tokensController?.state.detectedTokens;
this.chainId = this.getChainIdFromNetworkStore();
this._trackMetaMetricsEvent = trackMetaMetricsEvent;

Expand Down Expand Up @@ -101,15 +96,6 @@ export default class DetectTokensController extends StaticIntervalPollingControl
}
});

tokensController?.subscribe(
({ tokens = [], ignoredTokens = [], detectedTokens = [] }) => {
this.tokenAddresses = tokens.map((token) => {
return token.address;
});
this.hiddenTokens = ignoredTokens;
this.detectedTokens = detectedTokens;
},
);
Comment on lines -104 to -112
Copy link
Contributor Author

@bergeron bergeron Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

tokensController?.subscribe was typically the last event to fire during a chain switch. Therefore tokenAddresses, hiddenTokens, and detectedTokens were still from the old chain during detection, even when (most) everything else is pointing to the new chain. Since tokensController has state per-chain, we can just grab the tokens for the correct chain instead of waiting to be updated via subscription.

messenger.subscribe('NetworkController:stateChange', () => {
if (this.chainId !== this.getChainIdFromNetworkStore()) {
const chainId = this.getChainIdFromNetworkStore();
Expand Down Expand Up @@ -169,13 +155,19 @@ export default class DetectTokensController extends StaticIntervalPollingControl
const tokensToDetect = [];
for (const tokenAddress in tokenListUsed) {
if (
!this.tokenAddresses.find((address) =>
!this.tokensController.state.allTokens?.[chainIdAgainstWhichToDetect]?.[
addressAgainstWhichToDetect
]?.find(({ address }) =>
isEqualCaseInsensitive(address, tokenAddress),
) &&
!this.hiddenTokens.find((address) =>
!this.tokensController.state.allIgnoredTokens?.[
chainIdAgainstWhichToDetect
]?.[addressAgainstWhichToDetect]?.find((address) =>
isEqualCaseInsensitive(address, tokenAddress),
) &&
!this.detectedTokens.find(({ address }) =>
!this.tokensController.state.allDetectedTokens?.[
chainIdAgainstWhichToDetect
]?.[addressAgainstWhichToDetect]?.find(({ address }) =>
isEqualCaseInsensitive(address, tokenAddress),
)
) {
Expand All @@ -192,6 +184,9 @@ export default class DetectTokensController extends StaticIntervalPollingControl
result = await this.assetsContractController.getBalancesInSingleCall(
addressAgainstWhichToDetect,
tokensSlice,
this.network.findNetworkClientIdByChainId(
chainIdAgainstWhichToDetect,
),
Comment on lines +187 to +189
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels like maybe the sketchiest one. We weren't passing an explicit NetworkClientId here, which means AssetsContractController falls back to the chain in its state. But that controller's onNetworkDidChange has not always fired yet, so its still on the old chain, and we detect the new token addresses on the old chain.

This was the only way I found to go from chain id -> NetworkClientId since that's what getBalancesInSingleCall accepts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Brian, yeah so this is the best option available for right now. You've correctly identified an issue with this flow. We have implemented an alternative polling solution that we will be leveraging shortly but you've highlighted one piece that we're missing. @shanejonas we will need to pass along the networkClientId in the _executePoll method.

Copy link
Contributor

Choose a reason for hiding this comment

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

created a ticket to do that cleanup: https://app.zenhub.com/workspaces/devex-63bee08a4e3b9d001108416e/issues/gh/metamask/metamask-planning/2097

Copy link
Contributor

Choose a reason for hiding this comment

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

#22898

);
} catch (error) {
warn(
Expand Down Expand Up @@ -247,7 +242,8 @@ export default class DetectTokensController extends StaticIntervalPollingControl
*/
restartTokenDetection({ selectedAddress, chainId } = {}) {
const addressAgainstWhichToDetect = selectedAddress ?? this.selectedAddress;
const chainIdAgainstWhichToDetect = chainId ?? this.chainId;
const chainIdAgainstWhichToDetect =
chainId ?? this.getChainIdFromNetworkStore();
Comment on lines +245 to +246
Copy link
Contributor Author

Choose a reason for hiding this comment

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

During a network switch, this.chainId is from the old chain until NetworkController:stateChange fires and updates it. But TokenListController:stateChange can fire first, leaving it stale at this point. This change gets the updated chain id from the network controller, as happens elsewhere in this file.

if (!(this.isActive && addressAgainstWhichToDetect)) {
return;
}
Expand Down
Loading