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

feat: passthrough L1->L3 adapter to send messages to L3 via an L2 forwarder #607

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

bmzig
Copy link
Contributor

@bmzig bmzig commented Sep 16, 2024

The L3 adapter to send messages to Arbitrum-like L3 spoke pools should piggyback off of our existing adapters to first relay tokens/messages to an intermediate contract on L2. These L2 forwarders should then have logic to continue the token bridging or to execute stuck messages.

Signed-off-by: bennett <bennett@umaproject.org>
@bmzig bmzig changed the title feat: add L3 adapter to send messages to an L2 forwarder feat: add a passthrough L3 adapter to send messages to an L2 forwarder Sep 17, 2024
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Looks great -- just one comment

* spoke pool address is stored by the L2 forwarder and the L2 forwarder address is stored in this contract.
* @param message Data to send to target.
*/
function relayMessage(address, bytes memory message) external payable override {
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC, why did you decide to do this via delegatecall rather than inheritance? To avoid having to have a different contract for an L2 with custom gas vs non custom gas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a delegatecall so this can work with any L2 by piggybacking off of the adapter logic. Naming is admittedly poor here, but the idea was for this (L3) adapter to send messages and tokens to L2 using the exact same logic as the corresponding L2 adapter (e.g. an Arbitrum_*_Adapter, Optimism_Adapter, ZkSync_Adapter, etc) and overwrite the target (since the hub pool will specify the target as the L3 spoke pool due to setCrossChainContracts, which doesn't exist on L2). If I did inheritance, then we would need to make an adapter which inherits from the L2 adapters based on the architecture of the L2 -- something like Arbitrum_L3_Adapter is Arbitrum_CustomGasToken_Adapter or Arbitrum_L3_Adapter is Optimism_Adapter, whereas when we delegatecall, we can keep the code exactly the same and just change constructor arguments.

*/

// solhint-disable-next-line contract-name-camelcase
contract Arbitrum_L3_Adapter is AdapterInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this contract is specific to arbitrum, right? It's really just an adapter for any forwarder, no?

Copy link
Contributor Author

@bmzig bmzig Sep 19, 2024

Choose a reason for hiding this comment

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

This is likely due to my poor naming, but I did this to emphasize that the L3 is Arbitrum-like. There is no assumption about the structure of the L2.

Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is still a sticking point for me. I think it makes sense to spend some more time thinking about this. This contract actually functions more like a (re-)router I guess, but maybe that's just my networking bias.

Copy link
Member

Choose a reason for hiding this comment

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

I agree this naming is not useful right now. Because it has "L3" in it its non-intuitive that its actually meant to be deployed on L1. Let's also remove the Arbitrum mention from the name because its clearly not specific to Arbitrum. Please change all the comments also to make it clear this can be used generically.

I think a rerouter in the name could work, and clearly to be consistent with other L1 contracts it should end with Adapter. What about RerouterAdapter?

@pxrl

Copy link
Contributor Author

@bmzig bmzig Sep 24, 2024

Choose a reason for hiding this comment

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

Optimistically changing it to RerouterAdapter and updating the comments to emphasize that it can be used in other contexts 1e984cc

@bmzig bmzig marked this pull request as ready for review September 20, 2024 16:53
Signed-off-by: bennett <bennett@umaproject.org>
Signed-off-by: bennett <bennett@umaproject.org>
Signed-off-by: bennett <bennett@umaproject.org>
@nicholaspai nicholaspai changed the title feat: add a passthrough L3 adapter to send messages to an L2 forwarder feat: passthrough L1->L3 adapter to send messages to an L2 forwarder Sep 24, 2024
@nicholaspai nicholaspai changed the title feat: passthrough L1->L3 adapter to send messages to an L2 forwarder feat: passthrough L1->L3 adapter to send messages to L3 via an L2 forwarder Sep 24, 2024
Signed-off-by: bennett <bennett@umaproject.org>
Signed-off-by: bennett <bennett@umaproject.org>
Signed-off-by: bennett <bennett@umaproject.org>
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

I think the code looks great, but the comments need a lot of cleaning up

Signed-off-by: bennett <bennett@umaproject.org>
Signed-off-by: bennett <bennett@umaproject.org>
Signed-off-by: bennett <bennett@umaproject.org>
* @param message Data to send to target.
*/
function relayMessage(address target, bytes memory message) external payable override {
bytes memory updatedMessage = abi.encodeCall(AdapterInterface.relayMessage, (target, message));
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some comments explaining how this double relayMessage encoding works? Think its a bit confusing and assumes knowledge of the l2Target's interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this d87d09c. It definitely assumes knowledge of the l2Target's interface.

Signed-off-by: bennett <bennett@umaproject.org>
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