Merge pull request #1082 from 0xProject/fixGetBlocks

Fix block fetch error if block not found

8 files changed, 91 insertions, 31 deletions

diff --git a/packages/contract-wrappers/src/contract_wrappers/contract_wrapper.ts b/packages/contract-wrappers/src/contract_wrappers/contract_wrapper.ts
index 19de17c0a..19a882712 100644
--- a/packages/contract-wrappers/src/contract_wrappers/contract_wrapper.ts
+++ b/packages/contract-wrappers/src/contract_wrappers/contract_wrapper.ts
@@ -2,6 +2,7 @@ import { AbiDecoder, intervalUtils, logUtils } from '@0xproject/utils';
 import { Web3Wrapper } from '@0xproject/web3-wrapper';
 import {
     BlockParamLiteral,
+    BlockWithoutTransactionData,
     ContractAbi,
     ContractArtifact,
     FilterObject,
@@ -174,7 +175,7 @@ export abstract class ContractWrapper {
             throw new Error(ContractWrappersError.SubscriptionAlreadyPresent);
         }
         this._blockAndLogStreamerIfExists = new BlockAndLogStreamer(
-            this._web3Wrapper.getBlockAsync.bind(this._web3Wrapper),
+            this._getBlockOrNullAsync.bind(this),
             this._web3Wrapper.getLogsAsync.bind(this._web3Wrapper),
             ContractWrapper._onBlockAndLogStreamerError.bind(this, isVerbose),
         );
@@ -194,6 +195,14 @@ export abstract class ContractWrapper {
             this._onLogStateChanged.bind(this, isRemoved),
         );
     }
+    // This method only exists in order to comply with the expected interface of Blockstream's constructor
+    private async _getBlockOrNullAsync(): Promise<BlockWithoutTransactionData | null> {
+        const blockIfExists = await this._web3Wrapper.getBlockIfExistsAsync.bind(this._web3Wrapper);
+        if (_.isUndefined(blockIfExists)) {
+            return null;
+        }
+        return blockIfExists;
+    }
     // HACK: This should be a package-scoped method (which doesn't exist in TS)
     // We don't want this method available in the public interface for all classes
     // who inherit from ContractWrapper, and it is only used by the internal implementation
@@ -212,11 +221,14 @@ export abstract class ContractWrapper {
         delete this._blockAndLogStreamerIfExists;
     }
     private async _reconcileBlockAsync(): Promise<void> {
-        const latestBlock = await this._web3Wrapper.getBlockAsync(BlockParamLiteral.Latest);
+        const latestBlockIfExists = await this._web3Wrapper.getBlockIfExistsAsync(BlockParamLiteral.Latest);
+        if (_.isUndefined(latestBlockIfExists)) {
+            return; // noop
+        }
         // We need to coerce to Block type cause Web3.Block includes types for mempool blocks
         if (!_.isUndefined(this._blockAndLogStreamerIfExists)) {
             // If we clear the interval while fetching the block - this._blockAndLogStreamer will be undefined
-            await this._blockAndLogStreamerIfExists.reconcileNewBlock((latestBlock as any) as Block);
+            await this._blockAndLogStreamerIfExists.reconcileNewBlock((latestBlockIfExists as any) as Block);
         }
     }
 }
