Integrate PollingController into TokenRatesController
#2015
Conversation
…p-1025/multichain-v1-currency-rate-controller # Conflicts: # packages/network-controller/src/NetworkController.ts # packages/network-controller/tests/NetworkController.test.ts
…p-1025/multichain-v1-currency-rate-controller
| * should be used to query token exchange rates. | ||
| * @param tokenAddresses - The addresses for the token contracts. | ||
| * @returns An object with conversion rates for each token | ||
| * related to the network's native currency. | ||
| */ | ||
| async fetchAndMapExchangeRates( |
There was a problem hiding this comment.
Can this be made private? I'm not seeing it used by either of the clients or other controllers?
There was a problem hiding this comment.
typescript wouldn't be happy with that since these methods are tested directly in spec
There was a problem hiding this comment.
I'm not seeing this one in the test file?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok lets make this private in a subsequent PR per @mcmire 's suggestion
| * @param vsCurrency - Query according to tokens in tokenList and native currency. | ||
| * @param chainSlug - The chain string identifier. | ||
| * @param vsCurrency - The currency used to generate pairs against the tokens. | ||
| * @param tokenAddresses - The addresses for the token contracts. | ||
| * @returns The exchange rates for the given pairs. | ||
| */ | ||
| async fetchExchangeRate( |
There was a problem hiding this comment.
Also can this be made private? Helps to keep track of what changes should be considered and breaking and how to think about the method signatures
There was a problem hiding this comment.
This one is stubbed out a few times but we should be able to test the public methods without referring to it. I know its kindof annoying but it will make our tests better and this controller better if we make it private.
There was a problem hiding this comment.
had to do this because i couldn't get nock/fetch to play nicely with fake timers, I can take another look at this again though
There was a problem hiding this comment.
Ok lets make this private in a subsequent PR per @mcmire 's suggestion
| */ | ||
| async getChainSlug(): Promise<string | null> { | ||
| const { threshold, chainId } = this.config; | ||
| async getChainSlug( |
There was a problem hiding this comment.
Another one we can make private.
There was a problem hiding this comment.
I also don't see this one referenced in the test file
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh you're adding them in your PR... I was looking on main... so yea I'm sorry cuz you put in the effort to create tests for these but really they are essentially private methods that just weren't marked as such and I feel like we should mark them as private now. Lets see what @mcmire and @Gudahtt think?
There was a problem hiding this comment.
Less API surface area generally sounds good to me!
Alternatively, if we did want proper test coverage of an internal method we could pull it out into a separate module, and test it that way. Then it can still be private (in that it's not exported by the package), but will follow our convention of reducing API surface area and not testing private things.
The guideline of not testing private methods is really a nudge to organize code into more cohesive chunks.
There was a problem hiding this comment.
Lets make this private in a subsequent PR per @mcmire 's suggestion
Co-authored-by: Alex Donesky <adonesky@gmail.com>
Co-authored-by: Alex Donesky <adonesky@gmail.com>
Co-authored-by: Alex Donesky <adonesky@gmail.com>
|
Tracking remaining cleanup for marking methods private and updating specs in this issue |
|
FYI I've added this PR to the next release: #2063. |
Explanation
Integrates
PollingControllerintoTokensRatesController, keeping as much of the current method interfaces intact. Needed for eventual multichain support.References
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1122
Changelog
@metamask/assets-controllersTokenRatesControllerconstructor params now requiresgetNetworkClientByIdTokenRatesControllerstate now hascontractExchangeRatesByChainIdproperty which an object keyed bychainIdandnativeCurrencytokenRatesController.state.contractExchangeRatesByChainId[chainId][nativeCurrency]TokenRatesControllerconstructor params now accepts optionalintervalandthresholdupdateExchangeRatesByChainId,_executePoll,startPollingByNetworkClientId,stopAllPolling, andstopPollingByPollingTokenmethods toTokenRatesControllerTokenRatesController.fetchExchangeRate()now accepts an optionaltokenAddressesas the last parameterTokenRatesController.getChainSlug()now accepts an optionaltokenAddressesparameterTokenRatesController.fetchAndMapExchangeRates()now accepts an optionaltokenAddressesas the last parameterChecklist