Open threads

package.json

@@ -23,8 +23,8 @@
     "ganache:stop": "kill `ps ax|grep ganache-cli|grep -v grep| awk '{print $1}'` 2> /dev/null; exit 0",
     "truffle:migrate": "npx lerna run migrate --scope tasit-contracts --stream",
     "prepare:blockchain": "npm run ganache:stop && npm run ganache:start && npm run truffle:migrate",
-    "pretest": "npm run prepare:blockchain",
-    "test": "npm run lint && npx lerna run test --stream --concurrency 1",
+    "pretest": "npm run prepare:blockchain && npm run lint",
+    "test": "npx lerna run test --stream --concurrency 1",
     "posttest": "npm run ganache:stop",
     "start": "npm run clean:all && npm run bootstrap && npm run prepare:blockchain",
     "lint": "npx prettier --write './packages/*/src/{*.js,**/*.js}' && npx eslint './packages/*/src/{*.js,**/*.js}'"

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

@@ -12,7 +12,6 @@ export class Action extends Subscription {
   #txConfirmations = 0;
   #timeout;
   #lastConfirmationTime;
-  #isRunning = false;
 
   constructor(rawAction, provider, signer) {
     // Provider implements EventEmitter API and it's enough
@@ -124,7 +123,7 @@ export class Action extends Subscription {
     const eventName = "confirmation";
 
     // eslint-disable-next-line no-unused-vars
-    const baseEthersListener = async blockNumber => {
+    const ethersListener = async blockNumber => {
       try {
         const tx = await this.#tx;
         if (!tx) {
@@ -198,30 +197,6 @@ export class Action extends Subscription {
       }
     };
 
-    // 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.test.js

@@ -159,8 +159,6 @@ describe("TasitAction.Contract", () => {
         const contractErrorListener = sinon.fake(error => {
           const { message } = error;
           console.info(message);
-
-          sampleContract.off("error");
         });
 
         sampleContract.on("error", contractErrorListener);
@@ -472,9 +470,7 @@ describe("TasitAction.Contract", () => {
     //  new difficultywise-longest well-formed blockchain which excludes one or more blocks that
     //  the client previously thought were part of the difficultywise-longest well-formed blockchain.
     //  These excluded blocks become orphans.
-    //
-    // Non-deterministic
-    it.skip("should emit error event when block reorganization occurs - tx confirmed twice", async () => {
+    it("should emit error event when block reorganization occurs - tx confirmed twice", async () => {
       const confirmationListener = sinon.fake();
       const errorFn = sinon.fake();
 
@@ -533,8 +529,7 @@ describe("TasitAction.Contract", () => {
       expect(actionId).to.have.lengthOf(66);
     });
 
-    // Non-deterministic
-    it.skip("should be able to listen to an event before sending", async () => {
+    it("should be able to listen to an event before sending", async () => {
       const confirmationListener = sinon.fake(async () => {
         action.off("confirmation");
       });

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

@@ -31,16 +31,7 @@ export class Subscription {
 
   // TODO: Make protected
   _setEventTimer = (eventName, timer) => {
-    const eventListener = this.#eventListeners.get(eventName);
-
-    if (!eventListener) {
-      console.warn(`A listener for event '${eventName}' isn't registered.`);
-      return;
-    }
-
-    const { listener } = eventListener;
-
-    this.#eventListeners.set(eventName, { listener, timer });
+    this.#decorateEventListener(eventName, { timer });
   };
 
   // TODO: Make protected
@@ -93,8 +84,9 @@ export class Subscription {
   // If there is a error event already, it will be replaced by new listener function
   // that will call both new and old functions
   _addErrorListener = newListener => {
+    const eventName = "error";
     let listener = newListener;
-    const oldErrorEventListener = this.#eventListeners.get("error");
+    const oldErrorEventListener = this.#eventListeners.get(eventName);
 
     if (oldErrorEventListener) {
       listener = error => {
@@ -103,13 +95,13 @@ export class Subscription {
       };
     }
 
-    this.#eventListeners.set("error", {
+    this.#eventListeners.set(eventName, {
       listener,
     });
   };
 
   // TODO: Make protected
-  _addEventListener = (eventName, listener) => {
+  _addEventListener = (eventName, baseListener) => {
     if (eventName === "error")
       throw new Error(
         `Use _addErrorListener function to subscribe to an error event.`
@@ -120,9 +112,36 @@ export class Subscription {
         `A listener for event '${eventName}' is already registered.`
       );
 
-    this.#eventListeners.set(eventName, {
-      listener,
-    });
+    // 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
+    //
+    // Question:
+    // Is it possible that that behavior (listener concurrent calls for the same event) is desirable?
+    const listener = async (...args) => {
+      const eventListener = this.#eventListeners.get(eventName);
+      const { isRunning } = eventListener;
+
+      if (isRunning) {
+        console.info(`Listener is already running`);
+        return;
+      }
+
+      this.#decorateEventListener(eventName, { isRunning: true });
+      await baseListener(...args);
+      this.#decorateEventListener(eventName, { isRunning: false });
+    };
+
+    const eventListener = { listener, isRunning: false };
+
+    this.#eventListeners.set(eventName, eventListener);
 
     this.#ethersEventEmitter.on(this._toEthersEventName(eventName), listener);
   };
@@ -131,6 +150,19 @@ export class Subscription {
   getEmitter = () => {
     return this.#ethersEventEmitter;
   };
+
+  #decorateEventListener = (eventName, newArgs) => {
+    let eventListener = this.#eventListeners.get(eventName);
+
+    if (!eventListener) {
+      console.warn(`A listener for event '${eventName}' isn't registered.`);
+      return;
+    }
+
+    eventListener = { ...eventListener, ...newArgs };
+
+    this.#eventListeners.set(eventName, eventListener);
+  };
 }
 
 export default Subscription;

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

@@ -139,10 +139,7 @@ describe("TasitAction.ERC721.ERC721Full", () => {
       await expectExactTokenBalances(erc721, [ana.address], [1]);
     });
 
-    // Non-deterministic
-    // Enable again after solve this isse:
-    // https://github.com/tasitlabs/TasitSDK/issues/367
-    it.skip("should transfer an owned token", async () => {
+    it("should transfer an owned token", async () => {
       erc721 = new ERC721Full(ERC721_FULL_ADDRESS, ana);
 
       const transferListener = sinon.fake(message => {

packages/tasit-sdk/src/Decentraland.test.js

@@ -215,7 +215,43 @@ describe("Decentraland", () => {
           })();
         });
 
-        it("should buy an estate", done => {
+        it("should buy an estate - using action events", done => {
+          (async () => {
+            const { assetId, nftAddress, priceInWei } = estateForSale;
+
+            await checkAsset(estate, mana, estateForSale, ephemeralAddress);
+
+            await expectExactTokenBalances(estate, [ephemeralAddress], [0]);
+
+            const fingerprint = await estate.getFingerprint(`${assetId}`);
+
+            marketplace.setWallet(ephemeralWallet);
+            const executeOrderAction = marketplace.safeExecuteOrder(
+              nftAddress,
+              `${assetId}`,
+              `${priceInWei}`,
+              `${fingerprint}`
+            );
+
+            const confirmationListener = async () => {
+              executeOrderAction.off("error");
+              await expectExactTokenBalances(estate, [ephemeralAddress], [1]);
+              done();
+            };
+
+            const errorListener = error => {
+              executeOrderAction.off("error");
+              done(error);
+            };
+
+            executeOrderAction.once("confirmation", confirmationListener);
+            executeOrderAction.on("error", errorListener);
+
+            executeOrderAction.send();
+          })();
+        });
+
+        it("should buy an estate - using contract events", done => {
           (async () => {
             const { assetId, nftAddress, priceInWei } = estateForSale;
 
@@ -233,40 +269,67 @@ describe("Decentraland", () => {
               `${fingerprint}`
             );
 
-            // eslint-disable-next-line no-unused-vars
             const orderSuccessfulListener = async message => {
               const { data } = message;
               const { args } = data;
               const { buyer } = args;
-
+              marketplace.off("error");
               expect(buyer).to.equal(ephemeralAddress);
               await expectExactTokenBalances(estate, [ephemeralAddress], [1]);
-
               done();
             };
 
+            const errorListener = error => {
+              const { message } = error;
+              console.warn(message);
+              marketplace.off("error");
+              done(error);
+            };
+
+            marketplace.once("OrderSuccessful", orderSuccessfulListener);
+            marketplace.on("error", errorListener);
+
+            executeOrderAction.send();
+          })();
+        });
+
+        it("should buy a parcel of land - using action events", done => {
+          (async () => {
+            const { assetId, nftAddress, priceInWei } = landForSale;
+
+            await checkAsset(land, mana, landForSale, ephemeralAddress);
+
+            await expectExactTokenBalances(land, [ephemeralAddress], [0]);
+
+            // LANDRegistry contract doesn't implement getFingerprint function
+            const fingerprint = "0x";
+            marketplace.setWallet(ephemeralWallet);
+            const executeOrderAction = marketplace.safeExecuteOrder(
+              nftAddress,
+              `${assetId}`,
+              `${priceInWei}`,
+              `${fingerprint}`
+            );
+
             const confirmationListener = async () => {
-              await expectExactTokenBalances(estate, [ephemeralAddress], [1]);
+              executeOrderAction.off("error");
+              await expectExactTokenBalances(land, [ephemeralAddress], [1]);
               done();
             };
 
             const errorListener = error => {
-              marketplace.off("error");
+              executeOrderAction.off("error");
               done(error);
             };
 
-            // Note: These listeners aren't working properly
-            // See more: https://github.com/tasitlabs/TasitSDK/issues/367
-            //marketplace.once("OrderSuccessful", orderSuccessfulListener);
-            //marketplace.on("error", errorListener);
             executeOrderAction.once("confirmation", confirmationListener);
             executeOrderAction.on("error", errorListener);
 
             executeOrderAction.send();
           })();
         });
 
-        it("should buy a parcel of land", done => {
+        it("should buy a parcel of land - using contract events", done => {
           (async () => {
             const { assetId, nftAddress, priceInWei } = landForSale;
 
@@ -284,20 +347,14 @@ describe("Decentraland", () => {
               `${fingerprint}`
             );
 
-            // eslint-disable-next-line no-unused-vars
             const orderSuccessfulListener = async message => {
               const { data } = message;
               const { args } = data;
               const { buyer } = args;
 
               expect(buyer).to.equal(ephemeralAddress);
               await expectExactTokenBalances(land, [ephemeralAddress], [1]);
-
-              done();
-            };
-
-            const confirmationListener = async () => {
-              await expectExactTokenBalances(land, [ephemeralAddress], [1]);
+              marketplace.off("error");
               done();
             };
 
@@ -306,12 +363,8 @@ describe("Decentraland", () => {
               done(error);
             };
 
-            // Note: These listeners aren't working properly
-            // See more: https://github.com/tasitlabs/TasitSDK/issues/367
-            // marketplace.once("OrderSuccessful", orderSuccessfulListener);
-            // marketplace.on("error", errorListener);
-            executeOrderAction.once("confirmation", confirmationListener);
-            executeOrderAction.on("error", errorListener);
+            marketplace.once("OrderSuccessful", orderSuccessfulListener);
+            marketplace.on("error", errorListener);
 
             executeOrderAction.send();
           })();
@@ -651,6 +704,8 @@ describe("Decentraland", () => {
 
             const confirmationListener = async () => {
               // WIP: Not working because of gas issue on Marketplace.safeExecuteOrder() call
+              //
+              // When that issue was solved, the correct expectation is having balance equal 1 instead of 0
               //await expectExactTokenBalances(estate, [GNOSIS_SAFE_ADDRESS], [1]);
               await expectExactTokenBalances(
                 estate,
@@ -720,6 +775,8 @@ describe("Decentraland", () => {
             const confirmationListener = async () => {
               // WIP: Not working because of gas issue on Marketplace.safeExecuteOrder() call
               //await expectExactTokenBalances(land, [GNOSIS_SAFE_ADDRESS], [1]);
+              //
+              // When that issue was solved, the correct expectation is having balance equal 1 instead of 0
               await expectExactTokenBalances(land, [GNOSIS_SAFE_ADDRESS], [0]);
               done();
             };
@@ -880,7 +937,6 @@ describe("Decentraland", () => {
               // See more: https://github.com/tasitlabs/TasitSDK/issues/273
               //await expectExactTokenBalances(estate, [ephemeralAddress], [1]);
               await expectExactTokenBalances(estate, [ephemeralAddress], [0]);
-              done();
             };
 
             const errorListener = error => {
@@ -929,7 +985,6 @@ describe("Decentraland", () => {
               // See more: https://github.com/tasitlabs/TasitSDK/issues/273
               //await expectExactTokenBalances(land, [ephemeralAddress], [1]);
               await expectExactTokenBalances(land, [ephemeralAddress], [0]);
-              done();
             };
 
             const errorListener = error => {