diff --git a/packages/contract-wrappers/test/subscription_test.ts b/packages/contract-wrappers/test/subscription_test.ts
index 81b9012bd..68ef7225e 100644
--- a/packages/contract-wrappers/test/subscription_test.ts
+++ b/packages/contract-wrappers/test/subscription_test.ts
@@ -61,7 +61,7 @@ describe('SubscriptionTest', () => {
                     callback,
                 );
                 stubs = [
-                    Sinon.stub((contractWrappers as any)._web3Wrapper, 'getBlockAsync').throws(
+                    Sinon.stub((contractWrappers as any)._web3Wrapper, 'getBlockIfExistsAsync').throws(
                         new Error('JSON RPC error'),
                     ),
                 ];
diff --git a/packages/contracts/test/multisig/multi_sig_with_time_lock.ts b/packages/contracts/test/multisig/multi_sig_with_time_lock.ts
index 05d8bbb36..0b17c298b 100644
--- a/packages/contracts/test/multisig/multi_sig_with_time_lock.ts
+++ b/packages/contracts/test/multisig/multi_sig_with_time_lock.ts
@@ -269,7 +269,10 @@ describe('MultiSigWalletWithTimeLock', () => {
                 expect(confirmRes.logs).to.have.length(2);
 
                 const blockNum = await web3Wrapper.getBlockNumberAsync();
-                const blockInfo = await web3Wrapper.getBlockAsync(blockNum);
+                const blockInfo = await web3Wrapper.getBlockIfExistsAsync(blockNum);
+                if (_.isUndefined(blockInfo)) {
+                    throw new Error(`Unexpectedly failed to fetch block at #${blockNum}`);
+                }
                 const timestamp = new BigNumber(blockInfo.timestamp);
                 const confirmationTimeBigNum = new BigNumber(await multiSig.confirmationTimes.callAsync(txId));
 
diff --git a/packages/contracts/test/utils/block_timestamp.ts b/packages/contracts/test/utils/block_timestamp.ts
index 1159792c4..66c13eed1 100644
--- a/packages/contracts/test/utils/block_timestamp.ts
+++ b/packages/contracts/test/utils/block_timestamp.ts
@@ -35,6 +35,9 @@ export async function increaseTimeAndMineBlockAsync(seconds: number): Promise<nu
  * @returns a new Promise which will resolve with the timestamp in seconds.
  */
 export async function getLatestBlockTimestampAsync(): Promise<number> {
-    const currentBlock = await web3Wrapper.getBlockAsync('latest');
-    return currentBlock.timestamp;
+    const currentBlockIfExists = await web3Wrapper.getBlockIfExistsAsync('latest');
+    if (_.isUndefined(currentBlockIfExists)) {
+        throw new Error(`Unable to fetch latest block.`);
+    }
+    return currentBlockIfExists.timestamp;
 }
diff --git a/packages/order-watcher/src/order_watcher/event_watcher.ts b/packages/order-watcher/src/order_watcher/event_watcher.ts
index 9509c75de..eca235e26 100644
--- a/packages/order-watcher/src/order_watcher/event_watcher.ts
+++ b/packages/order-watcher/src/order_watcher/event_watcher.ts
@@ -1,6 +1,6 @@
 import { intervalUtils, logUtils } from '@0xproject/utils';
 import { Web3Wrapper } from '@0xproject/web3-wrapper';
-import { BlockParamLiteral, LogEntry, Provider } from 'ethereum-types';
+import { BlockParamLiteral, BlockWithoutTransactionData, LogEntry, Provider } from 'ethereum-types';
 import { Block, BlockAndLogStreamer, Log } from 'ethereumjs-blockstream';
 import * as _ from 'lodash';
 
@@ -62,7 +62,7 @@ export class EventWatcher {
             throw new Error(OrderWatcherError.SubscriptionAlreadyPresent);
         }
         this._blockAndLogStreamerIfExists = new BlockAndLogStreamer(
-            this._web3Wrapper.getBlockAsync.bind(this._web3Wrapper),
+            this._getBlockOrNullAsync.bind(this),
             this._web3Wrapper.getLogsAsync.bind(this._web3Wrapper),
             this._onBlockAndLogStreamerError.bind(this),
         );
@@ -82,6 +82,14 @@ export class EventWatcher {
             this._onLogStateChangedAsync.bind(this, callback, isRemoved),
         );
     }
+    // This method only exists in order to comply with the expected interface of Blockstream's constructor
+    private async _getBlockOrNullAsync(): Promise<BlockWithoutTransactionData | null> {
+        const blockIfExists = await this._web3Wrapper.getBlockIfExistsAsync.bind(this._web3Wrapper);
+        if (_.isUndefined(blockIfExists)) {
+            return null;
+        }
+        return blockIfExists;
+    }
     private _stopBlockAndLogStream(): void {
         if (_.isUndefined(this._blockAndLogStreamerIfExists)) {
             throw new Error(OrderWatcherError.SubscriptionNotFound);
@@ -100,11 +108,14 @@ export class EventWatcher {
         await this._emitDifferencesAsync(log, isRemoved ? LogEventState.Removed : LogEventState.Added, callback);
     }
     private async _reconcileBlockAsync(): Promise<void> {
-        const latestBlock = await this._web3Wrapper.getBlockAsync(this._stateLayer);
+        const latestBlockIfExists = await this._web3Wrapper.getBlockIfExistsAsync(this._stateLayer);
+        if (_.isUndefined(latestBlockIfExists)) {
+            return; // noop
+        }
         // We need to coerce to Block type cause Web3.Block includes types for mempool blocks
         if (!_.isUndefined(this._blockAndLogStreamerIfExists)) {
             // If we clear the interval while fetching the block - this._blockAndLogStreamer will be undefined
-            await this._blockAndLogStreamerIfExists.reconcileNewBlock((latestBlock as any) as Block);
+            await this._blockAndLogStreamerIfExists.reconcileNewBlock((latestBlockIfExists as any) as Block);
         }
     }
     private async _emitDifferencesAsync(
diff --git a/packages/web3-wrapper/CHANGELOG.json b/packages/web3-wrapper/CHANGELOG.json
index 1653f388c..ce3f5e3be 100644
--- a/packages/web3-wrapper/CHANGELOG.json
+++ b/packages/web3-wrapper/CHANGELOG.json
@@ -1,5 +1,15 @@
 [
     {
+        "version": "3.0.0",
+        "changes": [
+            {
+                "note":
+                    "Rename `getBlockAsync` to `getBlockIfExistsAsync` and rather then throw if the requested block wasn't found, return undefined.",
+                "pr": 1082
+            }
+        ]
+    },
+    {
         "version": "2.0.3",
         "changes": [
             {
diff --git a/packages/web3-wrapper/src/web3_wrapper.ts b/packages/web3-wrapper/src/web3_wrapper.ts
index af0fe3a72..dc634a57f 100644
--- a/packages/web3-wrapper/src/web3_wrapper.ts
+++ b/packages/web3-wrapper/src/web3_wrapper.ts
@@ -329,23 +329,29 @@ export class Web3Wrapper {
     /**
      * Fetch a specific Ethereum block without transaction data
      * @param blockParam The block you wish to fetch (blockHash, blockNumber or blockLiteral)
-     * @returns The requested block without transaction data
+     * @returns The requested block without transaction data, or undefined if block was not found
+     * (e.g the node isn't fully synced, there was a block re-org and the requested block was uncles, etc...)
      */
-    public async getBlockAsync(blockParam: string | BlockParam): Promise<BlockWithoutTransactionData> {
+    public async getBlockIfExistsAsync(
+        blockParam: string | BlockParam,
+    ): Promise<BlockWithoutTransactionData | undefined> {
         Web3Wrapper._assertBlockParamOrString(blockParam);
         const encodedBlockParam = marshaller.marshalBlockParam(blockParam);
         const method = utils.isHexStrict(blockParam) ? 'eth_getBlockByHash' : 'eth_getBlockByNumber';
         const shouldIncludeTransactionData = false;
-        const blockWithoutTransactionDataWithHexValues = await this._sendRawPayloadAsync<
+        const blockWithoutTransactionDataWithHexValuesOrNull = await this._sendRawPayloadAsync<
             BlockWithoutTransactionDataRPC
         >({
             method,
             params: [encodedBlockParam, shouldIncludeTransactionData],
         });
-        const blockWithoutTransactionData = marshaller.unmarshalIntoBlockWithoutTransactionData(
-            blockWithoutTransactionDataWithHexValues,
-        );
-        return blockWithoutTransactionData;
+        let blockWithoutTransactionDataIfExists;
+        if (!_.isNull(blockWithoutTransactionDataWithHexValuesOrNull)) {
+            blockWithoutTransactionDataIfExists = marshaller.unmarshalIntoBlockWithoutTransactionData(
+                blockWithoutTransactionDataWithHexValuesOrNull,
+            );
+        }
+        return blockWithoutTransactionDataIfExists;
     }
     /**
      * Fetch a specific Ethereum block with transaction data
@@ -376,8 +382,11 @@ export class Web3Wrapper {
      */
     public async getBlockTimestampAsync(blockParam: string | BlockParam): Promise<number> {
         Web3Wrapper._assertBlockParamOrString(blockParam);
-        const { timestamp } = await this.getBlockAsync(blockParam);
-        return timestamp;
+        const blockIfExists = await this.getBlockIfExistsAsync(blockParam);
+        if (_.isUndefined(blockIfExists)) {
+            throw new Error(`Failed to fetch block with blockParam: ${JSON.stringify(blockParam)}`);
+        }
+        return blockIfExists.timestamp;
     }
     /**
      * Retrieve the user addresses available through the backing provider
diff --git a/packages/web3-wrapper/test/web3_wrapper_test.ts b/packages/web3-wrapper/test/web3_wrapper_test.ts
index b4fd8bb44..385c469bf 100644
--- a/packages/web3-wrapper/test/web3_wrapper_test.ts
+++ b/packages/web3-wrapper/test/web3_wrapper_test.ts
@@ -85,28 +85,40 @@ describe('Web3Wrapper tests', () => {
             expect(typeof blockNumber).to.be.equal('number');
         });
     });
-    describe('#getBlockAsync', () => {
+    describe('#getBlockIfExistsAsync', () => {
         it('gets block when supplied a valid BlockParamLiteral value', async () => {
             const blockParamLiteral = BlockParamLiteral.Earliest;
-            const block = await web3Wrapper.getBlockAsync(blockParamLiteral);
-            expect(block.number).to.be.equal(0);
-            expect(utils.isBigNumber(block.difficulty)).to.equal(true);
-            expect(_.isNumber(block.gasLimit)).to.equal(true);
+            const blockIfExists = await web3Wrapper.getBlockIfExistsAsync(blockParamLiteral);
+            if (_.isUndefined(blockIfExists)) {
+                throw new Error('Expected block to exist');
+            }
+            expect(blockIfExists.number).to.be.equal(0);
+            expect(utils.isBigNumber(blockIfExists.difficulty)).to.equal(true);
+            expect(_.isNumber(blockIfExists.gasLimit)).to.equal(true);
         });
         it('gets block when supplied a block number', async () => {
             const blockParamLiteral = 0;
-            const block = await web3Wrapper.getBlockAsync(blockParamLiteral);
-            expect(block.number).to.be.equal(0);
+            const blockIfExists = await web3Wrapper.getBlockIfExistsAsync(blockParamLiteral);
+            if (_.isUndefined(blockIfExists)) {
+                throw new Error('Expected block to exist');
+            }
+            expect(blockIfExists.number).to.be.equal(0);
         });
         it('gets block when supplied a block hash', async () => {
             const blockParamLiteral = 0;
-            const block = await web3Wrapper.getBlockAsync(blockParamLiteral);
-            const sameBlock = await web3Wrapper.getBlockAsync(block.hash as string);
-            expect(sameBlock.number).to.be.equal(0);
+            const blockIfExists = await web3Wrapper.getBlockIfExistsAsync(blockParamLiteral);
+            if (_.isUndefined(blockIfExists)) {
+                throw new Error('Expected block to exist');
+            }
+            const sameBlockIfExists = await web3Wrapper.getBlockIfExistsAsync(blockIfExists.hash as string);
+            if (_.isUndefined(sameBlockIfExists)) {
+                throw new Error('Expected block to exist');
+            }
+            expect(sameBlockIfExists.number).to.be.equal(0);
         });
         it('should throw if supplied invalid blockParam value', async () => {
             const invalidBlockParam = 'deadbeef';
-            expect(web3Wrapper.getBlockAsync(invalidBlockParam)).to.eventually.to.be.rejected();
+            expect(web3Wrapper.getBlockIfExistsAsync(invalidBlockParam)).to.eventually.to.be.rejected();
         });
     });
     describe('#getBlockWithTransactionDataAsync', () => {