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

refactor(protocol): multiple improvements on implementation and tests #18483

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Nov 10, 2024

This is a large PR, with many changes aimed at adapting to recent updates. During your review, please focus primarily on storage and ABI upgradability aspects.

  • Introduced IResolver and DefaultResolver

    This update refactors the address resolution process by replacing IAddressManager, IAddressResolver, and AddressManager with a new interface, IResolver, and its implementation, DefaultResolver. Previously, EssentialContract relied on an addressManager contract for address resolution. Now, EssentialContract points to an IResolver instance directly. This update:

    • Removes the need for address caching, enabling a memory-based, pure-function Resolver to reduce gas costs.
    • Supports contract reconfiguration via EssentialContract.setResolver.
    • Eliminates MainnetXXX contracts, which previously handled caching, simplifying the structure.
  • Test Cleanup

    The test environment has been streamlined, with common logic refactored and dependencies on real provers minimized. The rollup protocol tests now have three main targets:

    • pnpm test:l1
    • pnpm test:l2
    • pnpm test:shared

    You can also run all tests with pnpm test.

  • Removed TierRouter

    Routing logic has been incorporated into the TierProvider implementation, allowing the deletion of TierRouter.

  • Deployment Scripts

    Many deployment scripts were designed for one-time use and are not intended for reuse. Redundant scripts were deleted, and others moved to script/layer1/mainnet and script/layer1/hekla. The GitHub workflow has been updated to ignore these scripts.

Todos

  • @dantaik script/layer1/DeployProtocolOnL1.s.sol: Working on rewriting this deployment script to improve accessibility for users less familiar with the protocol design. The goal is to make it interactive and/or support input files, with modular scripts for reuse.
  • @dantaik Additional test improvements needed before merging this PR.
  • @davidtaikochaTests in test/genesis/ were temporarily removed. Please reintroduce them using DefaultResolver (avoid using files from the script/ folder). Submit a separate PR.
  • @davidtaikocha Most modified scripts remain untested and may contain issues. Please verify functionality and address issues in separate PRs.
  • @smtmfft SP1-related tests are failing; please assist in resolving these. Run with FOUNDRY_PROFILE=layer1 forge test -vvv --mt test_sp1_.
  • Additional TODOs are noted in the code; please address them where possible.

Copy link

openzeppelin-code bot commented Nov 10, 2024

refactor(protocol): multiple improvements on implementation and tests

Generated at commit: baf6cfa3e904d36a070e8cd06223774fd761fbd8

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
3
0
8
42
56
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@dantaik dantaik added option.pinned Will not be marked as stale automatically option.do-not-merge labels Nov 10, 2024
@dantaik dantaik marked this pull request as ready for review November 10, 2024 15:17
@dantaik dantaik closed this Nov 14, 2024
@dantaik dantaik reopened this Nov 19, 2024
@smtmfft
Copy link
Contributor

smtmfft commented Nov 20, 2024

@smtmfft SP1-related tests are failing; please assist in resolving these. Run with FOUNDRY_PROFILE=layer1 forge test -vvv --mt test_sp1_.

That is because the tier verifier address changes. I will re-generate proof for the test soon, can ignore these 2 (any maybe risc0 has the same problem) for now as it is not a problem with the code logic.

@dantaik dantaik closed this Nov 22, 2024
@dantaik dantaik reopened this Nov 22, 2024
@dantaik dantaik marked this pull request as draft November 22, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area.protocol option.do-not-merge option.pinned Will not be marked as stale automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants