-
-
Notifications
You must be signed in to change notification settings - Fork 231
feat: add getErc20Balances helper TokenBalancesControllers #5925
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
base: main
Are you sure you want to change the base?
Conversation
32372f2
to
69122ec
Compare
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
c93d287
to
2a93861
Compare
* @returns The balances for the given token addresses. | ||
*/ | ||
async #batchBalanceOf({ | ||
chainId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do you think its better to pass the networkClientId instead of chainId?
We would also pass networkClientId in getErc20Balances instead of chainId
@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
- Added Base Network for networks to track in `TokenDetectionController` ([#5902](https://github.com/MetaMask/core/pull/5902)) | |||
- Network changes were added in `@metamask/controller-utils` | |||
- Add `getErc20Balances` function within `TokenBalancesController` to support fetching ERC-20 token balances for a given address and token list ([#5925](https://github.com/MetaMask/core/pull/5925)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we add this Changelog before the "- Added Base Network for network..."
tokenAddresses: Hex[]; | ||
}): Promise<Record<Hex, Hex | null>> { | ||
// Return early if no token addresses provided | ||
if (tokenAddresses.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to return early here; wondering if there is a specific use case you had in mind for adding the condition?
I think this fct will be used on clients to fetch balances on the fly for 1 or more tokens in the import flow; i , for now, dnt expect it to be called with an empty array; but makes sense to me to keep the check!
tokenAddresses.forEach((tokenAddress, i) => { | ||
balances[tokenAddress] = results[i]?.success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment - So this is a 1:1 mapping?
Reason I'm asking is below (in the updateBalancesByChainId
) we loop through the results and then index to get the tokenAddress; over here we are doing it the other way round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave as is, this was mostly a comment.
Explanation
This change introduces a new
getErc20Balances
service function within theTokenBalancesController
. The goal is to provide a reusable and isolated utility to fetch ERC-20 token balances for a given address and token list.The service is intended to be used internally by the controller or other consumers needing direct access to raw ERC-20 balances, while abstracting away Web3 calls and error handling.
References
Changelog
@metamask/assets-controllers
UPDATE: Created getErc20Balances function within TokenBalancesController to support fetching ERC-20 token balances for a given address and token list. This modular service simplifies balance retrieval logic and can be reused across different parts of the controller.
Checklist