-
-
Notifications
You must be signed in to change notification settings - Fork 261
Adding TokenListController to fetch the token list from token services API #478
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
Conversation
3805dc6 to
aac1d7d
Compare
86ddfb7 to
3d6e2c4
Compare
…r tokens in asset.state in AssestsDetectionController
77ec776 to
aeed783
Compare
|
@NiranjanaBinoy do you still need more input / feedback from Mobile? Thank you! |
Cal-L
left a comment
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.
Left some comments. Great work! 👍
|
Should there be a method present on the controller to update the chainId on the |
|
@adonesky1 I thiknk that is setup differently in controllers repo: see these lines EDIT: onNetworkStateChange: (callback) => {
this.networkController.on(NETWORK_EVENTS.NETWORK_DID_CHANGE, () => {
const chainId = this.networkController.getCurrentChainId();
callback({ chainId });
});
},Alternatively we might want to think about emitting a CHAIN_CHANGED event somewhere too that gets the new chainId emitted. NETWORK_DID_CHANGE in extension only emits the network type (mainnet/rinkeby/rpc) |
Oh weird I had pulled down and was looking at an older version of this PR that didn't contain those lines... LGTM |
…tching token on network change and test case updations.
brad-decker
left a comment
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.
One NIT that may be easily dismissed, curious about the follow up question though! Leaving this as 'request changes' but should be easy to convert me to a ✅
brad-decker
left a comment
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.
LGTM
The purpose of the controller is to poll the new token detection API
tokens/:chainIdat regular intervals to get the list of tokens dynamically instead of getting it from@metamask/contract-metadata. The controller also calls on the/sync/:chainIdAPI which initiates sync on the token list in the background andtokens/:chainId?address=<tokenAddress>which returns the metadata of the token whose address is provided.This controller will be used by
detect-token.js(DetectTokensController) frommetamask-extensionrepo andAssetsDetectionController.tsfrommetamask/controllersrepo.AssetsDetectionController.tsis updated as a part of this PR to use the TokeListcontroller as a source of the token list instead of@metamask/contract-metadata.