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: promise callback library #216

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

Conversation

hamdiallam
Copy link
Contributor

Description

By adding the return data to the RelayedMessage event, a callback library can be introduced allowing users to attach continuations dependent on the return. This makes any remote view function callable with low latency via superchain interop.

@hamdiallam hamdiallam changed the title Promise Callback Library feat: promise callback library Mar 18, 2025
@hamdiallam hamdiallam marked this pull request as ready for review March 18, 2025 16:46
@tynes
Copy link
Contributor

tynes commented Mar 19, 2025

I don't think any amount of design review will be able to give a thumbs up on this design better than working with teams to build interop into their applications. I recommend that we focus on getting the entrypoint primitive into the L2ToL2CrossDomainMessenger and then build this on top of that to see what design pattern works the best. With the entrypoint, you can iterate on this design without needing to make any additional changes to the core contracts


# Alternatives Considered

## Callback Entrypoint
Copy link
Contributor

Choose a reason for hiding this comment

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

The entrypoint solution and this proposal are not mutually exclusive though, right? You are simply proposing that the CDM emit return data that can be used by other contracts if I understand correctly. Entrypoints would still have a place but wouldn't be the preferred way to address this use case?

- Tight coupling between the source & destination chains. Callback information must be propogated between networks
- Multiple callbacks cant clearly be registered. Must be enshrined in the propogated callback data.
- Single interface for relayers `relayMessage`.
- The proposed solution could be implemented via a generic entrypoint to avoid adding the return data to the event. However this feels like more complexity from a code & call path perspective. Propogate callback -> parse callback -> relay -> capture return and sendMessage(). Versus -- RelayedMessage() -> continue().
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @tynes! Just wanted to bring your attention to this point from Hamdi - relying on entrypoints for callbacks seems like it would increase complexity for the app dev significantly, don't you think?

This proposal comes from feedback at ETH Denver around how app devs interact with the interop protocol as currently written, and I worry that a relatively minor change like this would be much harder to implement in the future when interop is live than it would be now. We'd be happy for entrypoints to be an option as well, but I'm not sure that effort should preclude full consideration of this idea. Curious to understand your thinking around this better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

If this involved a lot of change to the messenger, avoiding this and instead leveraging entrypoints would make a lot of sense.

However the change is very minimal and at the same time very high leverage (consuming the return data via the event itself) in the event promise based contract development becomes more interesting.

related to the callback is managed on the sending chain and works with any remote target.

```solidity
library Promise {
Copy link

@karlfloersch karlfloersch Mar 21, 2025

Choose a reason for hiding this comment

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

I think the API for incentivized relay / callbacks should be as minimal and flexible as possible. This makes me wary about using a Library because then it's harder to hide the underlying callback logic and harder to update. This is why I lean towards using a dedicated contract for registering callbacks that you call vs using a library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea totally agree, the functions here are marked as external meaning this library gets deployed as a dedicated contract

no point in declaring this as a lib though, i'll update this to be a contract

@tynes
Copy link
Contributor

tynes commented Mar 21, 2025

This document is coupling too many different things. We are talking about a modification to the cross domain messenger, a promise library and an incentivized relay. I recommend creating a design doc that specifically is focused on the changes to the cross domain messenger that you want to get in. I also am not confident in making a bunch of small changes to the messenger without understanding how a specific user's app needs the change. Historically we added features when they sounded cool, which has resulted in a lot of tech debt. I want to see changes driven by an app attempting to integrate and then unable to do so because of the lack of a feature

@hamdiallam
Copy link
Contributor Author

This document is coupling too many different things. We are talking about a modification to the cross domain messenger, a promise library and an incentivized relay. I recommend creating a design doc that specifically is focused on the changes to the cross domain messenger that you want to get in. I also am not confident in making a bunch of small changes to the messenger without understanding how a specific user's app needs the change. Historically we added features when they sounded cool, which has resulted in a lot of tech debt. I want to see changes driven by an app attempting to integrate and then unable to do so because of the lack of a feature

Nowhere in this design doc is incentivized relays mentioned. I've explicitly made them separate since they are not dependent on each other. In the doc you'll see this is only about chaining side effects to the return value of a remote call, generally useful when writing cross chain contracts, removing the need to write handlers that ping pong back the return value for every possible remote view call

I'll go ahead and pull out the changes to the messenger but you're unnecessarily commingling the two

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