[contract-manager] Evm upgrades #971

m30m · 2023-07-21T15:39:37Z

No description provided.

…e instructions

vercel · 2023-07-21T15:39:42Z

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

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
example-oracle-amm	⬜️ Ignored (Inspect)			Jul 25, 2023 6:12am
xc-admin-frontend	⬜️ Ignored (Inspect)			Jul 25, 2023 6:12am

ali-behjati · 2023-07-24T08:53:40Z

contract_manager/src/base.ts

+export interface Price {
+  price: string;
+  conf: string;
+  publishTime: string;
+  expo: string;
+}
+
+export interface PriceFeed {
+  price: Price;
+  emaPrice: Price;
+}
+


Hmmm, we have a PriceFeed definition in price-service-sdk. I think it's good to have it as it is now and don't using it. Just saying it for awareness.

ali-behjati · 2023-07-24T08:55:30Z

contract_manager/src/base.ts

  /**
   * Executes the governance instruction contained in the VAA using the sender credentials
-   * @param sender based on the contract type, this can be a private key, a mnemonic, a wallet, etc.
+   * @param senderPrivateKey private key of the sender in hex format without 0x prefix


nitpik: Maybe we can also make this a buffer? In some places i see uint8 array as private key and Buffer seems to a more flexible choice.

So far, I'm seeing hex format as the more common input format. I think if the implementation needs the uint8 array or Buffer it can easily convert the privateKey via Buffer.from

(i think a class is nice for stuff like this because it lets you write methods on the class itself for getting it in hex or other formats. not a big deal though)

ali-behjati · 2023-07-24T08:59:36Z

contract_manager/src/chains.ts

+  }
+
+  getRpcUrl(): string {
+    if (this.rpcUrl.includes("$INFURA_KEY") && !process.env.INFURA_KEY)


nitpik: maybe it's better to do this check on the constructor

I thought about it. If other packages start to use this one, they would all need the INFURA_KEY env variable even if they just want to use something general or not related to evm. That's why I put it here

I agree with ali on this, though also understand your point. Ideally we would have a way to only instantiate the chains the user requests, such that we can perform validity checks on instantiation. Not sure if that's easy to do though.

at the very least, this seems worth a code comment explaining why it's like this and what it would take to improve the situation.

I decided to replace the current RPC endpoints with the public ones as the simplest solution and remove this logic😆

ali-behjati · 2023-07-24T09:03:43Z

contract_manager/src/entities.ts

@@ -206,26 +236,30 @@ export class WormholeMultiSigTransaction {

  async getState() {
    const proposal = await this.squad.getTransaction(this.address);
-    return proposal.status; //TODO: make it typed and parse the status to a more readable format
+    return Object.keys(proposal.status)[0];


JS 🪄 🧙

Added some comments

ali-behjati · 2023-07-24T09:05:35Z

contract_manager/src/evm.ts

+    contract.options.data = bytecode;
+    const deployTx = contract.deploy({ data: "" });


why we couldn't pass bytecode as data 🤔 can you comment that?

I will try that 👍 This was a direct copy-paste from some example contract deployment code on the web

ali-behjati · 2023-07-24T09:05:58Z

contract_manager/src/evm.ts

+    const gas = await deployTx.estimateGas();
+    let gasPrice = await web3.eth.getGasPrice();
+
+    if (!chain.isMainnet()) {


can you comment why we do this?

ali-behjati · 2023-07-24T09:09:10Z

contract_manager/src/evm.ts

+    return transactionObject.send({
+      from: address,
+      value: updateFee,
+      gas: gasEstiamte * 2,


same comment on documenting why we do these.

This was a dirty attempt to eliminate any need for manually specifying gas and gasPrice values per chain. For the gasPrice I was not sure how costly would it be to double the value on mainnet chains, so I only do it for testnet chains for now. If this is not an issue, I can do it everywhere and we don't need to worry a lot about manual entry of these values.
For the gas value, the * 2 is just a very loose upper bound of how much gas is expected to be used.

ali-behjati · 2023-07-24T09:12:44Z

target_chains/cosmwasm/tools/src/chains-manager/cosmwasm.ts

+      mnemonic,
+      { prefix }
+    );
+    await directSecp256k1HdWallet.getAccounts();


why this call is needed? Can you document it? 🤔

ali-behjati · 2023-07-24T09:17:41Z

contract_manager/src/entities.ts

What is the responsibility of this module? Is it the controller? If so is this name the best name? 🤔

This was the first (and only file in the beginning) on this package. I'll organize it soon

ali-behjati

Fantastic! I left some minor inline comments.

I need to look at the codebase entirely to understand all the design decisions and grasp how different components relate to each other (and can be used) but this PR is very solid.

I also recommend moving chains definitions to a sub-module like this:

|- chains
  |- index.ts (or chains.ts on the folder outside)
  |- aptos.ts
  |- evm.ts
  |- ...
|- store.ts
|- entitities.ts

ali-behjati

🚢 it

jayantk

very cool. I left a bunch of comments but nothing major.

jayantk · 2023-07-24T12:44:21Z

contract_manager/src/base.ts

  /**
   * Executes the governance instruction contained in the VAA using the sender credentials
-   * @param sender based on the contract type, this can be a private key, a mnemonic, a wallet, etc.
+   * @param senderPrivateKey private key of the sender in hex format without 0x prefix


(i think a class is nice for stuff like this because it lets you write methods on the class itself for getting it in hex or other formats. not a big deal though)

jayantk · 2023-07-24T12:45:05Z

contract_manager/src/chains.ts

+
+  protected constructor(
+    public id: string,
+    public mainnet: boolean,


should these be private?

jayantk · 2023-07-24T12:45:50Z

contract_manager/src/chains.ts

+  protected constructor(
+    public id: string,
+    public mainnet: boolean,
+    wormholeChainName: string


suggest explaining the difference between id and wormholeChainName in a doc comment.

and that this is pyth's wormholeChainName, not wormhole's

jayantk · 2023-07-24T12:48:20Z

contract_manager/src/chains.ts

+export class GlobalChain extends Chain {
+  static type: string = "GlobalChain";
+  constructor() {
+    super("global", true, "unset");


technically there are two global chains, one for mainnet and one for testnet right?

Technically, it is one global chain x 2 multisig emitters. testnet contracts are listening to one emitter while mainnet contracts are listening to the other one.

jayantk · 2023-07-24T12:59:05Z

contract_manager/src/contracts/aptos.ts

+    };
+  }
+
+  async getPriceFeed(feedId: string) {


please annotate the return type on all public methods

In general, I didn't annotate the implementations or write any docs if the abstract parent function was annotated. I thought most IDEs are smart enough to show relevant information like this:

If that's not the case I can copy paste it in the implementations too.

jayantk · 2023-07-24T13:03:30Z

contract_manager/src/governance.ts

+ * @param senderPrivateKey the private key to execute the governance instruction with
+ * @param vaa the VAA to execute
+ */
+export async function executeVaa(senderPrivateKey: string, vaa: Buffer) {


this is going to be great

jayantk · 2023-07-24T13:05:47Z

contract_manager/src/governance.ts

        );
      } catch (e: any) {
        if (!(e instanceof InvalidTransactionError)) throw e;
      }
    }
