feat:update `StorageGasOracle` in `igpDeployer` #3101

aroralanuk · 2023-12-23T02:18:48Z

Description

CLI
- add oracleConfig and overhead to hooks config
- asking user for overhead, tokenExchangeRate (optional - use 1e10 as default), gasPrice (optional - use provider.getPrice() as default)
- parsing IGPConfig (ZOD) to IGPConfig (SDK)
IGPDeployer
- if onchain storage oracle config doesn't match the desired oracleConfig (inside igpConfig)
- getStorageGasOracleConfigs - fetches nested storage gas oracle from within the coreConfig (e.g. core -> defaultHook -> routingHook -> igp -> oracleConfig)
- added hardhat test for modifying oracle config
refactoring and pulling building configs from infra to SDK to make it independent of environment specific chains and state
- createIgpConfig
- buildMultisigIsmConfigs, buildAggregationIsmConfigs, etc
- buildRoutingOverAggregationIsmConfig routing over aggregation of msgId and merkleTree (current deployment)
test configs: infra -> sdk (for configureGasOracle.hardhat-test.ts)
remove igp deployments (will go through the core deployment)
remove update-storage-igp-oracle (same functionality by running the igp deployer or directly call deployer.configureStorageGasOracle)

Note: because of the commonJS issues with the cli package, I can't import {ethers} from hardhat which means i cannot test getGasPrice() in the CLI.

Drive-by changes

passing in all chains as a batch to TokenPriceGetter as sending chain by chain takes too long because of CoinGecko's 5s rest between API calls.

Related issues

fixes https://github.com/hyperlane-xyz/issues/issues/820
partially fixes Functional config generation in Infra #3064

Backward compatibility

Yes, refactoring and config changes

Testing

Hardhat/manual

changeset-bot · 2023-12-23T02:18:52Z

🦋 Changeset detected

Latest commit: 474bc96

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@hyperlane-xyz/infra	Minor
@hyperlane-xyz/cli	Minor
@hyperlane-xyz/sdk	Minor
@hyperlane-xyz/helloworld	Minor
@hyperlane-xyz/utils	Minor
@hyperlane-xyz/core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

yorhodes · 2024-01-02T16:36:41Z

will review once build is fixed

codecov · 2024-01-05T23:51:19Z

Codecov Report

Merging #3101 (c400ec9) into main (6044fb8) will decrease coverage by 67.52%.
Report is 131 commits behind head on main.
The diff coverage is n/a.

❗ Current head c400ec9 differs from pull request most recent head 474bc96. Consider uploading reports for the commit 474bc96 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #3101       +/-   ##
==========================================
- Coverage   67.51%   0.00%   -67.52%     
==========================================
  Files         101       1      -100     
  Lines        1022      16     -1006     
  Branches      106       0      -106     
==========================================
- Hits          690       0      -690     
+ Misses        292      16      -276     
+ Partials       40       0       -40

Components	Coverage Δ
core	`∅ <ø> (∅)`
hooks	`∅ <ø> (∅)`
isms	`∅ <ø> (∅)`
token	`∅ <ø> (∅)`
middlewares	`∅ <ø> (∅)`

solidity/CHANGELOG.md

typescript/cli/package.json

typescript/cli/src/config/hooks.ts

typescript/sdk/src/gas/HyperlaneIgpDeployer.ts

yorhodes · 2024-01-08T17:24:15Z

typescript/sdk/src/gas/oracle/utils.ts

+export function prettyRemoteGasDataConfig(
+  chain: ChainName,
+  config: RemoteGasData,
+): string {
+  return `\tRemote: (${chain})\n${prettyRemoteGasData(config)}`;
+}
+
+export function prettyRemoteGasData(data: RemoteGasData): string {
+  return `\tToken exchange rate: ${prettyTokenExchangeRate(
+    data.tokenExchangeRate,
+  )}\n\tGas price: ${data.gasPrice.toString()} (${ethers.utils.formatUnits(
+    data.gasPrice,
+    'gwei',
+  )} gwei)`;
+}


is gwei always the gas unit?
do we need to account for decimals here?

yorhodes · 2024-01-08T17:25:54Z

typescript/sdk/src/gas/token-prices.ts

+  getTokenExchangeRate(
+    base: ChainName,
+    quote: ChainName,
+    cacheThemAll: ChainName[],


this is technically a breaking change
the variable name is also unclear

is it chainsToCache or does it mean something else?

can you make this optional? I would have no idea from the name how to call getTokenPrice after this change

yorhodes · 2024-01-08T17:26:15Z

typescript/sdk/src/gas/token-prices.ts

-    const toQuery = chains.filter((c) => !this.cache.isFresh(c));
+    const toQuery = cacheThemAll.filter((c) => !this.cache.isFresh(c));


I dont understand this change

coingecko api takes it a list of tickers and the TokenPriceCache caches them. But earlier, we used to cache pairwise for origin and destination which is much slower as we're waiting 5s between every coingecko api, this expedites it by making an api call with all the tickers the first time.

yorhodes · 2024-01-08T17:27:51Z

typescript/sdk/src/ism/multisig.ts

this is another breaking change
why not reuse these?

what is breaking, you just commented on the whole file?

jmrossy · 2024-01-09T19:46:32Z

.changeset/loud-chairs-watch.md

+  - `createIgpConfig`
+  - `buildMultisigIsmConfigs`, `buildAggregationIsmConfigs`, etc
+  - `buildRoutingOverAggregationIsmConfig` routing over aggregation of msgId and merkleTree (_current deployement_)
+- test configs: infra -> sdk (for configureGasOracle.hardhat-test.ts)


Infra and/or test related changes don't need to be noted in the changeset

.changeset/loud-chairs-watch.md

typescript/cli/src/config/hooks.ts

jmrossy · 2024-01-09T19:52:32Z

typescript/cli/src/config/hooks.ts

+  hookConfig: HookConfig,
+): Promise<any> {
+  if (hookConfig.type === HookType.INTERCHAIN_GAS_PAYMASTER) {
+    return await processIgpConfig(multiProvider, hookConfig as IgpConfig);


The only reason to return await instead of just return is if you explicitly want to force errors to throw here instead of above, which isn't the case since you aren't using a try/catch in this function

typescript/cli/src/config/hooks.ts

jmrossy · 2024-01-09T20:13:12Z

typescript/sdk/src/config/test/chains.ts

+export type TestChains = keyof typeof testConfigs;
+export const testChainNames = Object.keys(testConfigs) as TestChains[];


Duplicate of existing code in typescript/sdk/src/consts/chains.ts

jmrossy · 2024-01-09T20:14:40Z

typescript/sdk/src/config/test/multisigIsm.ts

+  test1: '0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65',
+  test2: '0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc',
+  test3: '0x976EA74026E726554dB657fA54763abd0C3a0aa9',


Let's not start splitting up consts across the src/consts/ and src/config directories please. Can you find a home for these in consts?

typescript/sdk/src/gas/HyperlaneIgpDeployer.ts

jmrossy · 2024-01-09T20:20:27Z

typescript/sdk/src/gas/token-prices.ts

+  getTokenExchangeRate(
+    base: ChainName,
+    quote: ChainName,
+    cacheThemAll: ChainName[],


is it chainsToCache or does it mean something else?

yorhodes · 2024-01-17T20:23:02Z

typescript/sdk/src/ism/multisig.ts

why remove this? another breaking change

yorhodes · 2024-01-17T20:23:45Z

typescript/sdk/src/gas/types.ts

  oracleKey: Address;
-  overhead: ChainMap<number>;
+  overhead: ChainMap<BigNumber>;


why change this? imo we want this to be as serializable as possible

yorhodes · 2024-01-17T20:23:54Z

typescript/sdk/src/gas/types.ts

-  gasOracleType: ChainMap<GasOracleContractType>;
+  oracleConfig: StorageGasOraclesConfig;


why change this again? this is another breaking change

.changeset/loud-chairs-watch.md

yorhodes · 2024-01-17T20:24:13Z

.changeset/loud-chairs-watch.md

+  - `createIgpConfig`
+  - `buildMultisigIsmConfigs`, `buildAggregationIsmConfigs`, etc
+  - `buildRoutingOverAggregationIsmConfig` routing over aggregation of msgId and merkleTree (_current deployement_)
+- test configs: infra -> sdk (for configureGasOracle.hardhat-test.ts)


yorhodes · 2024-01-17T22:25:55Z

typescript/sdk/src/gas/HyperlaneIgpDeployer.ts

+    const configsToSet: Record<
+      Address,
+      StorageGasOracle.RemoteGasDataConfigStruct[]
+    > = {};
+
+    for (const remote of remotes) {


can this be a map instead?

yorhodes · 2024-01-17T22:26:09Z

typescript/sdk/src/gas/HyperlaneIgpDeployer.ts

+    for (const remote of remotes) {
+      const desiredGasData = gasOracleConfig[remote];
+      if (desiredGasData.type !== GasOracleContractType.StorageGasOracle) {
+        continue;


should we throw in this case?

yorhodes · 2024-01-17T22:26:45Z

typescript/sdk/src/gas/HyperlaneIgpDeployer.ts

+        .gasOracle;
+      const gasOracle = StorageGasOracle__factory.connect(
+        gasOracleAddress,
+        this.multiProvider.getSigner(chain),


this can be signer instead of provider

yorhodes · 2024-01-17T22:31:35Z

typescript/sdk/src/gas/token-prices.ts

+  getTokenExchangeRate(
+    base: ChainName,
+    quote: ChainName,
+    cacheThemAll: ChainName[],


can you make this optional? I would have no idea from the name how to call getTokenPrice after this change

yorhodes · 2024-01-17T22:31:46Z

typescript/sdk/src/gas/token-prices.ts

-    const [price] = await this.getTokenPrices([chain]);
+  async getTokenPrice(
+    chain: ChainName,
+    cacheThemAll: ChainName[],


ditto, can this be optional?

### Description HyperlaneIGPDeployer supports configuring storageGasOracle if provided. HyperlaneIgpDeployer takes in `IgpConfig & Partial<OracleConfig>` Note: storageGasOracle cannot be configured in CLI yet. Note: this is a more minimal version of #3101 where more breaking changes were made. ### Drive-by changes None ### Related issues - fixes hyperlane-xyz/issues#820 ### Backward compatibility Yes ### Testing Unit

### Description HyperlaneIGPDeployer supports configuring storageGasOracle if provided. HyperlaneIgpDeployer takes in `IgpConfig & Partial<OracleConfig>` Note: storageGasOracle cannot be configured in CLI yet. Note: this is a more minimal version of hyperlane-xyz#3101 where more breaking changes were made. ### Drive-by changes None ### Related issues - fixes hyperlane-xyz/issues#820 ### Backward compatibility Yes ### Testing Unit

sdk changes

90959ae

aroralanuk requested review from yorhodes and jmrossy as code owners December 23, 2023 02:18

changes to hook

014d1c7

yorhodes marked this pull request as draft January 2, 2024 16:36

aroralanuk added 9 commits January 2, 2024 15:26

infra consolidation

71b06d9

add signer

7ab0c25

refactor

432cac2

refactor infra pt 1

f2d165a

update comparing token exchange rates

7ba0392

Merge branch 'main' into kunal/update-gasOracle-deployer

f5a0eb3

dedupe aggregationConfig build

ad66569

getPrice if not provided

b347234

changeset and stronger typing on hookconfig

c400ec9

aroralanuk marked this pull request as ready for review January 5, 2024 23:47

aroralanuk requested review from tkporter, nambrot, Skunkchain and avious00 as code owners January 5, 2024 23:47

aroralanuk changed the title ~~[WIP]feat:update StorageGasOracle in igpDeployer~~ feat:update StorageGasOracle in igpDeployer Jan 5, 2024

jmrossy reviewed Jan 6, 2024

View reviewed changes

solidity/CHANGELOG.md Outdated Show resolved Hide resolved

aroralanuk added 2 commits January 6, 2024 00:32

revert release

f9508d0

revert release 2

2dd75d1

yorhodes requested changes Jan 8, 2024

View reviewed changes

pr comment changes

99c35f6

aroralanuk requested a review from yorhodes January 9, 2024 18:46

nested igp processing

6fd0408

jmrossy reviewed Jan 9, 2024

View reviewed changes

yorhodes reviewed Jan 17, 2024

View reviewed changes

aroralanuk added 2 commits January 24, 2024 15:33

async preset

e5ce12e

pr remediations

474bc96

aroralanuk closed this Feb 2, 2024

aroralanuk mentioned this pull request Feb 3, 2024

feat: configure GasOracle in HyperlaneIgpDeployer #3218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:update `StorageGasOracle` in `igpDeployer` #3101

feat:update `StorageGasOracle` in `igpDeployer` #3101

aroralanuk commented Dec 23, 2023 •

edited

Loading

changeset-bot bot commented Dec 23, 2023 •

edited

Loading

yorhodes commented Jan 2, 2024

codecov bot commented Jan 5, 2024 •

edited

Loading

yorhodes Jan 8, 2024

yorhodes Jan 8, 2024

jmrossy Jan 9, 2024

yorhodes Jan 17, 2024

yorhodes Jan 8, 2024

aroralanuk Jan 9, 2024

yorhodes Jan 8, 2024

aroralanuk Jan 9, 2024

jmrossy Jan 9, 2024

yorhodes Jan 17, 2024

jmrossy Jan 9, 2024

jmrossy Jan 9, 2024

jmrossy Jan 9, 2024

yorhodes Jan 17, 2024

jmrossy Jan 9, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

yorhodes Jan 17, 2024

		const toQuery = chains.filter((c) => !this.cache.isFresh(c));
		const toQuery = cacheThemAll.filter((c) => !this.cache.isFresh(c));

		export type TestChains = keyof typeof testConfigs;
		export const testChainNames = Object.keys(testConfigs) as TestChains[];

		gasOracleType: ChainMap<GasOracleContractType>;
		oracleConfig: StorageGasOraclesConfig;

feat:update StorageGasOracle in igpDeployer #3101

feat:update StorageGasOracle in igpDeployer #3101

Conversation

aroralanuk commented Dec 23, 2023 • edited Loading

Description

Drive-by changes

Related issues

Backward compatibility

Testing

changeset-bot bot commented Dec 23, 2023 • edited Loading

🦋 Changeset detected

yorhodes commented Jan 2, 2024

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

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

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

feat:update `StorageGasOracle` in `igpDeployer` #3101

feat:update `StorageGasOracle` in `igpDeployer` #3101

aroralanuk commented Dec 23, 2023 •

edited

Loading

changeset-bot bot commented Dec 23, 2023 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading