Audit config shapes

[nambrot]

We have a bunch of different configs and artifacts of varying shapes that are slightly different which makes it very annoying.

things like:

• chain metadata (chain ID, name, rpc)
• hyperlane artifacts (like core contracts, igps, isms, warp route contracts)
• agent specific config (reorg periods, signers)

[nambrot]

Currently, the config we use are the following

Types of config

1. Chain connection

• Chain/Domain ID
• Default/Public RPC Urls
• block explorers
• finality for waiting for tx receipts and indexing

2. Chain Safe Reorg Period

• finality for validators

3. Core Contract artifacts

• Mailbox address
• validator announce

4. ISM artifacts

• ISM addresses

5. ISM configuration

• validator addresses
• thresholds
• composition of routing/aggregation

6. IGP configuration

• gas oracles prices
• ISM overhead

7. IGP artifacts

• IGP addresses

8. Agent indexing config

• from
• page size

9. Testing artifacts

• Test Recipients

10. warp route token config

• collateral token address
• remote chain names

11. Runtime agent configuration

• safe reorg period
• signers
• RPC urls

12. Warp Route artifacts

• Collateral Router address
• Synthetic router addresses

13. Owner config

• owner address

Usages

SDK chain metadata

Contains: 1)

Usage:

• defaults for core chains in

  hyperlane-monorepo/typescript/sdk/src/consts/chainMetadata.ts

  Line 156 in 5b021c6

  export const alfajores: ChainMetadata = {

• infra re-exports mostly for gas overrrides

  hyperlane-monorepo/typescript/infra/config/environments/mainnet2/chains.ts

  Line 3 in e322270

  export const mainnetConfigs: ChainMap<ChainMetadata> = {

• hyperlane-deploy tells users to specify it in chains.ts

ISMConfig

Contains: 5)

Usage:

• default ISM configs are in SDK
• infra uses those for checking/deploying/governing
• hyperlane-deploy tells users to specify it in multisig_ism.ts

Agent config file

Contains: 1), 3), 7), 8)

Usage:

• Generated in hyperlane-deploy during contract deploy
• Used in agent configuration
  • (Should hyperlane-deploy include the RPC url in the agent config?)

Agent config ENV vars

Contains: 2), 11)

Usage:

• ENV vars for agent configuration

Hyperlane-deploy artifacts

Contains: 3), 4), 7), 9)

Usage:

• Generated in hyperlane-deploy during contract deploy
• Used for sending test messages
• (Should be used for warp route deployments)

Warp route config

Contains: 10), 3), 4), 7), 13)

Usage:

• Used for warp route deployment

Warp Route Template UI chains config

Contains: 1)

Usage:

• Used to configure the template UI chains (as json and not .ts like in hyperlane-deploy)

Warp Route Template UI tokens config

Contains: 12) + the token metadata + token logo

Usage:

• used to configure the template UI token list

Explorer PI config

Contains: 1), 3)

Usage:

• Used in explorer to check for messages on PI chains

[nambrot]

Questions:

• We generally have moved towards standardizing ChainMetadata (1) to be the "chain connection" config, the only odd ball here is the agent config. Can we have standard chain metadata for the most part?
  • Is it easy enough to specify overriden RPC urls?
  • Should index config 8) go into ChainMetadata as well?
• Should chain metadata have a common way of being extended with contracts for both agents and explorer config to have that standard as well. I.e. "anvil" => ({ ...chainMetadata, addresses: { mailbox: "0x" }})
  • Would it make it easier if hyperlane-deploy artifacts are keyed by that key (addresses) to copy and paste or even include the artifacts file straight?
