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

V2 Original Message A error json not returned to Sender from Receiver #468

Open
DanGould opened this issue Jan 8, 2025 · 6 comments · May be fixed by #474
Open

V2 Original Message A error json not returned to Sender from Receiver #468

DanGould opened this issue Jan 8, 2025 · 6 comments · May be fixed by #474
Labels
api bug Something isn't working

Comments

@DanGould
Copy link
Contributor

DanGould commented Jan 8, 2025

If a sender message is invalid, say because the receiver's wallet can't sign, it's supposed to return a error so the sender isn't just infinitely polling.

I don't believe this is happening. We have no way to extract an OHTTP Encapsulated response containing this error information and pass it back to the sender in our existing API. Only the happy path actually produces OHTTP messages. This means senders will poll until expiration rather than quickly stopping if they know something is wrong after receiving a rich error.

This presents both a problem and a design opportunity.

The most straightforward design based on our current implementation would be to pass the JSON error as a string in the payload before new newline containing the query parameter.

@nothingmuch
Copy link
Collaborator

Is there a well defined list of errors, and any recourse for the sender being able to distinguish them? "receiver's wallet can't sign" seems like it might reveal more information to a potentially malicious counterparty than desirable. As long as failure is signaled (perhaps just by receiver broadcasting the fallback tx?) then the sender is basically out of options, right?

Secondly, JSON kinda scares me as a precedent, for a few reasons mainly having to do with its rigidity, and because if there is a well defined list of errors that could just be an error code, and if there isn't then it's wrapping what will fundamentally be human readable data in a machine readable wrapper. If multiple machine readable errors are motivated, then this might be reason enough to revisit the #375 and whether or not we want NULL terminated base64 PSBT, or explicit TLV encoding of the payload, but my intuition is that it'd be better to just signal general failure since I don't see any recourse in the event of any failure other than to either give up on the payment (but receiver can still broadcast) or broadcast earlier than timeout.

@DanGould
Copy link
Contributor Author

DanGould commented Jan 8, 2025

Yes there is an error list which we handle thoroughly on the sender side. See Receiver's well known errors in BIP 78. They include custom messages for humans in debug logs as well but only present template messages to be presented to UIs

@nothingmuch
Copy link
Collaborator

Thanks, I forgot about that.

I still feel a little dubious about the value of handling those, since unavailable suggests perhaps there's good reason to retry later, but not-enough-money, version-unsupported and original-psbt-rejected all leave the sender without many options.

But assuming they're still appropriate, and ignoring my JSON related complaints since it's already in the spec, I see two options:

  1. explicit marking
  2. relying on the fact that a JSON object, so it should start with \s*{\s*", and neither character is in RFC 4648 base64, so technically no need to disambiguate

@DanGould
Copy link
Contributor Author

DanGould commented Jan 8, 2025

In practice these WellKnownErrors are most valuable for a developer debugging why their thing is wrong, especially the original-psbt-rejected messages. DX is the raison d'etre for PDK so I think it makes sense to handle them, especially as we have the handler code already for v1.

option 2 checking for \s*{\s*") seems the simple path forward to me

@DanGould
Copy link
Contributor Author

DanGould commented Jan 8, 2025

To address this cleanly I realize each typestate probably needs a way to transform errors into requests, which means having access to the SessionContext to ohttp_encapsulate the JSON error.

I see a few ways this could be done:

  1. passing SessionContext to the error (kind of a lot of boilerplate) and defining a function to extract the request + context
  2. Defining that functions for each typestate that might throw an error
  3. Or defining a generic Receiver that takes each state as its generic type instead of having individual states and defining one fn extract_error_request(self, error: Error) -> (Request, ohttp::ClientResponse) shared between them.

The last one seems like the cleanest to me but it's also the biggest change. I'm not sure how bindings would/could handle a generic typestate machine either, we may end up having to re-define each typestate in bindings anyway to support it.

@DanGould
Copy link
Contributor Author

If FeeRate preferences from receiver are incompatible with Sender preferences, they may be returned to the Sender and displayed as WellKnown. This has been ignored up to this point as far as I can tell, but we could easily template a error message that displays the receiver's feerate preference bounds.

@DanGould DanGould linked a pull request Jan 13, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api bug Something isn't working
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants