Rework web3 timeouts (#1393)

* Remove unused sendDataTransaction and related methods * Remove syncTimeout property from contracts * Set web3 transactionPollingTimeout with value from timeout option * Add new blockTimeout option used in websocket connections * Lint * Apply suggestions from code review Co-Authored-By: Nicolás Venturo <[email protected]> Co-authored-by: Nicolás Venturo <[email protected]>
OpenZeppelin · Jan 20, 2020 · 4efd4cc · 4efd4cc
1 parent e8a5fb4
commit 4efd4cc
Show file tree

Hide file tree

Showing 16 changed files with 76 additions and 361 deletions.
diff --git a/packages/cli/src/bin/options.ts b/packages/cli/src/bin/options.ts
@@ -1,10 +1,13 @@
 import program, { Command } from 'commander';
-import { DEFAULT_TX_TIMEOUT } from '../models/network/Session';
+import { DEFAULT_TX_TIMEOUT, DEFAULT_TX_BLOCK_TIMEOUT } from '../models/network/Session';
 
 program.Command.prototype.withNetworkTimeoutOption = function(): Command {
   return this.option(
     '--timeout <timeout>',
-    `timeout in seconds for each blockchain transaction (defaults to ${DEFAULT_TX_TIMEOUT}s)`,
+    `timeout in seconds for each transaction when using an http connection (defaults to ${DEFAULT_TX_TIMEOUT} seconds)`,
+  ).option(
+    '--blockTimeout <timeout>',
+    `timeout in blocks for each transaction when using a websocket connection (defaults to ${DEFAULT_TX_BLOCK_TIMEOUT} blocks)`,
   );
 };
 

diff --git a/packages/cli/src/commands/session.ts b/packages/cli/src/commands/session.ts
@@ -3,6 +3,7 @@ import session from '../scripts/session';
 import { promptIfNeeded, networksList, InquirerQuestions } from '../prompts/prompt';
 import ConfigManager from '../models/config/ConfigManager';
 import Telemetry from '../telemetry';
+import { DEFAULT_TX_TIMEOUT, DEFAULT_TX_BLOCK_TIMEOUT } from '../models/network/Session';
 
 const name = 'session';
 const signature: string = name;
@@ -21,7 +22,7 @@ const register: (program: any) => any = program =>
     .action(action);
 
 async function action(options: any): Promise<void> {
-  const { network: networkInOpts, expires, timeout, from, close, interactive } = options;
+  const { network: networkInOpts, expires, timeout, blockTimeout, from, close, interactive } = options;
 
   if (close) {
     await Telemetry.report('session', { close }, options.interactive);
@@ -34,7 +35,7 @@ async function action(options: any): Promise<void> {
     const { network } = await ConfigManager.initNetworkConfiguration(promptedNetwork, true);
     const accounts = await ZWeb3.eth.getAccounts();
     const promptedSession = await promptIfNeeded(
-      { opts: { timeout, from, expires }, props: getCommandProps(accounts) },
+      { opts: { timeout, blockTimeout, from, expires }, props: getCommandProps(accounts) },
       interactive,
     );
 
@@ -58,8 +59,13 @@ function getCommandProps(accounts: string[] = []): InquirerQuestions {
     },
     timeout: {
       type: 'input',
-      message: 'Enter a timeout to use for all web3 transactions (in seconds)',
-      default: 3600,
+      message: 'Enter a timeout in seconds to use for http-based web3 transactions',
+      default: DEFAULT_TX_TIMEOUT,
+    },
+    blockTimeout: {
+      type: 'input',
+      message: 'Enter a timeout in blocks to use for websocket-based web3 transactions',
+      default: DEFAULT_TX_BLOCK_TIMEOUT,
     },
     expires: {
       type: 'input',

diff --git a/packages/cli/src/models/config/ConfigManager.ts b/packages/cli/src/models/config/ConfigManager.ts
@@ -28,17 +28,16 @@ const ConfigManager = {
     root: string = process.cwd(),
   ): Promise<{ network: string; txParams: TxParams } | never> {
     this.initStaticConfiguration(root);
-    const { network: networkName, from, timeout } = Session.getOptions(options, silent);
+    const { network: networkName, from, timeout, blockTimeout } = Session.getOptions(options, silent);
     Session.setDefaultNetworkIfNeeded(options.network);
     if (!networkName) throw Error('A network name must be provided to execute the requested action.');
 
     const { provider, artifactDefaults, network } = await this.config.loadNetworkConfig(networkName, root);
 
-    Contracts.setSyncTimeout(timeout * 1000);
     Contracts.setArtifactsDefaults(artifactDefaults);
 
     try {
-      ZWeb3.initialize(provider);
+      ZWeb3.initialize(provider, { pollingTimeout: timeout, blockTimeout });
       await ZWeb3.checkNetworkId(network.networkId);
       const txParams = {
         from: ZWeb3.toChecksumAddress(from || artifactDefaults.from || (await ZWeb3.defaultAccount())),

diff --git a/packages/cli/src/models/network/Session.ts b/packages/cli/src/models/network/Session.ts
@@ -11,13 +11,15 @@ import { OPEN_ZEPPELIN_FOLDER } from '../files/constants';
 const state = { alreadyPrintedSessionInfo: false };
 const SESSION_FILE = '.session';
 const SESSION_PATH = path.join(OPEN_ZEPPELIN_FOLDER, SESSION_FILE);
-const DEFAULT_TX_TIMEOUT: number = 10 * 60; // 10 minutes
+const DEFAULT_TX_TIMEOUT = 750; // same as web3
+const DEFAULT_TX_BLOCK_TIMEOUT = 50; // same as web3
 const DEFAULT_EXPIRATION_TIMEOUT: number = 15 * 60; // 15 minutes
 
 interface SessionOptions {
   network?: string;
   from?: string;
   timeout?: number;
+  blockTimeout?: number;
   expires?: Date;
 }
 
@@ -45,14 +47,19 @@ const Session = {
     return { network, expired: this._hasExpired(session) };
   },
 
-  open({ network, from, timeout }: SessionOptions, expires: number = DEFAULT_EXPIRATION_TIMEOUT, logInfo = true): void {
+  open(
+    { network, from, timeout, blockTimeout }: SessionOptions,
+    expires: number = DEFAULT_EXPIRATION_TIMEOUT,
+    logInfo = true,
+  ): void {
     const expirationTimestamp = new Date(new Date().getTime() + expires * 1000);
     fs.writeJsonSync(
       SESSION_PATH,
       {
         network,
         from,
         timeout,
+        blockTimeout,
         expires: expirationTimestamp,
       },
       { spaces: 2 },
@@ -62,7 +69,7 @@ const Session = {
         __filename,
         'getOptions',
         `get-options`,
-        `Using ${describe({ network, from, timeout })} by default.`,
+        `Using ${describe({ network, from, timeout, blockTimeout })} by default.`,
       );
     }
   },
@@ -88,12 +95,13 @@ const Session = {
   _parseSession(): SessionOptions | undefined {
     const session = fs.existsSync(SESSION_PATH) ? fs.readJsonSync(SESSION_PATH) : null;
     if (isEmpty(session)) return undefined;
-    const parsedSession = pick(session, 'network', 'timeout', 'from', 'expires');
+    const parsedSession = pick(session, 'network', 'timeout', 'blockTimeout', 'from', 'expires');
     return this._setDefaults(parsedSession);
   },
 
   _setDefaults(session: SessionOptions): SessionOptions {
     if (!session.timeout) session.timeout = DEFAULT_TX_TIMEOUT;
+    if (!session.blockTimeout) session.blockTimeout = DEFAULT_TX_BLOCK_TIMEOUT;
     return session;
   },
 
@@ -108,9 +116,10 @@ function describe(session: SessionOptions): string {
       session.network && `network ${session.network}`,
       session.from && `sender address ${session.from}`,
       session.timeout && `timeout ${session.timeout} seconds`,
+      session.blockTimeout && `blockTimeout ${session.blockTimeout} blocks`,
     ]).join(', ') || 'no options'
   );
 }
 
-export { DEFAULT_TX_TIMEOUT };
+export { DEFAULT_TX_TIMEOUT, DEFAULT_TX_BLOCK_TIMEOUT };
 export default Session;
diff --git a/packages/cli/src/scripts/interfaces.ts b/packages/cli/src/scripts/interfaces.ts
@@ -136,6 +136,7 @@ export interface SessionParams {
   close?: boolean;
   network?: string;
   timeout?: number;
+  blockTimeout?: number;
   expires?: number;
 }
 

diff --git a/packages/cli/src/scripts/session.ts b/packages/cli/src/scripts/session.ts
@@ -1,10 +1,17 @@
 import Session from '../models/network/Session';
 import { SessionParams } from './interfaces';
 
-export default function session({ network, from, timeout, close = false, expires }: SessionParams): void | never {
-  const anyNetworkOption = network || from || timeout;
+export default function session({
+  network,
+  from,
+  timeout,
+  blockTimeout,
+  close = false,
+  expires,
+}: SessionParams): void | never {
+  const anyNetworkOption = network || from || timeout || blockTimeout;
   if (!!anyNetworkOption === !!close) {
-    throw Error('Please provide either a network option (--network, --timeout, --from) or --close.');
+    throw Error('Please provide either a network option (--network, --timeout, --blockTimeout, --from) or --close.');
   }
-  close ? Session.close() : Session.open({ network, from, timeout }, expires);
+  close ? Session.close() : Session.open({ network, from, timeout, blockTimeout }, expires);
 }
diff --git a/packages/cli/test/commands/session.test.js b/packages/cli/test/commands/session.test.js
@@ -17,6 +17,7 @@ describe('session command', function() {
         from: undefined,
         network: 'test',
         timeout: undefined,
+        blockTimeout: undefined,
       });
     },
   );

diff --git a/packages/cli/test/config/ConfigManager.test.js b/packages/cli/test/config/ConfigManager.test.js
@@ -91,6 +91,15 @@ describe('ConfigManager', function() {
       this.initialize.should.have.been.calledWith('http://localhost:8545');
     });
 
+    it('initializes web3 instance with timeout', async function() {
+      await ConfigManager.initNetworkConfiguration(
+        { network: 'local', timeout: 100, blockTimeout: 5 },
+        true,
+        configFileDir,
+      );
+      this.initialize.should.have.been.calledWith('http://localhost:8545', { pollingTimeout: 100, blockTimeout: 5 });
+    });
+
     describe('#setBaseConfig', function() {
       it('sets the correct config', function() {
         ConfigManager.setBaseConfig(configFileDir);

diff --git a/packages/cli/test/scripts/session.test.js b/packages/cli/test/scripts/session.test.js
@@ -4,12 +4,12 @@ require('../setup');
 import { expect } from 'chai';
 
 import session from '../../src/scripts/session';
-import Session, { DEFAULT_TX_TIMEOUT } from '../../src/models/network/Session';
+import Session, { DEFAULT_TX_TIMEOUT, DEFAULT_TX_BLOCK_TIMEOUT } from '../../src/models/network/Session';
 
 describe('session script', function() {
   afterEach(() => Session.close());
 
-  const opts = { network: 'foo', from: '0x1', timeout: 10 };
+  const opts = { network: 'foo', from: '0x1', timeout: 10, blockTimeout: 2 };
 
   describe('opening a new session', function() {
     context('when there was no session opened before', function() {
@@ -26,6 +26,7 @@ describe('session script', function() {
           Session.getOptions({ from: '0x2' }).should.include({
             network: 'foo',
             timeout: 10,
+            blockTimeout: 2,
             from: '0x2',
           });
         });
@@ -36,13 +37,15 @@ describe('session script', function() {
           session({ ...opts, expires: 0 });
           Session.getOptions().should.be.deep.equal({
             timeout: DEFAULT_TX_TIMEOUT,
+            blockTimeout: DEFAULT_TX_BLOCK_TIMEOUT,
           });
         });
 
         it('returns given options', function() {
           Session.getOptions({ from: '0x2' }).should.be.deep.equal({
             from: '0x2',
             timeout: DEFAULT_TX_TIMEOUT,
+            blockTimeout: DEFAULT_TX_BLOCK_TIMEOUT,
           });
         });
       });
@@ -57,6 +60,7 @@ describe('session script', function() {
           Session.getOptions().should.include({
             network: 'bar',
             timeout: DEFAULT_TX_TIMEOUT,
+            blockTimeout: DEFAULT_TX_BLOCK_TIMEOUT,
           });
         });
       });
@@ -66,6 +70,7 @@ describe('session script', function() {
           session({ network: 'bar', expires: 0 });
           Session.getOptions().should.be.deep.equal({
             timeout: DEFAULT_TX_TIMEOUT,
+            blockTimeout: DEFAULT_TX_BLOCK_TIMEOUT,
           });
         });
       });
@@ -78,6 +83,7 @@ describe('session script', function() {
         session({ close: true });
         Session.getOptions().should.be.deep.equal({
           timeout: DEFAULT_TX_TIMEOUT,
+          blockTimeout: DEFAULT_TX_BLOCK_TIMEOUT,
         });
       });
     });
@@ -89,6 +95,7 @@ describe('session script', function() {
         session({ close: true });
         Session.getOptions().should.be.deep.equal({
           timeout: DEFAULT_TX_TIMEOUT,
+          blockTimeout: DEFAULT_TX_BLOCK_TIMEOUT,
         });
       });
     });
@@ -98,15 +105,15 @@ describe('session script', function() {
     context('when no arguments are given', function() {
       it('throws an error', function() {
         expect(() => session({})).to.throw(
-          'Please provide either a network option (--network, --timeout, --from) or --close.',
+          'Please provide either a network option (--network, --timeout, --blockTimeout, --from) or --close.',
         );
       });
     });
 
     context('when both arguments are given', function() {
       it('throws an error', function() {
         expect(() => session({ network: 'boo', close: true })).to.throw(
-          'Please provide either a network option (--network, --timeout, --from) or --close.',
+          'Please provide either a network option (--network, --timeout, --blockTimeout, --from) or --close.',
         );
       });
     });

diff --git a/packages/lib/src/artifacts/Contracts.ts b/packages/lib/src/artifacts/Contracts.ts
@@ -5,21 +5,15 @@ import ZWeb3 from './ZWeb3';
 import { getSolidityLibNames, hasUnlinkedVariables } from '../utils/Bytecode';
 
 export default class Contracts {
-  private static DEFAULT_SYNC_TIMEOUT = 240000;
   private static DEFAULT_BUILD_DIR = `build/contracts`;
   private static DEFAULT_CONTRACTS_DIR = `contracts`;
 
-  private static timeout: number = Contracts.DEFAULT_SYNC_TIMEOUT;
   private static buildDir: string = Contracts.DEFAULT_BUILD_DIR;
   private static contractsDir: string = Contracts.DEFAULT_CONTRACTS_DIR;
   private static projectRoot: string = null;
   private static artifactDefaults: any = {};
   private static defaultFromAddress: string;
 
-  public static getSyncTimeout(): number {
-    return Contracts.timeout || Contracts.DEFAULT_SYNC_TIMEOUT;
-  }
-
   public static getLocalBuildDir(): string {
     return path.resolve(Contracts.buildDir || Contracts.DEFAULT_BUILD_DIR);
   }
@@ -78,10 +72,6 @@ export default class Contracts {
     return glob.sync(`${buildDir}/*.json`);
   }
 
-  public static setSyncTimeout(value: number): void {
-    Contracts.timeout = value;
-  }
-
   public static setLocalBuildDir(dir: string): void {
     Contracts.buildDir = dir;
   }