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

CustomOnionMessageHandler::handle_message doesn't expose the reply_path #2882

Closed
TheBlueMatt opened this issue Feb 7, 2024 · 5 comments · Fixed by #2907
Closed

CustomOnionMessageHandler::handle_message doesn't expose the reply_path #2882

TheBlueMatt opened this issue Feb 7, 2024 · 5 comments · Fixed by #2907

Comments

@TheBlueMatt
Copy link
Collaborator

I'm trying to implement an onion message handler that does some work async and then eventually replies to an onion message. However, handle_message currently only exposes the message itself but not the reply path, which would be needed to be able to respond async. To avoid cloneing the BlindedPath in such a case, maybe it makes sense to drop the whole reply-inline logic and force users to reply via release_pending_custom_messages?

@jkczyz
Copy link
Contributor

jkczyz commented Feb 7, 2024

Hmm... possibly though it would be nice to have some optionality here. We could pass the handler an object that has methods like:

  • respond for handling the current case
  • respond_with_reply_path for giving a reply path in the response (needed for proper invoice error handling still)
  • defer (or something) to consume the object and obtain the reply path, path_id, etc.

@TheBlueMatt
Copy link
Collaborator Author

That seems sensible, and then the method would return a PendingOnionMessage<Self::CustomMessage> rather than a Self::CustomMessage?

@jkczyz
Copy link
Contributor

jkczyz commented Feb 7, 2024

Not 100% sure. I was thinking those first two methods would essentially act as callbacks to OnionMessenger::handle_onion_message_response. (Note that we'll need the second method for OffersMessageHandler when responding with an invoice plus reply path, so this isn't only applicable to custom messages.) Then the message handler would no longer have a return value since responding async wouldn't require one. But I can see there being other ways of formulating this.

@TheBlueMatt
Copy link
Collaborator Author

Yea, I don't really have much of an opinion on how this should look, any of those seem reasonable to me.

@shaavan
Copy link
Contributor

shaavan commented Feb 15, 2024

Quick heads up, I'm actively tackling this issue now. Will update as soon as I make some good progress!

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 a pull request may close this issue.

3 participants