+    if (msgs.length > 0) return msgs;


you sure about this check? seems like if none of the txs sent wormhole messages, then the proposal executes successfully and returns 0 messages

So, this class was mainly designed for Wormhole messages, so I thought it should throw an exception if there is actually no wormhole instructions inside. I can add a check in constructor to verify this fact, but if someone is manually creating this object, hopefully they know what they are doing. I will add some comments. In the end, I think the main usage of this class will be in manual/dev testing as a cranker would do this automatically on our pre-configured vaults.

jayantk · 2023-07-24T13:07:53Z

governance/xc_admin/packages/xc_admin_common/src/governance_payload/PythGovernanceAction.ts

@@ -113,6 +113,8 @@ export class PythGovernanceHeader {
      module = MODULE_TARGET;
      action = TargetAction[this.action as keyof typeof TargetAction];
    }
+    if (toChainId(this.targetChainId) === undefined)
+      throw new Error(`Invalid chain id ${this.targetChainId}`);


fwiw we should make validity-checked types for all these things so that we don't need to put checks like this everywhere. ChainName shouldn't be able to represent an invalid chain.

jayantk · 2023-07-24T13:11:00Z

contract_manager/store/chains/EVMChain/aurora_testnet.yaml

+mainnet: false
+rpcUrl: https://testnet.aurora.dev
+networkId: 1313161555
+type: EVMChain


the ship may have sailed on this but i think this should be capitalized like EvmChain. I generally prefer to treat acronyms as a "word" for the purpose of camelcasing -- there's a whole discussion of this in the google coding conventions which i thought was persuasive.

definitely not important though

Yeah, you mentioned this before and I thought I fixed it everywhere. I missed this one, will make the changes 👍

jayantk · 2023-07-24T13:12:15Z

contract_manager/src/chains.ts

+  }
+
+  getRpcUrl(): string {
+    if (this.rpcUrl.includes("$INFURA_KEY") && !process.env.INFURA_KEY)


I agree with ali on this, though also understand your point. Ideally we would have a way to only instantiate the chains the user requests, such that we can perform validity checks on instantiation. Not sure if that's easy to do though.

at the very least, this seems worth a code comment explaining why it's like this and what it would take to improve the situation.

m30m · 2023-07-25T06:13:52Z

Thanks for all the comments, some more cleanups and changes are definitely still remaining to do and will be done in followup PRs.

m30m added 14 commits July 21, 2023 07:53

Add evm contract infos + minor improvements

a39fe64

Throw error if chainId is invalid instead of using 0 silently

c059ca4

Add wormholeChainName property for chains to use in governance actions

5548e11

Return all signatures upon proposal execution

b03c5e6

Unify privateKey schema for contract manager

d68c303

Implement getLastExecutedGovernanceSequence for all contract types

5fb0a17

Unify getPriceFeed interface between contracts

568072a

Add update price feed for evm

b3baf37

Adjust gasPrice on evm

9c2a17c

Add setter for datasources + add global chain for universal governanc…

5e983d7

…e instructions

Add utility function to execute arbitrary governance Vaa

adb10bf

Remove redundant contract

636084e

Better error message when gas is not enough on deployment

b11308a

Add aptos mainnet config

1587632

m30m changed the title ~~Evm upgrades~~ [contract-manager] Evm upgrades Jul 21, 2023

m30m added 2 commits July 21, 2023 17:59

Restore sui override of setFee governance message generation

c29ec38

Export executeVaa function

0a6c1b6

ali-behjati reviewed Jul 24, 2023

View reviewed changes

ali-behjati approved these changes Jul 24, 2023

View reviewed changes

Address PR feedbacks

4a28d12

ali-behjati approved these changes Jul 24, 2023

View reviewed changes

jayantk approved these changes Jul 24, 2023

View reviewed changes

m30m added 6 commits July 24, 2023 18:15

More documentation and cleanup

22f3cc4

Remove INFURA_KEY logic and replace RPC endpoints with public ones

57ee53f

EVMContract -> EvmContract, EVMChain -> EvmChain

4880e1f

Add missing import

1e40fda

Make executeUpdatePriceFeed interface on cosmos similar to evm

8441db3

Better error message and more comment regarding gas

fa22b3b

m30m merged commit 31ad2b6 into main Jul 25, 2023

m30m deleted the evm-upgrades branch July 25, 2023 06:14

		contract.options.data = bytecode;
		const deployTx = contract.deploy({ data: "" });

[contract-manager] Evm upgrades #971

[contract-manager] Evm upgrades #971

Uh oh!

Conversation

m30m commented Jul 21, 2023

Uh oh!

vercel bot commented Jul 21, 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!

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!

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

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment

Choose a reason for hiding this comment

Uh oh!

jayantk left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vercel bot commented Jul 21, 2023 •

edited

Loading