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.

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

Configure gas overhead IGP #1588

Merged

Conversation

yorhodes
Copy link
Member

@yorhodes yorhodes commented Jan 13, 2023

Description

  • Instruments multisigIsm.verify gas overhead for various permutations of threshold and validator set size
  • Add deployment tooling for IGP gas overhead

Drive-by

  • Update GasOverheadIgp to use immutable IGP

Related issues

Backward compatibility

Yes

Testing

Unit Tests

solidity/test/isms/multisigIsm.test.ts Outdated Show resolved Hide resolved
solidity/contracts/isms/MultisigIsm.sol Outdated Show resolved Hide resolved
);
const interchainGasOverhead = await this.deployProxiedContract(
chain,
'interchainGasOverhead',
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't @tkporter want to have the overhead contract to be at the existing IGP address?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, imo this would be the smoothest transition for users

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also wonder if we should rename the deployed overhead contract to be something like "multisigIsmIgp" and then to have the base / inner IGP be called something like "baseIgp". So that it's very clear what the use of each IGP is

solidity/contracts/igps/InterchainGasPaymaster.sol Outdated Show resolved Hide resolved
typescript/sdk/src/deploy/core/HyperlaneCoreDeployer.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@tkporter tkporter left a comment

Choose a reason for hiding this comment

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

great job 🙏 really neat to see the gas benchmarks too

I'll bring this up in parking lot but I'm wondering if we should consider mailbox costs in the multisig ISM IGP or if that should live in the base IGP. If it lives in the base IGP then it makes composing IGPs much easier, but it also increases gas / complexity

solidity/contracts/igps/GasOverheadIgp.sol Show resolved Hide resolved
solidity/contracts/isms/MultisigIsm.sol Outdated Show resolved Hide resolved
solidity/contracts/isms/MultisigIsm.sol Outdated Show resolved Hide resolved
solidity/contracts/isms/MultisigIsm.sol Outdated Show resolved Hide resolved
solidity/test/igps/GasOverheadIgp.t.sol Outdated Show resolved Hide resolved
);
const interchainGasOverhead = await this.deployProxiedContract(
chain,
'interchainGasOverhead',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, imo this would be the smoothest transition for users

solidity/contracts/isms/MultisigIsm.sol Outdated Show resolved Hide resolved
solidity/test/isms/multisigIsm.test.ts Outdated Show resolved Hide resolved
);
const interchainGasOverhead = await this.deployProxiedContract(
chain,
'interchainGasOverhead',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also wonder if we should rename the deployed overhead contract to be something like "multisigIsmIgp" and then to have the base / inner IGP be called something like "baseIgp". So that it's very clear what the use of each IGP is

@yorhodes yorhodes force-pushed the yorhodes/multisig-ism-verify-costs branch from 7c4c169 to a3a3f54 Compare January 13, 2023 18:50
@yorhodes yorhodes merged commit 5546465 into trevor/on-chain-fee-ism-igp Jan 18, 2023
@yorhodes yorhodes deleted the yorhodes/multisig-ism-verify-costs branch January 18, 2023 16:38
tkporter added a commit that referenced this pull request Jan 24, 2023
* IGP charges 1 wei

* Don't pay for gas in LiquidityLayerRouter

* yarn gas

* Add GasOverheadIgp

* Remove compiler silencing

* yarn gas

* Shift around some fns

* nit var name

* nit

* Configure gas overhead IGP (#1588)

* Instrument multisigIsm.verify gas costs

* Add deployment tooling for IGP gas overhead

* Include mailbox.process and max body in overhead

* Restore multisig tests

* GasOverheadIgp -> OverheadIgp, fix infra build

* Prettier, change some infra / deploy tooling

* run yarn hyperlane

* yarn gas

* yarn gas, update test contracts

* rm OwnableUpgradeable for Overhead ISM

* Move gas instrumentation into its own describe

* back to TestRecipient__factory for normal verify tests

* New submodule commits

* deploy test

* BaseInterchainGasPaymaster -> InterchainGasPaymaster

* trying to deploy

* Deploying with deployerOwnedProxyAdmin

* Deployed to testnet3

* Cleaning up

* cleaning up

* clean

* Fix tests

* revert infra/package.json

* clean up verification.json

* Update yarn.lock

* Fix lint

* Changing logic around deployerOwnedProxyAdmin

* Deploy mainnet

* Redeploy testnet with new salt versions

* nit

Co-authored-by: Yorke Rhodes <yorke@hyperlane.xyz>
tkporter pushed a commit that referenced this pull request Jan 24, 2023
* Instrument multisigIsm.verify gas costs

* Add deployment tooling for IGP gas overhead

* Include mailbox.process and max body in overhead

* Restore multisig tests
tkporter added a commit that referenced this pull request Jan 24, 2023
* IGP charges 1 wei

* Don't pay for gas in LiquidityLayerRouter

* Add GasOverheadIgp

* Remove compiler silencing

* Shift around some fns

* nit var name

* nit

* Configure gas overhead IGP (#1588)

* Instrument multisigIsm.verify gas costs

* Add deployment tooling for IGP gas overhead

* Include mailbox.process and max body in overhead

* Restore multisig tests

* GasOverheadIgp -> OverheadIgp, fix infra build

* Prettier, change some infra / deploy tooling

* run yarn hyperlane

* yarn gas, update test contracts

* rm OwnableUpgradeable for Overhead ISM

* Move gas instrumentation into its own describe

* back to TestRecipient__factory for normal verify tests

* deploy test

* BaseInterchainGasPaymaster -> InterchainGasPaymaster

* trying to deploy

* Deploying with deployerOwnedProxyAdmin

* Deployed to testnet3

* Cleaning up

* cleaning up

* clean

* everything

* Clean up, remove more coingecko stuff

* Use actual quoted value in kathy

* Fix yarn lint-ts

* Fix flaky test

* yarn gas

* Add back gas payment estimates

* fix test

* reset src/index.ts

* revert infra/package.json

* clean up verification.json

* Update yarn.lock

* Fix build

* update kathy, some nits

* Rebase had some incorrect code; fixed that

* ran yarn hyperlane

* update test

* reset test_config.json

Co-authored-by: Yorke Rhodes <yorke@hyperlane.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants