Improve ElectrumNetworkProvider reliability

closes #74
- Keep counter of concurrentConnections, and only open a connection
  if the counter is at 0, and only close the connection when the
  counter is at 1.
- Update tests to prevent concurrency issues.

Author: rkalis

jest.setup.js

@@ -1 +1 @@
-jest.setTimeout(25000);
+jest.setTimeout(15000);

packages/cashscript/src/network/ElectrumNetworkProvider.ts

@@ -11,6 +11,7 @@ import { addressToLockScript, sha256 } from '../util';
 
 export default class ElectrumNetworkProvider implements NetworkProvider {
   private electrum: ElectrumCluster;
+  private concurrentRequests: number = 0;
 
   constructor(
     public network: Network = Network.MAINNET,
@@ -82,34 +83,51 @@ export default class ElectrumNetworkProvider implements NetworkProvider {
     }
   }
 
-  disconnectCluster(): boolean[] {
+  async disconnectCluster(): Promise<boolean[]> {
     return this.electrum.shutdown();
   }
 
   private async performRequest(
     name: string,
     ...parameters: (string | number | boolean)[]
   ): Promise<RequestResponse> {
-    if (!this.manualConnectionManagement) {
+    // Only connect the cluster when no concurrent requests are running
+    if (this.shouldConnect()) {
       this.connectCluster();
     }
 
+    this.concurrentRequests += 1;
+
     await this.electrum.ready();
 
     let result;
     try {
       result = await this.electrum.request(name, ...parameters);
     } finally {
       // Always disconnect the cluster, also if the request fails
-      if (!this.manualConnectionManagement) {
-        this.disconnectCluster();
+      if (this.shouldDisconnect()) {
+        await this.disconnectCluster();
       }
     }
 
+    this.concurrentRequests -= 1;
+
     if (result instanceof Error) throw result;
 
     return result;
   }
+
+  private shouldConnect(): boolean {
+    if (this.manualConnectionManagement) return false;
+    if (this.concurrentRequests !== 0) return false;
+    return true;
+  }
+
+  private shouldDisconnect(): boolean {
+    if (this.manualConnectionManagement) return false;
+    if (this.concurrentRequests !== 1) return false;
+    return true;
+  }
 }
 
 interface ElectrumUtxo {

packages/cashscript/test/e2e/FullStack.test.ts

@@ -1,9 +1,9 @@
 import { Contract, SignatureTemplate, FullStackNetworkProvider } from '../../src';
 import {
-  alicePkh,
-  alicePk,
   alice,
   bob,
+  bobPkh,
+  bobPk,
 } from '../fixture/vars';
 import { getTxOutputs } from '../test-util';
 import { FailedSigCheckError, Reason } from '../../src/Errors';
@@ -17,7 +17,7 @@ describe('P2PKH (using FullStackNetworkProvider)', () => {
     // eslint-disable-next-line global-require
     const artifact = require('../fixture/p2pkh.json');
     const provider = new FullStackNetworkProvider('mainnet', new BCHJS({ restURL: 'https://free-main.fullstack.cash/v3/' }));
-    p2pkhInstance = new Contract(artifact, [alicePkh], provider);
+    p2pkhInstance = new Contract(artifact, [bobPkh], provider);
     console.log(p2pkhInstance.address);
   });
 
@@ -29,7 +29,7 @@ describe('P2PKH (using FullStackNetworkProvider)', () => {
 
       // when
       const txPromise = p2pkhInstance.functions
-        .spend(alicePk, new SignatureTemplate(bob))
+        .spend(bobPk, new SignatureTemplate(alice))
         .to(to, amount)
         .send();
 
@@ -45,7 +45,7 @@ describe('P2PKH (using FullStackNetworkProvider)', () => {
 
       // when
       const tx = await p2pkhInstance.functions
-        .spend(alicePk, new SignatureTemplate(alice))
+        .spend(bobPk, new SignatureTemplate(bob))
         .to(to, amount)
         .send();
 

packages/cashscript/test/e2e/P2PKH.test.ts

@@ -102,30 +102,41 @@ describe('P2PKH', () => {
       });
     });
 
-    it('can send to multiple recipients', async () => {
+    it('can call to() multiple times', async () => {
       // given
       const outputs = [
         { to: p2pkhInstance.address, amount: 10000 },
         { to: p2pkhInstance.address, amount: 20000 },
       ];
 
       // when
-      const tx1 = await p2pkhInstance.functions
+      const tx = await p2pkhInstance.functions
         .spend(alicePk, new SignatureTemplate(alice))
-        .to(outputs)
+        .to(outputs[0].to, outputs[0].amount)
+        .to(outputs[1].to, outputs[1].amount)
         .send();
 
-      const tx2 = await p2pkhInstance.functions
+      // then
+      const txOutputs = getTxOutputs(tx);
+      expect(txOutputs).toEqual(expect.arrayContaining(outputs));
+    });
+
+    it('can send to list of recipients', async () => {
+      // given
+      const outputs = [
+        { to: p2pkhInstance.address, amount: 10000 },
+        { to: p2pkhInstance.address, amount: 20000 },
+      ];
+
+      // when
+      const tx = await p2pkhInstance.functions
         .spend(alicePk, new SignatureTemplate(alice))
-        .to(outputs[0].to, outputs[0].amount)
-        .to(outputs[1].to, outputs[1].amount)
+        .to(outputs)
         .send();
 
       // then
-      const txOutputs1 = getTxOutputs(tx1);
-      const txOutputs2 = getTxOutputs(tx2);
-      expect(txOutputs1).toEqual(expect.arrayContaining(outputs));
-      expect(txOutputs2).toEqual(expect.arrayContaining(outputs));
+      const txOutputs = getTxOutputs(tx);
+      expect(txOutputs).toEqual(expect.arrayContaining(outputs));
     });
 
     it('can include OP_RETURN data as an output', async () => {

packages/cashscript/test/e2e/TransferWithTimeout.test.ts

@@ -23,7 +23,7 @@ describe('TransferWithTimeout', () => {
   });
 
   describe('send', () => {
-    it('should fail when using incorrect function arguments', async () => {
+    it('should fail when using incorrect function arguments to transfer', async () => {
       // given
       const to = twtInstancePast.address;
       const amount = 10000;
@@ -34,19 +34,28 @@ describe('TransferWithTimeout', () => {
         .to(to, amount)
         .send();
 
-      const txPromise2 = twtInstancePast.functions
+      // then
+      await expect(txPromise).rejects.toThrow(FailedSigCheckError);
+      await expect(txPromise).rejects.toThrow(Reason.SIG_NULLFAIL);
+    });
+
+    it('should fail when using incorrect function arguments to timeout', async () => {
+      // given
+      const to = twtInstancePast.address;
+      const amount = 10000;
+
+      // when
+      const txPromise = twtInstancePast.functions
         .timeout(new SignatureTemplate(bob))
         .to(to, amount)
         .send();
 
       // then
       await expect(txPromise).rejects.toThrow(FailedSigCheckError);
       await expect(txPromise).rejects.toThrow(Reason.SIG_NULLFAIL);
-      await expect(txPromise2).rejects.toThrow(FailedSigCheckError);
-      await expect(txPromise2).rejects.toThrow(Reason.SIG_NULLFAIL);
     });
 
-    it('should fail when called before timeout', async () => {
+    it('should fail when timeout is called before timeout block', async () => {
       // given
       const to = twtInstanceFuture.address;
       const amount = 10000;
@@ -62,61 +71,52 @@ describe('TransferWithTimeout', () => {
       await expect(txPromise).rejects.toThrow(Reason.UNSATISFIED_LOCKTIME);
     });
 
-    it('should succeed when using correct function arguments', async () => {
+    it('should succeed when transfer is called after timeout block', async () => {
       // given
-      const toFuture = twtInstanceFuture.address;
-      const toPast = twtInstancePast.address;
+      const to = twtInstancePast.address;
       const amount = 10000;
 
       // when
-      const tx1 = await twtInstancePast.functions
+      const tx = await twtInstancePast.functions
         .transfer(new SignatureTemplate(bob))
-        .to(toPast, amount)
-        .send();
-
-      const tx2 = await twtInstanceFuture.functions
-        .transfer(new SignatureTemplate(bob))
-        .to(toFuture, amount)
-        .send();
-
-      const tx3 = await twtInstancePast.functions
-        .timeout(new SignatureTemplate(alice))
-        .to(toPast, amount)
+        .to(to, amount)
         .send();
 
       // then
-      const tx1Outputs = getTxOutputs(tx1);
-      const tx2Outputs = getTxOutputs(tx2);
-      const tx3Outputs = getTxOutputs(tx3);
-      expect(tx1Outputs).toEqual(expect.arrayContaining([{ to: toPast, amount }]));
-      expect(tx2Outputs).toEqual(expect.arrayContaining([{ to: toFuture, amount }]));
-      expect(tx3Outputs).toEqual(expect.arrayContaining([{ to: toPast, amount }]));
+      const txOutputs = getTxOutputs(tx);
+      expect(txOutputs).toEqual(expect.arrayContaining([{ to, amount }]));
     });
 
-    it('can send to multiple recipients', async () => {
+    it('should succeed when transfer is called before timeout block', async () => {
       // given
-      const outputs = [
-        { to: twtInstancePast.address, amount: 10000 },
-        { to: twtInstancePast.address, amount: 20000 },
-      ];
+      const to = twtInstanceFuture.address;
+      const amount = 10000;
 
       // when
-      const tx1 = await twtInstancePast.functions
+      const tx = await twtInstanceFuture.functions
         .transfer(new SignatureTemplate(bob))
-        .to(outputs)
+        .to(to, amount)
         .send();
 
-      const tx2 = await twtInstancePast.functions
+      // then
+      const txOutputs = getTxOutputs(tx);
+      expect(txOutputs).toEqual(expect.arrayContaining([{ to, amount }]));
+    });
+
+    it('should succeed when timeout is called after timeout block', async () => {
+      // given
+      const to = twtInstancePast.address;
+      const amount = 10000;
+
+      // when
+      const tx = await twtInstancePast.functions
         .timeout(new SignatureTemplate(alice))
-        .to(outputs[0].to, outputs[0].amount)
-        .to(outputs[1].to, outputs[1].amount)
+        .to(to, amount)
         .send();
 
       // then
-      const txOutputs1 = getTxOutputs(tx1);
-      const txOutputs2 = getTxOutputs(tx2);
-      expect(txOutputs1).toEqual(expect.arrayContaining(outputs));
-      expect(txOutputs2).toEqual(expect.arrayContaining(outputs));
+      const txOutputs = getTxOutputs(tx);
+      expect(txOutputs).toEqual(expect.arrayContaining([{ to, amount }]));
     });
   });
 });