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

evm: add WormholeCctpTokenMessenger abstract and optimize CircleIntegration #48

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

a5-pickle
Copy link
Contributor

@a5-pickle a5-pickle commented Dec 20, 2023

PR Summary

  • Add WormholeCctpTokenMessenger abstract contract for integrators that do not want to compose with CircleIntegration
  • Leverage WormholeCctpTokenMessenger for optimized CircleIntegration
  • Add various helper libraries
  • Add forge tests

CircleIntegration Improvements

  Fork Test Latest Test Control Test   Fork Adjusted Latest Adjusted Improvement
Inbound 278,944 255,611 73,885   205,059 181,726 23,333
Outbound 365,874 357,024 207,775   158,099 149,249 8,850

Inheriting WormholeCctp vs Composing with CircleIntegration

  Composed Test Inherited Test   Composed Adjusted Inherited Adjusted Inherited Benefit
Inbound 271,973 230,418   198,088 156,533 41,555
Outbound 389,673 341,201   181,898 133,426 48,472

Run make gas-report to generate above values. There is slight variance in computed gas numbers due to fuzzed inputs.

NOTE: Integration tests are removed in this PR. These will be added back in when we add another network like Solana.

@a5-pickle a5-pickle force-pushed the evm/optimize branch 2 times, most recently from 0c80c47 to 8095770 Compare December 21, 2023 21:46
evm/src/contracts/CircleIntegration/Governance.sol Outdated Show resolved Hide resolved
evm/src/contracts/CircleIntegration/Logic.sol Show resolved Hide resolved
evm/src/contracts/WormholeCctp.sol Outdated Show resolved Hide resolved
evm/src/libraries/WormholeCctpMessages.sol Outdated Show resolved Hide resolved
evm/src/contracts/CircleIntegration/Logic.sol Show resolved Hide resolved
@djb15
Copy link
Collaborator

djb15 commented Dec 22, 2023

Just to confirm, the reasoning for the storage slot layout is because this is going to be an upgrade of an existing deployment?

@a5-pickle
Copy link
Contributor Author

Just to confirm, the reasoning for the storage slot layout is because this is going to be an upgrade of an existing deployment?

Yes exactly. Before the contract laid out the storage without specifying specific slots. But now that I moved some of these too immutables, I did not want the references to unused slots. I guess I could call the unused ones "gap". Which is preferred?

@djb15
Copy link
Collaborator

djb15 commented Dec 22, 2023

Just to confirm, the reasoning for the storage slot layout is because this is going to be an upgrade of an existing deployment?

Yes exactly. Before the contract laid out the storage without specifying specific slots. But now that I moved some of these too immutables, I did not want the references to unused slots. I guess I could call the unused ones "gap". Which is preferred?

No I'm happy to keep that as is. I think it's good to be clear that these slots were used before. Although I'd suggest wiping those storage slots to 0 in the initialiser of the new deployment just in case. And then on the next upgrade (the one after this) we could use those slots again. And then add sufficient docstrings/comments to explain what has happened with those slots, are they safe, etc. Happy to diff this change if you would like?

@a5-pickle a5-pickle requested review from djb15 and gator-boi January 2, 2024 23:06
@a5-pickle a5-pickle force-pushed the evm/optimize branch 3 times, most recently from 2e9cc44 to 5ca4dbd Compare January 3, 2024 17:54
@a5-pickle a5-pickle changed the title evm: add WormholeCctp abstract and optimize CircleIntegration evm: add WormholeCctpTokenMessenger abstract and optimize CircleIntegration Jan 11, 2024
* add WormholeCctpTokenMessenger

* inherit above in CircleIntegration

* remove updateWormholeFinality (gov)

* reorganize

* add more forge tests

* remove integration tests

* add gas snapshot
@a5-pickle a5-pickle force-pushed the evm/optimize branch 2 times, most recently from 6f93f93 to f40b4d0 Compare January 15, 2024 15:10
djb15 and others added 3 commits January 16, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants