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

[SC-102] Add Arbitrum and Optimism forwarders and executors #1

Merged
merged 16 commits into from
Aug 25, 2023

Conversation

barrutko
Copy link
Contributor

@barrutko barrutko commented Aug 23, 2023

Proposed directory structure (updated):

  • src
    • dependencies
      • arbitrum
        • AddressAliasHelper.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum]
        • interfaces
          • IBridge.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum/interfaces]
          • IDelayedMessageProvider.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum/interfaces]
          • IGasRefunder.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum/libraries]
          • IInbox.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum/interfaces]
          • IOwnable.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum/interfaces]
          • ISequencerInbox.sol [governance-crosschain-bridges/contracts/dependencies/arbitrum/interfaces]
      • optimism
        • interfaces
          • ICrossDomainMessenger.sol [governance-crosschain-bridges/contracts/dependencies/optimism/interfaces]
          • IL2CrossDomainMessenger.sol [governance-crosschain-bridges/contracts/dependencies/optimism/interfaces]
    • executors
      • BridgeExecutorBase.sol [governance-crosschain-bridges/contracts/bridges]
      • L2BridgeExecutor.sol [governance-crosschain-bridges/contracts/bridges]
      • ArbitrumBridgeExecutor.sol [governance-crosschain-bridges/contracts/bridges]
      • OptimismBridgeExecutor.sol [governance-crosschain-bridges/contracts/bridges]
    • forwarders
      • CrosschainForwarderArbitrum.sol [aave-helpers/src/crosschainforwarders]
      • CrosschainForwarderOptimism.sol [aave-helpers/src/crosschainforwarders]
    • interfaces
      • IExecutorBase.sol [governance-crosschain-bridges/contracts//interfaces]
      • IL2BridgeExecutor.sol [governance-crosschain-bridges/contracts//interfaces]
      • IProposalGenericExecutor.sol [aave-helpers/src/interfaces]
  • tests
    • mocks
      • PayloadWithEmit.sol [aave-helpers/tests/mocks]
    • ArbitrumCrosschainForwarderTest.t.sol [aave-helpers/tests/crosschainforwarders]
    • OptimismCrosschainForwarderTest.t.sol [aave-helpers/tests/crosschainforwarders]

@linear
Copy link

linear bot commented Aug 23, 2023

SC-102 Create spark-gov-relay repo

  • ALso import governance L2 executor from governance-crosschain-bridges
  • Copy over Optimism, Arbitrum,
    • Add Gnosis Chain, Polygon zkEVM after review

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Before continuing I think this should be restructured slightly. Also there is missing newlines at EOF. You should configure Visual Studio Code or whatever your IDE is to add these automatically.

Proposed Adjustments:

  • Move BridgeExecutorBase and L2BridgeExecutor into executors dir.
  • Create two directories in dependencies called optimism and arbitrum to put all the chain-specific dependencies (including interfaces)
  • Move interfaces/(arbitrum|optimism) into dependencies/(arbitrum|optimism)/interfaces
  • Move interfaces/executors up one directory to interfaces
  • Move AddressAliasHelper into dependencies/arbitrum as it's specific to Arbitrum.

.gitmodules Show resolved Hide resolved
@barrutko barrutko requested a review from hexonaut August 24, 2023 18:45
Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Minor changes, all code matches source repos. After they are addressed this looks good for merging. I don't care too much about the state of the tests rn as they should be rewritten to be generic and use xchain-helpers instead of manually doing the message relaying in the e2e tests BGD built.

src/dependencies/arbitrum/interfaces/IGasRefunder.sol Outdated Show resolved Hide resolved
src/interfaces/IProposalGenericExecutor.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lucas-manuel lucas-manuel left a comment

Choose a reason for hiding this comment

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

LGTM, lets merge as is, going to make another ticket to investigate how to structure GovHelpers cause it looks like its needed in both spells and here

@barrutko barrutko merged commit aa074bf into master Aug 25, 2023
@barrutko barrutko deleted the SC-102-setup-xchain-infra branch August 25, 2023 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants