[xc-admin] Contract management tool #885

jayantk · 2023-06-13T20:51:58Z

I've been experimenting with different ways to read / write the state of contracts, and I have an approach that seems reasonable. The idea is to make a collection of Contract objects, each of which represents a single deployed on-chain contract. There can be separate types of Contracts to represent different types of programs (target chain vs. oracle program vs. ...). Each contract exposes its state as a json object which can be both read & written. You can then update contract state by manipulating the JSON object and calling the write method. The JSON interface makes this approach very flexible, as we can change its schema to fit whatever kind of contract we have.

Writing the state back to the chain is a little complicated because we need to run operations that pause and wait for external input (e.g., multisig approvals). I've implemented this using a filesystem dependency, which is not perfect, but is how we've solved this problem in all of our previous implementations. I think we can circle back and fix that dependency later by exposing sufficient APIs on all of our various components to let this code directly query the real world state.

There's also a CLI that lets you query and update the state. The upshot of all of this is that you'll be able to make a change to a set of contracts by running a command like:

npm run cli -- set --network avalanche --type evmTargetChain validTimePeriod=30

then approving the multisig tx and rerunning the above command from the same computer. You can rerun the command whenever and it'll do something sane (e.g., if you approve some proposals but not others, it'll submit the approved changes and wait for the remaining pending changes).

There's still a lot of work to do to finish this up, but I wanted to share to get feedback on the general idea. I'll fix things up in smaller changes as we go.

vercel · 2023-06-13T20:52:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
xc-admin-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 13, 2023 9:03pm

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
example-oracle-amm	⬜️ Ignored (Inspect)			Jun 13, 2023 9:03pm

jayantk · 2023-06-13T21:01:07Z

governance/xc_admin/packages/xc_admin_frontend/next.config.js

    externalDir: true,
  },
  webpack(config) {
+    config.experiments = { asyncWebAssembly: true }


here and below is fixing some weird build issues from pulling the xc-governance-sdk into xc-admin & the frontend components.

guibescos · 2023-06-14T13:01:58Z

governance/xc_admin/packages/xc_admin_common/src/contracts/Contract.ts

@@ -0,0 +1,139 @@
+import { ChainId, Instruction } from "@pythnetwork/xc-governance-sdk";


Do you see any problem in using PythGovernanceAction instead? Instruction from @pythnetwork/xc-governance-sdk. Goal is enumerating all the available governance actions in one place and using the same class for deserializing them in the proposal page.

guibescos

Overall it makes sense to me. It seems an amazing improvement for setting up the config of many contracts on many chains at once.
I guess my question is whether an evm relayer for pyth governance messages is out of the picture, but I guess we can always build the relayer later and make it compatible with this code.

guibescos · 2023-06-14T13:53:50Z

governance/xc_admin/packages/xc_admin_cli/src/cli.ts

+};
+
+// TODO: we will need configuration of this stuff to decide which multisig to run.
+const multisigs = [


I'd reuse https://github.com/pyth-network/pyth-crosschain/blob/main/governance/xc_admin/packages/xc_admin_common/src/multisig.ts

guibescos · 2023-06-14T13:54:32Z

governance/xc_admin/packages/xc_admin_common/src/contracts/Contract.ts

+ * A unique identifier for a blockchain. Note that we cannot use ChainId for this, as ChainId currently reuses
+ * some ids across mainnet / testnet chains (e.g., ethereum goerli has the same id as ethereum mainnet).
+ */
+export type NetworkId = string;


Cartesian product of wormhole network + chainid?

we could use that tuple, but it's going to get confusing, especially as we start rolling out stable/edge configurations to each network. In that case ethereum goerli stable will be [ethereum_goerli, mainnet] and ethereum goerli edge will be [ethereum, testnet]. I think it's better to make a clean break here and define the right concept.

ali-behjati · 2023-06-15T10:44:44Z

governance/xc_admin/packages/xc_admin_cli/package.json

    "build": "tsc",
-    "format": "prettier --write \"src/**/*.ts\""
+    "format": "prettier --write \"src/**/*.ts\"",
+    "cli": "npm run build && node lib/cli.js"


This command is a bit brittle as the build is not stanalone in the monorepo. Can you either run build using npx lerna run build --scope="xc_admin_cli" --include-dependencies && ..

ali-behjati · 2023-06-15T10:55:46Z

governance/xc_admin/packages/xc_admin_common/src/contracts/EvmPythUpgradable.ts

+  }
+
+  public async submitGovernanceVaa(vaa: string): Promise<boolean> {
+    // FIXME: also needs the full PythUpgradable ABI


You will just need to single abi to executeGovernanceInstruction. You can manually write that down and it's fine. I think full abi is not needed at all.

ali-behjati · 2023-06-15T10:58:56Z

governance/xc_admin/packages/xc_admin_common/src/contracts/config.ts

+      const ethersContract = new ethers.Contract(
+        contractConfig.address,
+        PythAbi,
+        getEvmProvider(contractConfig.networkId, networksConfig)
+      );
+
+      return new EvmPythUpgradable(
+        contractConfig.networkId,
+        contractConfig.address,
+        ethersContract
+      );


It's easier to understand (and less coupled) if this be a factory method in the contract file. And in general we automatically call the factory methods without having switch case at all.

ali-behjati · 2023-06-15T11:00:29Z

governance/xc_admin/packages/xc_admin_common/src/contracts/Contract.ts

+export class SendGovernanceInstruction implements SyncOp {
+  private instruction: Instruction;
+  private sender: WormholeAddress;
+  // function to submit the signed VAA to the target chain contract
+  private submitVaa: (vaa: string) => Promise<boolean>;
+
+  constructor(
+    instruction: Instruction,
+    from: WormholeAddress,
+    submitVaa: (vaa: string) => Promise<boolean>
+  ) {
+    this.instruction = instruction;
+    this.sender = from;
+    this.submitVaa = submitVaa;
+  }
+
+  public id(): string {
+    // TODO: use a more understandable identifier (also this may not be unique)
+    return ethers.utils.sha256(this.instruction.serialize());
+  }
+
+  public async run(cache: Record<string, any>): Promise<boolean> {
+    // FIXME: this implementation is temporary. replace with something like the commented out code below.
+    if (cache["multisigTx"] === undefined) {
+      cache["multisigTx"] = "fooooo";
+      return false;
+    }
+
+    if (cache["vaa"] === undefined) {
+      return false;
+    }
+
+    // VAA is guaranteed to be defined here
+    const vaa = cache["vaa"];
+
+    // assertVaaPayloadEquals(vaa, payload);
+
+    return await this.submitVaa(vaa);
+  }
+
+  /*
+  public async run(cache: Record<string,any>): Promise<boolean> {
+    if (cache["multisigTx"] === undefined) {
+      // Have not yet submitted this operation to the multisig.
+      const payload = this.instruction.serialize();
+      const txKey = vault.sendWormholeInstruction(payload);
+      cache["multisigTx"] = txKey;
+      return false;
+    }
+
+    if (cache["vaa"] === undefined) {
+      const vaa = await executeMultisigTxAndGetVaa(txKey, payloadHex);
+      if (vaa === undefined) {
+        return false;
+      }
+      cache["vaa"] = vaa;
+    }
+
+    // VAA is guaranteed to be defined here
+    const vaa = cache["vaa"];
+
+    assertVaaPayloadEquals(vaa, payload);
+
+    // await proxy.executeGovernanceInstruction("0x" + vaa);
+    await submitVaa(vaa);
+  }
+   */
+}


I think they should not be here and should live in their respective contract files.

ali-behjati · 2023-06-15T11:02:33Z

governance/xc_admin/packages/xc_admin_cli/src/cli.ts

+class Cache {
+  private path: string;
+
+  constructor(path: string) {
+    this.path = path;
+  }
+
+  private opFilePath(op: SyncOp): string {
+    return `${this.path}/${op.id()}.json`;
+  }
+
+  public readOpCache(op: SyncOp): Record<string, any> {
+    const path = this.opFilePath(op);
+    if (fs.existsSync(path)) {
+      return JSON.parse(fs.readFileSync(path).toString("utf-8"));
+    } else {
+      return {};
+    }
+  }
+
+  public writeOpCache(op: SyncOp, cache: Record<string, any>) {
+    fs.writeFileSync(this.opFilePath(op), JSON.stringify(cache));
+  }
+
+  public deleteCache(op: SyncOp) {
+    fs.rmSync(this.opFilePath(op));
+  }
+}
+


I think it's better to move the cache to the common package. Ideally the CLI doesn't have any business logic and storage is kind of a business logic. Moving it there allows us to use it in other places if needed too.

ali-behjati · 2023-06-15T11:03:54Z

governance/xc_admin/packages/xc_admin_cli/src/cli.ts

+        (options.network === undefined ||
+          contract.networkId == options.network) &&


why not contract.networkId === options.network (tripple eq + no undefined check)?

ali-behjati · 2023-06-15T11:06:26Z

governance/xc_admin/packages/xc_admin_cli/src/cli.ts

+  .option("-n, --network <network-id>", "Find contracts on the given network")
+  .option("-a, --address <address>", "Find contracts with the given address")
+  .option("-t, --type <type-id>", "Find contracts of the given type")
+  .argument("<fields...>", "Fields to set on the given contracts")


I think having the contract state written in a file is a better idea and easier to understand. Instead of set and get we can have a sync method that checks the written file is a match. The current way is prone to mistakes and requires looking at the source.

ali-behjati

I really like the design! Thanks!

It seems that this code is no utilizing what xc-admin is offering. I think as Guillermo said we can get use of xc-admin structures in the code and it will help with parsing the proposals and displaying them.

My general thinking is that the logics here should be moved to a separate package and only the wire format live in the xc-admin-common to have a better boundry.

I left some inline comments which are mostly my opinions about the structure of the code. I might be wrong but would be good if you address them in later PRs.

Lastly, another cool feature that is good if the CL offer is checking the balance of the our deployer key in different networks.

jayantk added 14 commits June 12, 2023 13:23

cleanup

e0d59b1

blah

84059a1

gr

f68c301

stuff

e673831

Merge branch 'main' into xc_admin_list_contracts

11f94ff

hm

5645973

hmm

f6deb13

wtf

d38d273

ah fix this

028269e

ok finally it does something

d16bc44

ok

667beff

hrm

b5ebd1e

hrm

002c615

blah

f2ce2b7

blah

050b282

jayantk commented Jun 13, 2023

View reviewed changes

vercel bot deployed to Preview – xc-admin-frontend June 13, 2023 21:03 View deployment

guibescos reviewed Jun 14, 2023

View reviewed changes

ali-behjati reviewed Jun 15, 2023

View reviewed changes

ali-behjati approved these changes Jun 15, 2023

View reviewed changes

jayantk merged commit 25c1ac2 into main Jun 15, 2023

jayantk deleted the xc_admin_list_contracts branch June 15, 2023 14:17

		@@ -0,0 +1,139 @@
		import { ChainId, Instruction } from "@pythnetwork/xc-governance-sdk";

		(options.network === undefined \|\|
		contract.networkId == options.network) &&

[xc-admin] Contract management tool #885

[xc-admin] Contract management tool #885

Uh oh!

Conversation

jayantk commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guibescos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jayantk commented Jun 13, 2023 •

edited

Loading

vercel bot commented Jun 13, 2023 •

edited

Loading

guibescos left a comment •

edited

Loading

ali-behjati left a comment •

edited

Loading