From 8214d55f307171355f008376e9ab656a8de59905 Mon Sep 17 00:00:00 2001 From: Michael Elliot Date: Mon, 24 Feb 2020 16:31:25 +0800 Subject: [PATCH] Change multicall error validation behaviour Change error validation behaviour to prevent entire observable stream from erroring giving errorred values an opportunity to update with valid values at a later time --- .../dai-plugin-mcd/src/schemas/_validators.js | 12 ++- .../dai-plugin-mcd/src/schemas/cdpManager.js | 8 +- .../dai-plugin-mcd/src/schemas/computed.js | 16 +-- .../dai-plugin-mcd/src/schemas/getCdps.js | 5 + .../src/schemas/proxyRegistry.js | 15 +-- packages/dai-plugin-mcd/src/schemas/token.js | 12 ++- .../dai-plugin-mcd/test/schemas/cat.spec.js | 5 +- .../test/schemas/cdpManager.spec.js | 5 +- .../test/schemas/computed.spec.js | 100 ++++++++++++------ .../dai-plugin-mcd/test/schemas/pot.spec.js | 12 ++- .../test/schemas/proxyRegistry.spec.js | 13 ++- .../dai-plugin-mcd/test/schemas/spot.spec.js | 22 ++-- packages/dai/src/eth/MulticallService.js | 66 ++++++++++-- .../dai/test/eth/MulticallService.spec.js | 26 +++-- 14 files changed, 215 insertions(+), 102 deletions(-) diff --git a/packages/dai-plugin-mcd/src/schemas/_validators.js b/packages/dai-plugin-mcd/src/schemas/_validators.js index c536eff03..ef65ea630 100644 --- a/packages/dai-plugin-mcd/src/schemas/_validators.js +++ b/packages/dai-plugin-mcd/src/schemas/_validators.js @@ -8,7 +8,13 @@ export const tag = (strings, ...keys) => (...values) => { }; export const validateAddress = (...args) => address => - !/^0x[0-9a-fA-F]{40}$/.test(address) && tag(...args)({ address }); + (!/^0x[0-9a-fA-F]{40}$/.test(address) || + address === '0x0000000000000000000000000000000000000000') && + tag(...args)({ address: address === null ? '(null)' : address }); -export const validatProxyAddressResult = (...args) => (proxyAddress, [address]) => - proxyAddress === '0x0000000000000000000000000000000000000000' && tag(...args)({ address }); // prettier-ignore +export const validateVaultId = id => + !/^\d+$/.test(id) && + `Invalid vault id: must be a positive integer. Received ${id}`; + +export const validateVaultTypeResult = vaultType => + !vaultType && 'Vault does not exist'; diff --git a/packages/dai-plugin-mcd/src/schemas/cdpManager.js b/packages/dai-plugin-mcd/src/schemas/cdpManager.js index c00ed7747..87f0454cd 100644 --- a/packages/dai-plugin-mcd/src/schemas/cdpManager.js +++ b/packages/dai-plugin-mcd/src/schemas/cdpManager.js @@ -7,6 +7,7 @@ import { VAULTS_CREATED, VAULT_OWNER } from './_constants'; +import { validateVaultId, validateVaultTypeResult } from './_validators'; export const cdpManagerUrns = { generate: id => ({ @@ -17,13 +18,6 @@ export const cdpManagerUrns = { returns: [VAULT_ADDRESS] }; -const validateVaultId = id => - !/^\d+$/.test(id) && - `Invalid vault id: must be a positive integer. Received ${id}`; - -const validateVaultTypeResult = vaultType => - !vaultType && 'Vault does not exist'; - export const cdpManagerIlks = { generate: id => ({ id: `CDP_MANAGER.ilks(${id})`, diff --git a/packages/dai-plugin-mcd/src/schemas/computed.js b/packages/dai-plugin-mcd/src/schemas/computed.js index 93c66d8ad..69ccab6c3 100644 --- a/packages/dai-plugin-mcd/src/schemas/computed.js +++ b/packages/dai-plugin-mcd/src/schemas/computed.js @@ -54,7 +54,7 @@ import { COLLATERAL_TYPE_COLLATERALIZATION, COLLATERAL_TYPE_DATA } from './_constants'; -import { validateAddress } from './_validators'; +import { validateAddress, validateVaultId } from './_validators'; export const collateralTypePrice = { generate: collateralTypeName => ({ @@ -106,8 +106,7 @@ export const vaultTypeAndAddress = { export const vaultExternalOwner = { generate: id => ({ - dependencies: [[PROXY_OWNER, [VAULT_OWNER, id]]], - // TODO: throw error if no owner (DSProxy contract doesn't exist) + dependencies: [[PROXY_OWNER, [VAULT_OWNER, id]], [VAULT_OWNER, id]], computed: owner => owner }) }; @@ -381,7 +380,10 @@ export const vault = { .toNumber(); } }) - }) + }), + validate: { + args: validateVaultId + } }; export const daiLockedInDsr = { @@ -395,7 +397,7 @@ export const daiLockedInDsr = { } }), validate: { - args: validateAddress`Invalid address: ${'address'}` + args: validateAddress`Invalid address for daiLockedInDsr: ${'address'}` } }; @@ -461,7 +463,7 @@ export const savings = { }) }), validate: { - args: validateAddress`Invalid address: ${'address'}` + args: validateAddress`Invalid address for savings: ${'address'}` } }; @@ -491,7 +493,7 @@ export const userVaultsList = { ) }), validate: { - args: validateAddress`Invalid address: ${'address'}` + args: validateAddress`Invalid address for userVaultsList: ${'address'}` } }; diff --git a/packages/dai-plugin-mcd/src/schemas/getCdps.js b/packages/dai-plugin-mcd/src/schemas/getCdps.js index ca492fce4..008debe8a 100644 --- a/packages/dai-plugin-mcd/src/schemas/getCdps.js +++ b/packages/dai-plugin-mcd/src/schemas/getCdps.js @@ -3,6 +3,7 @@ import { USER_VAULT_ADDRESSES, USER_VAULT_TYPES } from './_constants'; +import { validateAddress } from './_validators'; import { bytesToString } from '../utils'; export const getCdps = { @@ -17,6 +18,10 @@ export const getCdps = { proxyAddress ] }), + validate: { + args: (_, address) => + validateAddress`Invalid address for getCdps: ${'address'}`(address) + }, returns: [ [USER_VAULT_IDS, v => v.map(n => n.toNumber())], [USER_VAULT_ADDRESSES], diff --git a/packages/dai-plugin-mcd/src/schemas/proxyRegistry.js b/packages/dai-plugin-mcd/src/schemas/proxyRegistry.js index 3bbd86412..e8030cc7d 100644 --- a/packages/dai-plugin-mcd/src/schemas/proxyRegistry.js +++ b/packages/dai-plugin-mcd/src/schemas/proxyRegistry.js @@ -1,16 +1,19 @@ import { PROXY_ADDRESS } from './_constants'; -// import { validateAddress, validatProxyAddressResult } from './_validators'; +import { validateAddress } from './_validators'; export const proxyRegistryProxies = { generate: address => ({ id: `PROXY_REGISTRY.proxies(${address})`, contract: 'PROXY_REGISTRY', - call: ['proxies(address)(address)', address] + call: ['proxies(address)(address)', address], + transforms: { + [PROXY_ADDRESS]: v => + v === '0x0000000000000000000000000000000000000000' ? null : v + } }), - // validate: { - // args: validateAddress`Invalid address: ${'address'}`, - // [PROXY_ADDRESS]: validatProxyAddressResult`No proxy found for account address: ${'address'}` - // }, + validate: { + args: validateAddress`Invalid address for proxyAddress: ${'address'}` + }, returns: [[PROXY_ADDRESS]] }; diff --git a/packages/dai-plugin-mcd/src/schemas/token.js b/packages/dai-plugin-mcd/src/schemas/token.js index fb1c1f4a5..2a5ac12fa 100644 --- a/packages/dai-plugin-mcd/src/schemas/token.js +++ b/packages/dai-plugin-mcd/src/schemas/token.js @@ -2,6 +2,7 @@ import { getMcdToken } from '../utils'; import BigNumber from 'bignumber.js'; import { TOKEN_BALANCE, TOKEN_ALLOWANCE_BASE } from './_constants'; +import { validateAddress } from './_validators'; export const ALLOWANCE_AMOUNT = BigNumber( '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff' @@ -76,7 +77,16 @@ export const tokenAllowance = { : [TOKEN_ALLOWANCE_BASE, address, proxyAddress, symbol] ], computed: v => v - }) + }), + validate: { + args: (address, proxyAddress) => + validateAddress`Invalid address for tokenAllowance: ${'address'}`( + address + ) || + validateAddress`Invalid proxy address for tokenAllowance: ${'address'}`( + proxyAddress + ) + } }; export const adapterBalance = { diff --git a/packages/dai-plugin-mcd/test/schemas/cat.spec.js b/packages/dai-plugin-mcd/test/schemas/cat.spec.js index 29f5340d8..7782573c0 100644 --- a/packages/dai-plugin-mcd/test/schemas/cat.spec.js +++ b/packages/dai-plugin-mcd/test/schemas/cat.spec.js @@ -33,9 +33,8 @@ test(LIQUIDATOR_ADDRESS, async () => { const address = await maker.latest(LIQUIDATOR_ADDRESS, 'ETH-A'); expect(address.toLowerCase()).toEqual(expected); - expect(() => { - maker.latest(LIQUIDATOR_ADDRESS, null); - }).toThrow(/invalid/i); + const promise = maker.latest(LIQUIDATOR_ADDRESS, null); + await expect(promise).rejects.toThrow(/invalid/i); }); test(LIQUIDATION_PENALTY, async () => { diff --git a/packages/dai-plugin-mcd/test/schemas/cdpManager.spec.js b/packages/dai-plugin-mcd/test/schemas/cdpManager.spec.js index 10bead233..b0f940974 100644 --- a/packages/dai-plugin-mcd/test/schemas/cdpManager.spec.js +++ b/packages/dai-plugin-mcd/test/schemas/cdpManager.spec.js @@ -73,9 +73,8 @@ test(VAULT_TYPE, async () => { await expect(vaultType2).rejects.toThrow(/does not exist/i); const cdpId3 = -9000; - expect(() => { - maker.latest(VAULT_TYPE, cdpId3); - }).toThrow(/invalid vault id/i); + const vaultType3 = maker.latest(VAULT_TYPE, cdpId3); + await expect(vaultType3).rejects.toThrow(/invalid vault id/i); }); test(VAULTS_CREATED, async () => { diff --git a/packages/dai-plugin-mcd/test/schemas/computed.spec.js b/packages/dai-plugin-mcd/test/schemas/computed.spec.js index 0ca477df0..b18fe9712 100644 --- a/packages/dai-plugin-mcd/test/schemas/computed.spec.js +++ b/packages/dai-plugin-mcd/test/schemas/computed.spec.js @@ -57,7 +57,13 @@ import computedSchemas from '../../src/schemas/computed'; import { createCurrencyRatio } from '@makerdao/currency'; -let maker, snapshotData, address, address2, proxyAddress, expectedVaultAddress; +let maker, + multicall, + snapshotData, + address, + address2, + proxyAddress, + expectedVaultAddress; const ETH_A_COLLATERAL_AMOUNT = ETH(1); const ETH_A_DEBT_AMOUNT = MDAI(1); @@ -78,9 +84,9 @@ beforeAll(async () => { }); address = maker.service('web3').currentAddress(); address2 = TestAccountProvider.nextAccount().address; - - maker.service('multicall').createWatcher(); - maker.service('multicall').registerSchemas({ + multicall = maker.service('multicall'); + multicall.createWatcher(); + multicall.registerSchemas({ vatIlks, vatUrns, vatGem, @@ -101,7 +107,7 @@ beforeAll(async () => { getCdps, ...computedSchemas }); - maker.service('multicall').start(); + multicall.start(); await setupCollateral(maker, 'ETH-A', { price: ETH_A_PRICE @@ -134,6 +140,42 @@ afterAll(async () => { await restoreSnapshot(snapshotData, maker); }); +test(DAI_LOCKED_IN_DSR, async () => { + const daiLockedInDsr = await maker.latest(DAI_LOCKED_IN_DSR, address); + expect(daiLockedInDsr.symbol).toEqual('DSR-DAI'); + expect(daiLockedInDsr.toNumber()).toBeCloseTo(1, 18); +}); + +test(`${DAI_LOCKED_IN_DSR} using invalid account address`, async () => { + const promise = maker.latest(DAI_LOCKED_IN_DSR, '0xfoobar'); + await expect(promise).rejects.toThrow(/invalid/i); +}); + +test(`${DAI_LOCKED_IN_DSR} before and after account has proxy`, async () => { + const nextAccount = TestAccountProvider.nextAccount(); + await maker.addAccount({ ...nextAccount, type: 'privateKey' }); + maker.useAccount(nextAccount.address); + + const promise = maker.latest(DAI_LOCKED_IN_DSR, nextAccount.address); + await expect(promise).rejects.toThrow(/invalid/i); + + await maker.service('proxy').ensureProxy(); + await mineBlocks(maker.service('token'), 1); + + const daiLockedInDsr = await maker.latest( + DAI_LOCKED_IN_DSR, + nextAccount.address + ); + expect(daiLockedInDsr.symbol).toEqual('DSR-DAI'); + expect(daiLockedInDsr.toNumber()).toEqual(0); +}); + +test(TOTAL_DAI_LOCKED_IN_DSR, async () => { + const totalDaiLockedInDsr = await maker.latest(TOTAL_DAI_LOCKED_IN_DSR); + expect(totalDaiLockedInDsr.symbol).toEqual('DSR-DAI'); + expect(totalDaiLockedInDsr.toNumber()).toBeCloseTo(1, 18); +}); + test(COLLATERAL_TYPE_PRICE, async () => { const ethAPrice = await maker.latest(COLLATERAL_TYPE_PRICE, 'ETH-A'); expect(ethAPrice.toNumber()).toEqual(180); @@ -333,32 +375,8 @@ test(`${VAULT} with non-existent id`, async () => { test(`${VAULT} with invalid id`, async () => { const cdpId = -9000; - expect(() => { - maker.latest(VAULT, cdpId); - }).toThrow(/invalid vault id/i); -}); - -test(DAI_LOCKED_IN_DSR, async () => { - const daiLockedInDsr = await maker.latest(DAI_LOCKED_IN_DSR, address); - expect(daiLockedInDsr.symbol).toEqual('DSR-DAI'); - expect(daiLockedInDsr.toNumber()).toBeCloseTo(1, 18); -}); - -test.skip(`${DAI_LOCKED_IN_DSR} using invalid account address`, async () => { - expect(() => { - maker.latest(DAI_LOCKED_IN_DSR, '0xfoobar'); - }).toThrow(/invalid/i); -}); - -test.skip(`${DAI_LOCKED_IN_DSR} using account with no proxy`, async () => { - const promise = maker.latest(DAI_LOCKED_IN_DSR, address2); - await expect(promise).rejects.toThrow(); -}); - -test(TOTAL_DAI_LOCKED_IN_DSR, async () => { - const totalDaiLockedInDsr = await maker.latest(TOTAL_DAI_LOCKED_IN_DSR); - expect(totalDaiLockedInDsr.symbol).toEqual('DSR-DAI'); - expect(totalDaiLockedInDsr.toNumber()).toBeCloseTo(1, 18); + const vault = maker.latest(VAULT, cdpId); + await expect(vault).rejects.toThrow(/invalid vault id/i); }); test(BALANCE, async () => { @@ -423,6 +441,16 @@ test(ALLOWANCE, async () => { maker.useAccount('default'); }); +test(`${ALLOWANCE} using invalid account address`, async () => { + const promise = maker.latest(ALLOWANCE, 'BAT', null); + await expect(promise).rejects.toThrow(/invalid address/i); +}); + +test(`${ALLOWANCE} for account with no proxy`, async () => { + const promise = maker.latest(ALLOWANCE, 'BAT', address2); + await expect(promise).rejects.toThrow(/invalid proxy/i); +}); + test(USER_VAULTS_LIST, async () => { const [batVault, ethVault] = await maker.latest(USER_VAULTS_LIST, address); @@ -440,6 +468,16 @@ test(USER_VAULTS_LIST, async () => { ); }); +test(`${USER_VAULTS_LIST} for account with no proxy`, async () => { + const promise = maker.latest(USER_VAULTS_LIST, address2); + await expect(promise).rejects.toThrow(/invalid/i); +}); + +test(`${USER_VAULTS_LIST} for account with no proxy`, async () => { + const promise = maker.latest(USER_VAULTS_LIST, address2); + await expect(promise).rejects.toThrow(/invalid/i); +}); + test(PROXY_OWNER, async () => { const proxyOwner = await maker.latest(PROXY_OWNER, proxyAddress); expect(isValidAddressString(proxyOwner)).toEqual(true); diff --git a/packages/dai-plugin-mcd/test/schemas/pot.spec.js b/packages/dai-plugin-mcd/test/schemas/pot.spec.js index ba7fa55a2..dd91c054c 100644 --- a/packages/dai-plugin-mcd/test/schemas/pot.spec.js +++ b/packages/dai-plugin-mcd/test/schemas/pot.spec.js @@ -11,7 +11,6 @@ import { ANNUAL_DAI_SAVINGS_RATE, DATE_EARNINGS_LAST_ACCRUED } from '../../src/schemas'; - import potSchemas from '../../src/schemas/pot'; let maker, snapshotData, cdpMgr, saveService; @@ -69,6 +68,17 @@ test(SAVINGS_DAI, async () => { expect(savingsDai.toNumber()).toBeCloseTo(0.99995); }); +test(`${SAVINGS_DAI} using invalid account address`, async () => { + const promise1 = maker.latest(SAVINGS_DAI, '0xfoobar'); + await expect(promise1).rejects.toThrow(/invalid/i); + + const promise2 = maker.latest( + SAVINGS_DAI, + '0x0000000000000000000000000000000000000000' + ); + await expect(promise2).rejects.toThrow(/invalid/i); +}); + test(DAI_SAVINGS_RATE, async () => { const daiSavingsRate = await maker.latest(DAI_SAVINGS_RATE); expect(daiSavingsRate).toEqual(BigNumber('1.000000000315522921573372069')); diff --git a/packages/dai-plugin-mcd/test/schemas/proxyRegistry.spec.js b/packages/dai-plugin-mcd/test/schemas/proxyRegistry.spec.js index f9b1c41f1..e1066be67 100644 --- a/packages/dai-plugin-mcd/test/schemas/proxyRegistry.spec.js +++ b/packages/dai-plugin-mcd/test/schemas/proxyRegistry.spec.js @@ -40,16 +40,15 @@ test(PROXY_ADDRESS, async () => { expect(proxy2).toEqual(proxyAddress2); }); -test.skip(`${PROXY_ADDRESS} using invalid account address`, async () => { - expect(() => { - maker.latest(PROXY_ADDRESS, '0xfoobar'); - }).toThrow(/invalid/i); +test(`${PROXY_ADDRESS} using invalid account address`, async () => { + const promise = maker.latest(PROXY_ADDRESS, '0xfoobar'); + await expect(promise).rejects.toThrow(/invalid/i); }); -test.skip(`${PROXY_ADDRESS} using account with no proxy`, async () => { - const promise = maker.latest( +test(`${PROXY_ADDRESS} using account with no proxy`, async () => { + const proxy = await maker.latest( PROXY_ADDRESS, '0x1111111111111111111111111111111111111111' ); - await expect(promise).rejects.toThrow(/no proxy found/i); + expect(proxy).toBeNull(); }); diff --git a/packages/dai-plugin-mcd/test/schemas/spot.spec.js b/packages/dai-plugin-mcd/test/schemas/spot.spec.js index f38e0b367..459aa7097 100644 --- a/packages/dai-plugin-mcd/test/schemas/spot.spec.js +++ b/packages/dai-plugin-mcd/test/schemas/spot.spec.js @@ -40,12 +40,11 @@ test(PRICE_FEED_ADDRESS, async () => { expect(ethAPriceFeedAddress.toLowerCase()).toEqual(PIP_ETH); expect(batAPriceFeedAddress.toLowerCase()).toEqual(PIP_BAT); - await expect(maker.latest(PRICE_FEED_ADDRESS, 'FOO')).rejects.toThrow( - /no collateral type/i - ); - expect(() => { - maker.latest(PRICE_FEED_ADDRESS, ''); - }).toThrow(/invalid collateral/i); + const promise1 = maker.latest(PRICE_FEED_ADDRESS, 'FOO'); + await expect(promise1).rejects.toThrow(/no collateral type/i); + + const promise2 = maker.latest(PRICE_FEED_ADDRESS, ''); + await expect(promise2).rejects.toThrow(/invalid collateral/i); }); test(LIQUIDATION_RATIO, async () => { @@ -58,12 +57,11 @@ test(LIQUIDATION_RATIO, async () => { expect(ethALiquidationRatio.toNumber()).toEqual(1.5); expect(batALiquidationRatio.toNumber()).toEqual(2.0); - await expect(maker.latest(LIQUIDATION_RATIO, 'FOO')).rejects.toThrow( - /no collateral type/i - ); - expect(() => { - maker.latest(LIQUIDATION_RATIO, ''); - }).toThrow(/invalid collateral/i); + const promise1 = maker.latest(LIQUIDATION_RATIO, 'FOO'); + await expect(promise1).rejects.toThrow(/no collateral type/i); + + const promise2 = maker.latest(LIQUIDATION_RATIO, ''); + await expect(promise2).rejects.toThrow(/invalid collateral/i); }); test(RATIO_DAI_USD, async () => { diff --git a/packages/dai/src/eth/MulticallService.js b/packages/dai/src/eth/MulticallService.js index 112ed419c..512501ad9 100644 --- a/packages/dai/src/eth/MulticallService.js +++ b/packages/dai/src/eth/MulticallService.js @@ -1,14 +1,33 @@ import { PublicService } from '@makerdao/services-core'; import { createWatcher } from '@makerdao/multicall'; import debug from 'debug'; -import { Observable, ReplaySubject, combineLatest, from } from 'rxjs'; -import { map, flatMap, debounceTime, take } from 'rxjs/operators'; +import { Observable, ReplaySubject, combineLatest, from, throwError, timer } from 'rxjs'; +import { + map, + flatMap, + debounceTime, + take, + catchError, + filter, + takeUntil, + throwIfEmpty, + tap +} from 'rxjs/operators'; import get from 'lodash/get'; import set from 'lodash/set'; +import find from 'lodash/find'; const log = debug('dai:MulticallService'); const log2 = debug('dai:MulticallService:observables'); +const throwIfErrorInValues = values => values.map(v => { if (v instanceof Error) throw v; }); // prettier-ignore +const checkForErrors = values => find(values, v => v instanceof Error) === undefined; +const catchNestedErrors = key => f => + catchError(err => { + log2(`Caught nested error in ${key}: ${err}`); + return from([new Error(err)]); + })(f); + export default class MulticallService extends PublicService { constructor(name = 'multicall') { super(name, ['web3', 'smartContract']); @@ -32,6 +51,7 @@ export default class MulticallService extends PublicService { this._removeSchemaDelay = settings.removeSchemaDelay || 1000; this._debounceTime = settings.debounceTime || 1; this._latestDebounceTime = settings.latestDebounceTime || 1; + this._latestTimeout = settings.latestTimeout || 10000; } authenticate() { @@ -164,8 +184,14 @@ export default class MulticallService extends PublicService { } latest(key, ...args) { - return this.watch(key, ...args) + const obsPath = `${key}${args.length > 0 ? '.' : ''}${args.join('.')}`; + return this._watch({ depth: 0, throwIfError: true }, key, ...args) .pipe( + catchError(err => { + throw new Error(err); + }), + takeUntil(timer(this._latestTimeout)), + throwIfEmpty(() => new Error(`Timed out waiting for latest value of: ${obsPath}`)), debounceTime(this._latestDebounceTime), take(1) ) @@ -176,22 +202,26 @@ export default class MulticallService extends PublicService { return this._watch({ depth: 0 }, key, ...args); } - _watch({ depth }, key, ...args) { + _watch({ depth, throwIfError = false }, key, ...args) { // Find schema definition associated with this observable key const schemaDefinition = this.schemaByObservableKey(key); const expectedArgs = schemaDefinition.generate.length; if (args.length < expectedArgs) - throw new Error(`Observable ${key} expects at least ${expectedArgs} argument(s)`); + return throwError(`Observable ${key} expects at least ${expectedArgs} argument(s)`); + + const obsPath = `${key}${args.length > 0 ? '.' : ''}${args.join('.')}`; // Validate arguments using schema args validator if (schemaDefinition?.validate?.args) { const validate = schemaDefinition.validate.args(...args); - if (validate) throw new Error(validate); + if (validate) { + log2(`Input validation failed for observable: ${obsPath} (depth: ${depth})`); + return throwError(validate); + } } // Create or get existing schema instance for this instance path (schema definition + args) const schemaInstance = this._createSchemaInstance(schemaDefinition, ...args); - const obsPath = `${key}${args.length > 0 ? '.' : ''}${args.join('.')}`; const { computed } = schemaInstance; log2(`watch() called for ${computed ? 'computed ' : 'base '}observable: ${obsPath} (depth: ${depth})`); // prettier-ignore @@ -202,7 +232,13 @@ export default class MulticallService extends PublicService { log2(`Returning existing computed observable: ${obsPath} (depth: ${depth})`); // Only debounce if call to watch() is not nested if (depth === 0) existing = existing.pipe(debounceTime(this._debounceTime)); - return existing.pipe(map(result => computed(...result))); + if (throwIfError) existing = existing.pipe(tap(throwIfErrorInValues)); + return existing.pipe( + // Don't pass values to computed() if any of them are errors + filter(checkForErrors), + // Pass values to computed() on the computed observable + map(result => computed(...result)) + ); } log2(`Returning existing base observable: ${obsPath}`); return existing; @@ -251,7 +287,11 @@ export default class MulticallService extends PublicService { trie.map((node, idx) => { return indexesAtLeafNodes[idx] ? [node] : recurseDependencyTree(node); }) - ).pipe(flatMap(result => this._watch({ depth: depth + 1 }, key, ...result))); + ).pipe( + flatMap(result => + this._watch({ depth: depth + 1 }, key, ...result).pipe(catchNestedErrors(key)) + ) + ); } }; @@ -262,7 +302,13 @@ export default class MulticallService extends PublicService { set(this._observables, obsPath, observable); // Only debounce if call to watch() is not nested if (depth === 0) observable = observable.pipe(debounceTime(this._debounceTime)); - return observable.pipe(map(result => computed(...result))); + if (throwIfError) observable = observable.pipe(tap(throwIfErrorInValues)); + return observable.pipe( + // Don't pass values to computed() if any of them are errors + filter(checkForErrors), + // Pass values to computed() on the computed observable + map(result => computed(...result)) + ); } // This is a base observable diff --git a/packages/dai/test/eth/MulticallService.spec.js b/packages/dai/test/eth/MulticallService.spec.js index 27c4bd5c8..63d17204e 100644 --- a/packages/dai/test/eth/MulticallService.spec.js +++ b/packages/dai/test/eth/MulticallService.spec.js @@ -109,10 +109,9 @@ test('computed observable with nested dependencies', async () => { expect(multicall.totalActiveSchemas).toEqual(3); }); -test('observable throws args validation error', () => { - expect(() => { - maker.latest(CDP_COLLATERAL, -9000); - }).toThrow(/invalid cdp id/i); +test('observable throws args validation error', async () => { + const promise = maker.latest(CDP_COLLATERAL, -9000); + await expect(promise).rejects.toThrow(/invalid cdp id/i); }); test('observable throws invalid key error', () => { @@ -121,18 +120,23 @@ test('observable throws invalid key error', () => { }).toThrow(/invalid observable key/i); }); -test('observable throws insufficient args error', () => { +test('observable throws no registered schema error', () => { expect(() => { - maker.latest(CDP_OWNER); - }).toThrow(/expects.*argument/i); + maker.latest('foo'); + }).toThrow(/no registered schema/i); +}); + +test('observable throws insufficient args error', async () => { + const promise = maker.latest(CDP_OWNER); + await expect(promise).rejects.toThrow(/expects.*argument/i); }); test('observable throws result validation error', async () => { - const cdpCollateral = maker.latest(CDP_COLLATERAL, cdpId2); - await expect(cdpCollateral).rejects.toThrow(/Φ/); + const promise = maker.latest(CDP_COLLATERAL, cdpId2); + await expect(promise).rejects.toThrow(/Φ/); }); test('observable throws result validation error 2', async () => { - const cdpOwner = maker.latest(CDP_OWNER, 9000); - await expect(cdpOwner).rejects.toThrow(); + const promise = maker.latest(CDP_OWNER, 9000); + await expect(promise).rejects.toThrow(); });