• How should owner config 13) be handled? Should it just be assumed to be the deployer and have tooling that checks and governs ownership separately?
• Warp route deployments should use the hyperlane-deploy artificats when necessary instead of letting the user specify manually
• Warp route deployment should probably just spit out a tokenList directly (WarpRouteDeployer should output a tokenList json directly #2046)

[nambrot]

Here is an attempt to harmonize the agent config and existing SDK configs better so that existing chain metadata + contract artifacts can just be used out of the box. Would then also suggest for the explorer to use this. Looking for your feedback @mattiecnvr @jmrossy

   "chains": {
     "celo": {
+      // SDK chain metadata schema
       "name": "celo",
-      "domain": 42220,
-      "addresses": {
-        "mailbox": "0x35231d4c2D8B8ADcB5617A638A0c4548684c7C70",
-        "interchainGasPaymaster": "0x6cA0B6D22da47f091B7613223cD4BB03a2d77918",
-        "validatorAnnounce": "0x9bBdef63594D5FFc2f370Fe52115DdFFe97Bc524"
-      },
-      "protocol": "ethereum",
-      "finalityBlocks": 0,
+      "displayName": "Celo", // optional
+      "chainId": 42220,
+      "blocks": {
+        "reorgPeriod": 0, // the old finality blocks
+      },
+      "publicRpcUrls": [{
+        "http": "https://forno.celo.org"
+      }],
+
+      // SDK contract addresses schema
+      "mailbox": "0x35231d4c2D8B8ADcB5617A638A0c4548684c7C70",
+      "interchainGasPaymaster": "0x6cA0B6D22da47f091B7613223cD4BB03a2d77918",
+      "validatorAnnounce": "0x9bBdef63594D5FFc2f370Fe52115DdFFe97Bc524",
+
+
+      // agent specific
+      "protocol": "ethereum", // can be defaulted to ethereum
+      "rpcUrlAggregationType": "quorum", // use this to determine whether the public RPC urls is quorum or fallback or whether just the first entry should be used, and maybe default to fallback for relayer and quorum for validator
-      "connection": {
+      "connectionUrl": "https://forno.celo.org" // use this if there are no public RPC urls (probably override with ENV var)"
-        "type": "http"
-       },
+
+      // right now specific to the agent, but can be moved via https://github.com/hyperlane-xyz/hyperlane-monorepo/issues/2004#issuecomment-1497709408
       "index": {
         "from": 16884144

[mattiekat]

I don't understand the shape of publicRpcUrls, you have an external type tag but then seem to have a usless http tag in it? Maybe I am just missing something.

What is blocks and why is this better than having reog period/finalityBlocks outside it?

I guess I also don't understand connection and generally how that plays together with the public urls.

The publicRpcUrls format is also not useable with env vars so we would have to make it a JSON string due to the list. This is generally worth avoiding.

Otherwise I think it should be a config shape we can support, but it does seem to imply some other code changes and I am a little confused by some of the specifics.

[nambrot]

I don't understand the shape of publicRpcUrls, you have an external type tag but then seem to have a usless http tag in it? Maybe I am just missing something.

This is the current SDK chain metadata shape

hyperlane-monorepo/typescript/sdk/src/consts/chainMetadata.ts

Line 22 in 5b021c6

export interface ChainMetadata {

What do you mean by external type tag? Do you mean the publicRpcUrlsType key? So in SDK chain metadata, publicRpcUrls is just a list of rpc urls, so we need some kind configuration to tell the agent whether to use the list of rpc urls in quorum or fallback fashion, right? Renamed it to rpcUrlAggregationType

What is blocks and why is this better than having reog period/finalityBlocks outside it?

This is just what it is in sdk's chain metadata

hyperlane-monorepo/typescript/sdk/src/consts/chainMetadata.ts

Line 52 in 5b021c6

blocks?: {

I guess I also don't understand connection and generally how that plays together with the public urls.

The publicRpcUrls format is also not useable with env vars so we would have to make it a JSON string due to the list. This is generally worth avoiding.

The name publicRpcUrls is a bit unfortunate, and I could see us wanting to rename this to just rpcUrls in the SDK to make this less prescriptive on the public nature. Like you mentioned, this being an array of objects makes this a bit awkward, which is why I was thinking of the additional connection field (or maybe better phrased as agentRpcUrl) that is more conducive to ENV var specification. During config loading, the agent could first check the publicRpcUrls field and whether there is an array of rpc urls, if not, use the connection field to load rpc urls like we do right now (and using rpcUrlAggregationType when necessary.

Otherwise I think it should be a config shape we can support, but it does seem to imply some other code changes and I am a little confused by some of the specifics.

Any other confusion besides what you have specified above?

[asaj]

Be careful about making the chainId === domainId assumption..

[asaj]

Also curious, what do we get out of this? Are we expecting people to manually create these configs? If not, do we get anything other than simplification of the code that creates the agent config?

[nambrot]

Be careful about making the chainId === domainId assumption..

Good call out, the sdk chain metadata right now assumes domainId to be the same as chainID unless it's specified separately in the metadata

Also curious, what do we get out of this? Are we expecting people to manually create these configs? If not, do we get anything other than simplification of the code that creates the agent config?

We do expect folks to currently copy and paste the related config for the explorer. For the agents, i would say ideally most folks do not need to actually look at the file ideally (let along create). Though I also feel that it would be quite nice for folks to specify chain metadata only once and then be able to reuse things like the rpc urls in the agent config (admittedly a somewhat orthogonal point).

I do think the main benefit is the simplification of our config surface area in general.

[nambrot]

@mattiecnvr friendly ping on your opinion on whether the suggested config shape changes are easy to do

[mattiekat]

After discussing in person, there are just a couple name changes and clarifications I would like to see added, but otherwise I am in support of this.

1. we should make it more clear that the domain id defaults to the chain id but that it can be overridden for those special cases
2. as discussed displayName is currently going to be unused in the agents
3. we should pull in the changes for the validator finality to this set of changes (Finality blocks config #2091)
4. To default to "ethereum" for the protocol we agreed that it would be okay if the agents did not validate the string (so we can use #[serde(other)] to deserialize to that
5. publicRpcUrls
   5a) I would like to see this renamed to defaultRpcUrls
   5b) why an array of objects? We did make it clear this is fine because it would only be set in the json as part of the config we ship, but at least from what is presented above it feels like an array of urls would be sufficient. Are there any keys other than "http" that you are imagining?
   5c) I would like to see it clearly documented that if any connection url is set by the user these "defaults" will be ignored
6. rpcUrlAggregationType -> rpcConsensusType OR rpcCallType OR rpcCallStyle OR multiRpcCallStyle and so on... (I found aggregation to be confusing in this context and I don't like url in it).
7. connectionUrl
   7a) I don't like that this does not mirror the name of publicRpcUrls so following my above suggestion (5a) I would propose rpcUrls
   7b) We should make it clear it is a comma separated list of connection urls. (I assume it is this at least, if not that is another discussion around we will handle our case)

[jmrossy]

@mattiecnvr a few quick thoughts/responses:

1. Sure, any recs on how to make that more clear? There's a comment now but maybe not enough
2. displayName is currently optional
   5a. publicRpcUrls: I would also like to rename this! Though I'd prefer just rpcUrls instead of defaultRpcUrls
   5b. we do need the other fields (e.g. pagination) for smarter usage of RPC in a few places
   5c. Sorry I don't follow, can you clarify?

[nambrot]

@mattiecnvr @jmrossy Made a spec via this PR https://github.com/hyperlane-xyz/hyperlane-monorepo/pull/2203/files#diff-0dda34e51b45fcb259676791b216443495e465f6ec7c372fc77ec9abe9b8409aR90

Lmk if that matches what we have discussed here. If so, I would suggest to make two tickets:

1. for @jmrossy to make the necessary changes on the TS side Streamline config shapes (typescript) #2214
2. for @mattiecnvr to follow on the agent side

[mattiekat]

Made some comments on the PR, generally looking good to me