diff --git a/CHANGELOG.md b/CHANGELOG.md index 317a37d22a..197d4f6d0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [10.0.0] +### Fixed +- **BREAKING**: Fix stale conversionRate after switching network ([#465](git+https://github.com/MetaMask/controllers/pull/465)) + - The breaking change is the change in type of the `conversionRate` state of the `CurrencyRateController` - it's now nullable. + ## [9.1.0] - 2021-05-20 ### Added - Add support for unicode domains to PhishingController ([#471](https://github.com/MetaMask/controllers/pull/471)) @@ -243,7 +248,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Removed - Remove shapeshift controller (#209) -[Unreleased]: https://github.com/MetaMask/controllers/compare/v9.1.0...HEAD +[Unreleased]: https://github.com/MetaMask/controllers/compare/v10.0.0...HEAD +[10.0.0]: https://github.com/MetaMask/controllers/compare/v9.1.0...v10.0.0 [9.1.0]: https://github.com/MetaMask/controllers/compare/v9.0.0...v9.1.0 [9.0.0]: https://github.com/MetaMask/controllers/compare/v8.0.0...v9.0.0 [8.0.0]: https://github.com/MetaMask/controllers/compare/v7.0.0...v8.0.0 diff --git a/package.json b/package.json index 0c11a5584b..e38c92b88f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/controllers", - "version": "9.1.0", + "version": "10.0.0", "description": "Collection of platform-agnostic modules for creating secure data models for cryptocurrency wallets", "license": "MIT", "files": [ @@ -69,7 +69,7 @@ "web3-provider-engine": "^16.0.1" }, "devDependencies": { - "@metamask/auto-changelog": "^2.0.1", + "@metamask/auto-changelog": "^2.1.0", "@metamask/eslint-config": "^6.0.0", "@metamask/eslint-config-jest": "^6.0.0", "@metamask/eslint-config-nodejs": "^6.0.0", diff --git a/src/apis/crypto-compare.test.ts b/src/apis/crypto-compare.test.ts index 8f37f2d821..fb78846a09 100644 --- a/src/apis/crypto-compare.test.ts +++ b/src/apis/crypto-compare.test.ts @@ -27,19 +27,6 @@ describe('CryptoCompare', () => { expect(conversionRate).toStrictEqual(2000.42); }); - it('should return conversion date', async () => { - nock(cryptoCompareHost) - .get('/data/price?fsym=ETH&tsyms=CAD') - .reply(200, { CAD: 2000.42 }); - - const before = Date.now() / 1000; - const { conversionDate } = await fetchExchangeRate('CAD', 'ETH'); - const after = Date.now() / 1000; - - expect(conversionDate).toBeGreaterThanOrEqual(before); - expect(conversionDate).toBeLessThanOrEqual(after); - }); - it('should return CAD conversion rate given lower-cased currency', async () => { nock(cryptoCompareHost) .get('/data/price?fsym=ETH&tsyms=CAD') @@ -152,4 +139,17 @@ describe('CryptoCompare', () => { 'Invalid response for usdConversionRate: invalid', ); }); + + it('should throw an error if either currency is invalid', async () => { + nock(cryptoCompareHost) + .get('/data/price?fsym=ETH&tsyms=EUABRT') + .reply(200, { + Response: 'Error', + Message: 'Market does not exist for this coin pair', + }); + + await expect(fetchExchangeRate('EUABRT', 'ETH')).rejects.toThrow( + 'Market does not exist for this coin pair', + ); + }); }); diff --git a/src/apis/crypto-compare.ts b/src/apis/crypto-compare.ts index 4e1f292b91..593f7aa817 100644 --- a/src/apis/crypto-compare.ts +++ b/src/apis/crypto-compare.ts @@ -25,14 +25,27 @@ export async function fetchExchangeRate( nativeCurrency: string, includeUSDRate?: boolean, ): Promise<{ - conversionDate: number; conversionRate: number; usdConversionRate: number; }> { const json = await handleFetch( getPricingURL(currency, nativeCurrency, includeUSDRate), ); + + /* + Example expected error response (if pair is not found) + { + Response: "Error", + Message: "cccagg_or_exchange market does not exist for this coin pair (ETH-)", + HasWarning: false, + } + */ + if (json.Response === 'Error') { + throw new Error(json.Message); + } + const conversionRate = Number(json[currency.toUpperCase()]); + const usdConversionRate = Number(json.USD); if (!Number.isFinite(conversionRate)) { throw new Error( @@ -46,7 +59,6 @@ export async function fetchExchangeRate( } return { - conversionDate: Date.now() / 1000, conversionRate, usdConversionRate, }; diff --git a/src/assets/CurrencyRateController.test.ts b/src/assets/CurrencyRateController.test.ts index 9cb1a8a5c5..c5cceb88a3 100644 --- a/src/assets/CurrencyRateController.test.ts +++ b/src/assets/CurrencyRateController.test.ts @@ -37,9 +37,15 @@ function getRestrictedMessenger() { return messenger; } +const getStubbedDate = () => { + return new Date('2019-04-07T10:20:30Z').getTime(); +}; + describe('CurrencyRateController', () => { + const realDate = Date.now; afterEach(() => { nock.cleanAll(); + global.Date.now = realDate; }); it('should set default state', () => { @@ -245,4 +251,75 @@ describe('CurrencyRateController', () => { controller.destroy(); }); + + it('should throw unexpected errors', async () => { + const cryptoCompareHost = 'https://min-api.cryptocompare.com'; + nock(cryptoCompareHost) + .get('/data/price?fsym=ETH&tsyms=XYZ') + .reply(200, { + Response: 'Error', + Message: 'this method has been deprecated', + }) + .persist(); + + const messenger = getRestrictedMessenger(); + const controller = new CurrencyRateController({ + messenger, + state: { currentCurrency: 'xyz' }, + }); + + await expect(controller.updateExchangeRate()).rejects.toThrow( + 'this method has been deprecated', + ); + controller.destroy(); + }); + + it('should catch expected errors', async () => { + const cryptoCompareHost = 'https://min-api.cryptocompare.com'; + nock(cryptoCompareHost) + .get('/data/price?fsym=ETH&tsyms=XYZ') + .reply(200, { + Response: 'Error', + Message: 'market does not exist for this coin pair', + }) + .persist(); + + const messenger = getRestrictedMessenger(); + const controller = new CurrencyRateController({ + messenger, + state: { currentCurrency: 'xyz' }, + }); + + await controller.updateExchangeRate(); + expect(controller.state.conversionRate).toBeNull(); + controller.destroy(); + }); + + it('should update conversionRates in state to null if either currentCurrency or nativeCurrency is null', async () => { + jest.spyOn(global.Date, 'now').mockImplementation(() => getStubbedDate()); + const cryptoCompareHost = 'https://min-api.cryptocompare.com'; + nock(cryptoCompareHost) + .get('/data/price?fsym=ETH&tsyms=XYZ') + .reply(200, { XYZ: 2000.42 }) + .persist(); + + const messenger = getRestrictedMessenger(); + const existingState = { currentCurrency: '', nativeCurrency: 'BNB' }; + const controller = new CurrencyRateController({ + messenger, + state: existingState, + }); + + await controller.updateExchangeRate(); + + expect(controller.state).toStrictEqual({ + conversionDate: getStubbedDate() / 1000, + conversionRate: null, + currentCurrency: '', + nativeCurrency: 'BNB', + pendingCurrentCurrency: null, + pendingNativeCurrency: null, + usdConversionRate: null, + }); + }); }); diff --git a/src/assets/CurrencyRateController.ts b/src/assets/CurrencyRateController.ts index 0dc925c9b7..ba14e02be3 100644 --- a/src/assets/CurrencyRateController.ts +++ b/src/assets/CurrencyRateController.ts @@ -20,7 +20,7 @@ import type { RestrictedControllerMessenger } from '../ControllerMessenger'; */ export type CurrencyRateState = { conversionDate: number; - conversionRate: number; + conversionRate: number | null; currentCurrency: string; nativeCurrency: string; pendingCurrentCurrency: string | null; @@ -190,35 +190,59 @@ export class CurrencyRateController extends BaseController< async updateExchangeRate(): Promise { const releaseLock = await this.mutex.acquire(); const { - currentCurrency, - nativeCurrency, + currentCurrency: stateCurrentCurrency, + nativeCurrency: stateNativeCurrency, pendingCurrentCurrency, pendingNativeCurrency, } = this.state; + + const conversionDate: number = Date.now() / 1000; + let conversionRate: number | null = null; + let usdConversionRate: number | null = null; + const currentCurrency = pendingCurrentCurrency ?? stateCurrentCurrency; + const nativeCurrency = pendingNativeCurrency ?? stateNativeCurrency; + try { - const { - conversionDate, - conversionRate, - usdConversionRate, - } = await this.fetchExchangeRate( - pendingCurrentCurrency || currentCurrency, - pendingNativeCurrency || nativeCurrency, - this.includeUsdRate, - ); - this.update(() => { - return { - conversionDate, - conversionRate, - currentCurrency: pendingCurrentCurrency || currentCurrency, - nativeCurrency: pendingNativeCurrency || nativeCurrency, - pendingCurrentCurrency: null, - pendingNativeCurrency: null, - usdConversionRate, - }; - }); + if ( + currentCurrency && + nativeCurrency && + // if either currency is an empty string we can skip the comparison + // because it will result in an error from the api and ultimately + // a null conversionRate either way. + currentCurrency !== '' && + nativeCurrency !== '' + ) { + ({ conversionRate, usdConversionRate } = await this.fetchExchangeRate( + currentCurrency, + nativeCurrency, + this.includeUsdRate, + )); + } + } catch (error) { + if (!error.message.includes('market does not exist for this coin pair')) { + throw error; + } } finally { - releaseLock(); + try { + this.update(() => { + return { + conversionDate, + conversionRate, + // we currently allow and handle an empty string as a valid nativeCurrency + // in cases where a user has not entered a native ticker symbol for a custom network + // currentCurrency is not from user input but this protects us from unexpected changes. + nativeCurrency, + currentCurrency, + pendingCurrentCurrency: null, + pendingNativeCurrency: null, + usdConversionRate, + }; + }); + } finally { + releaseLock(); + } } + return this.state; } } diff --git a/yarn.lock b/yarn.lock index 54967b2443..7d949c6b33 100644 --- a/yarn.lock +++ b/yarn.lock @@ -658,10 +658,10 @@ "@types/yargs" "^15.0.0" chalk "^4.0.0" -"@metamask/auto-changelog@^2.0.1": - version "2.0.1" - resolved "https://registry.yarnpkg.com/@metamask/auto-changelog/-/auto-changelog-2.0.1.tgz#c5e9099c11f05d5e77fea91722a612c971764eff" - integrity sha512-ugIjpnPcf7nhIKEpGb4bfwTILzfwQae2aNJhrqxicY9LqI+CuTNA4+VFJIaX6zRYrgFINMYpLMgYvMB3Gj61kA== +"@metamask/auto-changelog@^2.1.0": + version "2.1.0" + resolved "https://registry.yarnpkg.com/@metamask/auto-changelog/-/auto-changelog-2.1.0.tgz#5cb546f05b07695476d9489540b8d2744d350eda" + integrity sha512-FO1NsgcrHQY6CWlD9wFEWgmxWgam1GqtfFGiRH7lp4aYSGn/43ktBMkZ/rF0//7XvUDXn27nC2CClRUBCrg8Jg== dependencies: cross-spawn "^7.0.3" diff "^5.0.0"