throw an error if the registry contract hasn't been deployed #22

joewagner · 2023-08-24T19:45:08Z

Overview

As an improvement working toward fixing #40 this PR introduces behavior that check if the registry has been deployed before starting the Validator. If the contract is not deployed an error is thrown. This will make CI tests fail immediately and obviously, which will make the reason for failure more obvious.

Details

Along with requiring the existence of the registry on chain for successful startup, there are a few other changes:

This reverts some of the port failure retry logic since that didn't solve the issue with CI failures.
This moves the startup and shutdown tests into their own test file
This moves the hard coded registry contract address up one level, from the validator.ts file to the main.ts file. I needed to do this to enable checking the existence of the deployed contract. This also moves one step closer to getting the contract out of the deploy, which isn't possible atm since the deploy fails silently.

NOTE: this is based off the publish branch. Once that's merged this will be pointed at main.

joewagner · 2023-08-24T23:05:46Z

packages/local/src/main.ts

  waitForReady,
 } from "./util.js";

 const spawnSync = spawn.sync;

+// TODO: maybe this can be parsed out of the deploy process?
+// Since it's always the same address just hardcoding for now.
+const registryAddress = "0xe7f1725e7734ce288f8367e1bb143e90bb3f0512";


This used to reside in the validator.ts file, but we need access to it here now so it's been moved. If we can find a way to get this value from the deploy process, that would be ideal. Until then this works.

We can't just get this from the underlying client package?

by the client you mean @tableland/evm?

joewagner · 2023-08-24T23:06:47Z

packages/local/src/main.ts

