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

Add arbitrum specific gateway contracts #7

Merged
merged 11 commits into from
Jan 20, 2022
Merged

Add arbitrum specific gateway contracts #7

merged 11 commits into from
Jan 20, 2022

Conversation

shalzz
Copy link
Contributor

@shalzz shalzz commented Dec 10, 2021

This is the native bridge implementation.

Some unsolved problems:

  • Transferring ETH for paying the arbitrationCost after bridging:
    1.) This is inherently more complex than just state bridging (involves a bunch of error handling and refund handling which is very bridge specific and may not be refund claimable from non-EOA accounts)
    2.) This is especially true for the xdai bridge since eth<->xdai transfer is not intuitive

  • Calculation of the arbitration cost in relation to the numOfJurors:
    1.) The same logic for this from the KlerosCore would need to be re-implemented here.
    2.) This would most likely require a governance function that updates the jurorFees per court on the gateways whenever there is a governance proposal to so on the KlerosCore contract as well.
    For now, the arbitration cost is just a constant i.e it does not scale with the numOfJurors.

@shalzz shalzz force-pushed the feature/gateway branch 2 times, most recently from b99df7e to 2698575 Compare December 27, 2021 13:14
Note they will always revert right now as the actual sender of
the cross domain message will be the wrapper bridge contracts.
We need to either have the bridge contracts be inherited or
pass their address to the constructor of the other bridge in
addition to the address of the gateway.
@shalzz shalzz marked this pull request as ready for review January 5, 2022 09:55
@jaybuidl
Copy link
Member

jaybuidl commented Jan 12, 2022

Shaleen there is no need for such complex inheritance, in particular diamond inheritance is generally an anti-pattern which may yield unexpected results due to the C3 linearization (a call to a super can be substituted by a call to a sibling).

Favor composition over inheritance: BaseXGateway does not need to inherit from ILXBridge, from an OO point of view, a gateway has a bridge seems more correct than a gateway is a bridge. So ILXBridge can just be a contract variable passed during initialization of BaseXGateway.

@shalzz
Copy link
Contributor Author

shalzz commented Jan 12, 2022

Well, this is not diamond inheritance

@jaybuidl
Copy link
Member

        IL1Bridge
        /       \
ArbL1Bridge   BaseForeignGateway
        \       /
     EthereumGateway

Am I missing anything?

@shalzz
Copy link
Contributor Author

shalzz commented Jan 12, 2022

        IL1Bridge
        /       \
ArbL1Bridge   BaseForeignGateway
        \       /
     EthereumGateway

Am I missing anything?

IL1Bridge is an interface. So I wouldn't really count that as an inherited contract.
BaseForeignGateway is an abstract contract that need a IL1Bridge implementation. ArbL1Bridge provides that implementation.

@jaybuidl
Copy link
Member

Right IL1Bridge is an interface. I still think that it can simplified bearing in mind that a Gateway is not a Bridge, it breaks the abstraction of the bridge which the gateway provides. To me BaseForeignGateway has a IL1Bridge. In this case the gateway would still need to expose the getSubmissionPrice() function but it can be arranged.

That would give:

 IL1Bridge ⬥-------- BaseForeignGateway
     ▲                       ▲
     |                       |
ArbL1Bridge           EthereumGateway

@shalzz
Copy link
Contributor Author

shalzz commented Jan 12, 2022

The implementation would definitely change while solving the open problems I mentioned above which are bigger issues IMO.

@codeclimate
Copy link

codeclimate bot commented Jan 18, 2022

Code Climate has analyzed commit 26d4641 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@jaybuidl jaybuidl left a comment

Choose a reason for hiding this comment

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

Not feature complete but good enough to integrate and test the existing features.

@jaybuidl jaybuidl merged commit 4e13c91 into master Jan 20, 2022
@jaybuidl jaybuidl deleted the feature/gateway branch January 20, 2022 15:19
@jaybuidl jaybuidl added this to the prealpha-1 milestone Jan 20, 2022
Params10 pushed a commit that referenced this pull request Feb 3, 2023
Add arbitrum specific gateway contracts
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.

2 participants