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

feat: multi-chain-token-detection #4894

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented Nov 4, 2024

Explanation

This PR modifies the token detection controller to support detecting tokens across multiple network client IDs at once, instead of handling only a single network client ID. This enhancement allows for more efficient token detection and state updates across multiple chains in a single polling cycle.

Key Changes

Multi-Chain Token Detection:

Updated the token detection controller to accept an array of network client IDs as input.
On each polling interval, the controller will detect tokens and update the state for all chains in the provided array.

References

Changelog

@metamask/assets-controllers

  • CHANGED: the polling token autodetection take an array of chainIds now

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
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@salimtb salimtb force-pushed the salim/multi-chain-token-detection branch 3 times, most recently from dc12481 to fe1454b Compare November 4, 2024 10:48
@@ -394,28 +394,6 @@ export class TokenDetectionController extends StaticIntervalPollingController<To
}
},
);

this.messagingSystem.subscribe(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanation: we don't need to handle this case since the detection is multichain now , this is why we're removing this code

@salimtb salimtb force-pushed the salim/multi-chain-token-detection branch from fe1454b to b93d09a Compare November 4, 2024 10:52
@@ -501,6 +479,52 @@ export class TokenDetectionController extends StaticIntervalPollingController<To
return isEqualValues;
}

#getCorrectNetworkClientIdByChainId(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this function is to convert an array of chainId values into an array of objects, each containing { chainId, networkClientId } pairs. If a network is selected, it assigns selectedNetworkClientId as the networkClientId; otherwise, it uses the first available network client ID. This function will be used within detectTokens to facilitate token detection in a loop

@salimtb salimtb changed the title fix: multi-chain-detection feat: multi-chain-token-detection Nov 4, 2024
@salimtb salimtb marked this pull request as ready for review November 6, 2024 12:39
@salimtb salimtb requested a review from a team as a code owner November 6, 2024 12:39
@salimtb salimtb requested a review from bergeron November 6, 2024 12:40
@salimtb salimtb force-pushed the salim/multi-chain-token-detection branch 3 times, most recently from 11e986c to 5a2763f Compare November 11, 2024 12:17
@salimtb salimtb force-pushed the salim/multi-chain-token-detection branch from 5a2763f to bdcf0f0 Compare November 11, 2024 13:41
Comment on lines +506 to +514
const newDetectedTokens =
interactingChainId &&
allDetectedTokens[interactingChainId]?.[this.#getSelectedAddress()]
? allDetectedTokens[interactingChainId]?.[
this.#getSelectedAddress()
].filter(
(token: Token) => !importedTokensMap[token.address.toLowerCase()],
)
: [];
Copy link
Contributor

Choose a reason for hiding this comment

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

So much syntactic sugar here makes this too sweet 🤣

To summarise this:

  1. Grabs tokens for a specific chain, for a specific address
  2. Then we filter over these tokens to make sure that we get tokens that have not been imported before (newly detected tokens)
  3. Otherwise we will fallback to an empty array.

I wonder if we can clean this up a little (I think it's the ternary switch that adds complexity).

// Bad at naming
const foobar = allDetectedTokens?.[interactingChainId]?.[this.#getSelectedAddress()] ?? [];
const newDetectedTokens = foobar.filter(t => !importedTokensMap[t.address.toLowerCase()])

@@ -198,6 +198,84 @@ describe('TokensController', () => {
});
});

it('should add tokens and update existing ones and detected tokens', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - we can do this later in a future PR.

There is a lot of things happening in the controller/test. It would be nice to move/group some composable "arrange" functions to clean up these tests and make it clear what we are testing.

@salimtb salimtb merged commit 25e3182 into main Nov 12, 2024
120 checks passed
@salimtb salimtb deleted the salim/multi-chain-token-detection branch November 12, 2024 16:07
},
mocks: {
getAccount: selectedAccount,
getSelectedAccount: selectedAccount,
},
},

// Salim ....
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

@@ -2303,6 +2350,7 @@ describe('TokenDetectionController', () => {
options: {
disabled: false,
getBalancesInSingleCall: mockGetBalancesInSingleCall,
// useAccountsAPI: true, // USING ACCOUNTS API
Copy link
Contributor

Choose a reason for hiding this comment

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

clean up later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants