Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Aug 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa3783a
Throw an error if registry deploy fails
joewagner Aug 24, 2023
ae5528d
cleanup
joewagner Aug 24, 2023
48113be
bump local patch version
joewagner Aug 25, 2023
87b3c6c
(local) wait 5 sec before runing contract deploy
joewagner Aug 28, 2023
755931a
fix regression in sdk during monorepo switch
joewagner Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/local/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@tableland/local",
"version": "2.0.1",
"version": "2.0.2",
"description": "Tooling to start a sandboxed Tableland network.",
"repository": "https://github.com/tablelandnetwork/local-tableland",
"license": "MIT",
Expand Down
85 changes: 58 additions & 27 deletions packages/local/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import { type ChildProcess } from "node:child_process";
import { EventEmitter } from "node:events";
import spawn from "cross-spawn";
import shell from "shelljs";
import { getDefaultProvider } from "ethers";
import { chalk } from "./chalk.js";
import { ValidatorDev, ValidatorPkg } from "./validators.js";
import {
buildConfig,
type Config,
checkPortInUse,
defaultRegistryDir,
inDebugMode,
isValidPort,
Expand All @@ -22,12 +24,15 @@ import {
getValidator,
logSync,
pipeNamedSubprocess,
probePortInUse,
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";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the client you mean @tableland/evm?


class LocalTableland {
config;
initEmitter;
Expand Down Expand Up @@ -62,7 +67,7 @@ class LocalTableland {
typeof config.registryDir === "string" &&
config.registryDir.trim() !== ""
) {
this.registryDir = config.registryDir;
this.registryDir = config.registryDir.trim();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
this.registryDir = defaultRegistryDir();
}
Expand All @@ -80,19 +85,15 @@ class LocalTableland {
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above... This has already been trimmed.

) {
if (typeof this.registryDir !== "string" || this.registryDir === "") {
throw new Error("cannot start a local network without Registry");
}

// make sure we are starting fresh
// TODO: I don't think this is doing anything anymore...
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const registryPortIsTaken = await checkPortInUse(this.registryPort);
// Note: this generally works, but there is a chance that the port will be
// taken but returns `false`. E.g., try racing two instances at *exactly*
// the same, and `EADDRINUSE` occurs. But generally, it'll work as expected.
Expand All @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Notify that we're using a custom port since it's not the default 8545
}

// Notify that we're using a custom port since it's not the default 8545
if (
this.registryPort !== this.defaultRegistryPort &&
shell.echo(
`[${chalk.magenta.bold("Notice")}] Registry is using custom port ${
this.registryPort
}`
);
this.silent !== true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) {
shell.echo(
`[${chalk.magenta.bold("Notice")}] Registry is using custom port ${
this.registryPort
}`
);
}

// You *must* store these in `process.env` to access within the hardhat subprocess
Expand Down Expand Up @@ -149,17 +154,16 @@ class LocalTableland {
// wait until initialization is done
await waitForReady(registryReadyEvent, this.initEmitter);

// Deploy the Registry to the Hardhat node
logSync(
spawnSync(
isWindows() ? "npx.cmd" : "npx",
["hardhat", "run", "--network", "localhost", "scripts/deploy.ts"],
{
cwd: this.registryDir,
}
),
!inDebugMode()
);
await new Promise((resolve) => setTimeout(resolve, 5000));

this._deployRegistry();

const deployed = await this.#_ensureRegistry();
if (!deployed) {
throw new Error(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"deploying registry contract failed, cannot start network"
);
}

// Need to determine if we are starting the validator via docker
// and a local repo, or if are running a binary etc...
Expand All @@ -172,7 +176,7 @@ class LocalTableland {
// run this before starting in case the last instance of the validator didn't get cleanup after
// this might be needed if a test runner force quits the parent local-tableland process
this.validator.cleanup();
this.validator.start();
this.validator.start(registryAddress);

// TODO: It seems like this check isn't sufficient to see if the process is gonna get to a point
// where the on error listener can be attached.
Expand Down Expand Up @@ -220,6 +224,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// use the ts private modifier here in order to test the failure to deploy the registry.
private _deployRegistry(): void {
// Deploy the Registry to the Hardhat node
logSync(
spawnSync(
isWindows() ? "npx.cmd" : "npx",
["hardhat", "run", "--network", "localhost", "scripts/deploy.ts"],
{
cwd: this.registryDir,
}
),
!inDebugMode()
);
}

async #_ensureRegistry(): Promise<boolean> {
const provider = getDefaultProvider(
`http://127.0.0.1:${this.registryPort}`
);
const code = await provider.getCode(registryAddress);

// if the contract exists, and is not empty, code will not be equal to 0x
return code !== "0x";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart

}

async #_setReady(): Promise<void> {
this.ready = true;
while (this.#_readyResolves.length > 0) {
Expand Down
25 changes: 0 additions & 25 deletions packages/local/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,31 +402,6 @@ export async function checkPortInUse(port: number): Promise<boolean> {
});
}

/**
* Probe a port with retries to check if it is in use.
* @param port The port number.
* @param tries Number of retries to attempt. Defaults to 5.
* @param delay Time to wait between retries (in milliseconds). Defaults to 300.
* @returns true if the port is in use, false otherwise
*/
export async function probePortInUse(
port: number,
tries: number = 5,
delay: number = 300
): Promise<boolean> {
let numTries = 0;
while (numTries < tries) {
// Note: consider splitting the delay into before and after this check
// Racing two instances might cause this to incorrectly return `false`
const portIsTaken = await checkPortInUse(port);
if (!portIsTaken) return false;

await new Promise((resolve) => setTimeout(resolve, delay));
numTries++;
}
return true;
}

const hardhatAccounts = [
"ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80",
"59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d",
Expand Down
16 changes: 8 additions & 8 deletions packages/local/src/validators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class ValidatorPkg {
}
}

start(): void {
start(registryAddress?: string): void {
const binPath = getBinPath();
if (binPath == null) {
throw new Error(
Expand Down Expand Up @@ -141,7 +141,10 @@ class ValidatorDev {
}
}

start(): void {
start(registryAddress?: string): void {
if (typeof registryAddress !== "string") {
throw new Error("must provide registry address");
}
// Add the registry address to the Validator config
// TODO: when https://github.com/tablelandnetwork/go-tableland/issues/317 is
// resolved we may be able to refactor a lot of this
Expand All @@ -163,14 +166,11 @@ class ValidatorDev {
if (
validatorConfig.Chains[0].Registry.EthEndpoint !==
`ws://localhost:${this.registryPort}`
)
) {
validatorConfig.Chains[0].Registry.EthEndpoint = `ws://localhost:${this.registryPort}`;
}

// TODO: this could be parsed out of the deploy process, but since
// it's always the same address just hardcoding it here
// TODO: maybe we can get this from evm-tableland?
validatorConfig.Chains[0].Registry.ContractAddress =
"0xe7f1725e7734ce288f8367e1bb143e90bb3f0512";
validatorConfig.Chains[0].Registry.ContractAddress = registryAddress;

writeFileSync(configFilePath, JSON.stringify(validatorConfig, null, 2));

Expand Down
Loading
Loading