@@ -62,7 +67,7 @@ class LocalTableland {
      typeof config.registryDir === "string" &&
      config.registryDir.trim() !== ""
    ) {
-      this.registryDir = config.registryDir;
+      this.registryDir = config.registryDir.trim();


trimming before setting the instance value, which means all later access doesn't need to trim

joewagner · 2023-08-24T23:07:08Z

packages/local/src/main.ts

@@ -80,19 +85,15 @@ class LocalTableland {
  }

  async #_start(config: Config = {}): Promise<void> {
-    if (
-      typeof this.registryDir !== "string" ||
-      this.registryDir.trim() === ""


See above... This has already been trimmed.

joewagner · 2023-08-24T23:10:26Z

packages/local/src/main.ts

    this.#_cleanup();

    // Check if the hardhat port is in use (defaults to 5 retries, 300ms b/w each try)
-    const registryPortIsTaken = await probePortInUse(this.registryPort);


An old PR attempted to fix the CI issues by retrying the same hardhat node port in hopes it will become available. The issue comes from a crashing contract deploy process, not a failure to start the hardhat node. We no longer need this retry logic. I've left the logic that ensures the port is available, since this is a nice mechanism to fail fast and loud if the port was already taken.

joewagner · 2023-08-24T23:11:42Z

packages/local/src/main.ts

@@ -101,14 +102,18 @@ class LocalTableland {
    // Else, notify the user only if it's a not the default and is custom.
    if (registryPortIsTaken) {
      throw new Error(`port ${this.registryPort} already in use`);
-    } else {


Some cleanup here. We don't need an else clause because throwing breaks out of this function.

joewagner · 2023-08-24T23:12:17Z

packages/local/src/main.ts

-            this.registryPort
-          }`
-        );
+      this.silent !== true


This was getting logged even if the dev had set silent: true

joewagner · 2023-08-24T23:13:50Z

packages/local/src/main.ts

+
+    const deployed = await this.#_ensureRegistry();
+    if (!deployed) {
+      throw new Error(


This ensures we fail fast and loud if the deploy failed. At the moment you have to look through the CI logs and see if the failing tests are becaseue the registry wasn't deployed, or for some legitimate reason.

joewagner · 2023-08-24T23:15:10Z

packages/local/src/main.ts

+    const code = await provider.getCode(registryAddress);
+
+    // if the contract exists, and is not empty, code will not be equal to 0x
+    return code !== "0x";


To check if the registry was deployed, we can use the ethers.js getCode method. If the contract exists it will have a long string corresponding to the contract code. If not it will equal "0x".

joewagner · 2023-08-24T23:17:11Z

packages/local/test/e2e.test.ts

@@ -1,220 +1,17 @@
-import { type Server } from "node:net";


The changes in this file are the result of moving the network startup and shutdown tests to their own file.

joewagner · 2023-08-24T23:18:20Z

packages/local/test/start.test.ts

+  test("fails to start due to registry deploy failure", async function () {
+    const lt = new LocalTableland({ silent: true });
+    // note: need to cast as `any` since the method is private
+    const deployStub = stub(lt, "_deployRegistry" as any);


I created a noop stub so the registry isn't deployed. This must result in an error instead of successful startup.

All the other tests in this file were moved from the e2e file.

carsonfarmer

Really nice work here @joewagner!

carsonfarmer · 2023-08-25T16:41:17Z

packages/local/src/main.ts

  waitForReady,
 } from "./util.js";

 const spawnSync = spawn.sync;

+// TODO: maybe this can be parsed out of the deploy process?
+// Since it's always the same address just hardcoding for now.
+const registryAddress = "0xe7f1725e7734ce288f8367e1bb143e90bb3f0512";


We can't just get this from the underlying client package?

carsonfarmer · 2023-08-25T16:44:17Z

packages/local/src/main.ts

@@ -220,6 +222,33 @@ class LocalTableland {
    console.log("\n\n*************************************\n");
  }

+  // note: Tests are using sinon to stub this method. Because typescript compiles ecmascript
+  //       private features, i.e. hash syntax, in a way that does not work with sinon we must


Wait, really? Huh, there are apparently other libs that have a problem with this. Do you have a reference for the issue? Just for interest sake?

Great question, I couldn't find an issue in Sinon. Maybe I should open one? Here's the Sinon source https://github.com/sinonjs/sinon/blob/main/lib/sinon/util/core/is-non-existent-property.js#L10
When you have a class the uses the js private class feature, you'll always get false when you do something like:

class Derp { #_foo() { return "foo"; } } const instance = new Derp(); console.log("#_foo" in instance); // => false console.log(instance.#_foo); // => type error

Maybe it's impossible to spy on private fields?

carsonfarmer · 2023-08-25T16:45:20Z

packages/local/src/main.ts

+    const code = await provider.getCode(registryAddress);
+
+    // if the contract exists, and is not empty, code will not be equal to 0x
+    return code !== "0x";


The base branch was changed.

joewagner · 2023-08-25T19:18:21Z

@carsonfarmer This was automatically changed to point at main after merging tablelandnetwork/local-tableland#21, can you give another ✅ ?

[EDIT]: almost forgot to bump the patch version, gtg now.

joewagner · 2023-08-28T17:00:20Z

@asutula I added the fix for the SDK regression here since without the changes in this PR the CI fails constantly.
Can you give a 👍 to this?

Throw an error if registry deploy fails

aa3783a

joewagner force-pushed the joe/fix-ci branch from 60efc47 to aa3783a Compare August 24, 2023 19:48

joewagner changed the base branch from main to publish August 24, 2023 20:13

joewagner marked this pull request as ready for review August 24, 2023 20:41

joewagner requested a review from carsonfarmer as a code owner August 24, 2023 20:41

cleanup

ae5528d

joewagner commented Aug 24, 2023

View reviewed changes

carsonfarmer previously approved these changes Aug 25, 2023

View reviewed changes

Base automatically changed from publish to main August 25, 2023 18:49

joewagner added 3 commits August 25, 2023 14:49

bump local patch version

48113be

(local) wait 5 sec before runing contract deploy

87b3c6c

fix regression in sdk during monorepo switch

755931a

asutula approved these changes Aug 28, 2023

View reviewed changes

joewagner merged commit 04cdf8a into main Aug 28, 2023
4 checks passed

joewagner deleted the joe/fix-ci branch August 28, 2023 17:08

joewagner mentioned this pull request Aug 29, 2023

Using this package to run tests with github actions is buggy #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

throw an error if the registry contract hasn't been deployed #22

throw an error if the registry contract hasn't been deployed #22

joewagner commented Aug 24, 2023 •

edited

Loading

joewagner Aug 24, 2023

carsonfarmer Aug 25, 2023

joewagner Aug 25, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

carsonfarmer Aug 25, 2023

joewagner Aug 24, 2023

joewagner Aug 24, 2023

carsonfarmer left a comment

carsonfarmer Aug 25, 2023

carsonfarmer Aug 25, 2023

joewagner Aug 25, 2023

carsonfarmer Aug 25, 2023

joewagner commented Aug 25, 2023 •

edited

Loading

joewagner commented Aug 28, 2023

throw an error if the registry contract hasn't been deployed #22

throw an error if the registry contract hasn't been deployed #22

Conversation

joewagner commented Aug 24, 2023 • edited Loading

Overview

Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonfarmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joewagner commented Aug 25, 2023 • edited Loading

joewagner commented Aug 28, 2023

joewagner commented Aug 24, 2023 •

edited

Loading

joewagner commented Aug 25, 2023 •

edited

Loading