rust-payjoin icon indicating copy to clipboard operation
rust-payjoin copied to clipboard

Review usage of `crate::receive::Error`

Open spacebear21 opened this issue 1 year ago • 1 comments

Follow-up from https://github.com/payjoin/rust-payjoin/pull/277#pullrequestreview-2139827469

Server errors are meant to be returned as a response, but are being returned in places that don't always make sense, e.g.:

https://github.com/payjoin/rust-payjoin/blob/4bc8fe362226b07becd5ea8edd839bc5a4ff7a44/payjoin/src/receive/mod.rs#L486

https://github.com/payjoin/rust-payjoin/blob/37d255e9e5e4b4aeeb8c75f96e09cbb69c00552b/payjoin-cli/src/app/v1.rs#L318

https://github.com/payjoin/rust-payjoin/blob/37d255e9e5e4b4aeeb8c75f96e09cbb69c00552b/payjoin-cli/src/app/v2.rs#L345

It would be good to review usage of crate::receive::Error and improve error nesting generally.

spacebear21 avatar Jun 27 '24 19:06 spacebear21

Rather than having v2 wrapping the v1 logic, might it make more sense to have some core PSBT mutation receive logic that gets wrapped by the network-specific details of either v2 and v1 protocols? The v1 module could handle encoding the errors for HTTP (as is done now, since Error::Server is ~a 5xx error and Error::BadRequest is 4xx), and the inner error could be made which don't make as much sense in the context of v2 where the client sender speak HTTP directly to the receiver, and inner typestate error could be made to be generic, but not over the irrelevant HTTP error abstraction.

DanGould avatar Sep 30 '24 18:09 DanGould

After some high level review in #466 I discovered #468.

There are more than a a few issues here.

I think we can tackle this by following the general shape of the solution we've come up with in the send module:

  1. Separate payjoin validation and external errors. This is roughly what the BadRequest / Server variants started as trying to do but they encapsulate too much in one variant. Validation errors are those that might arise from a receiver not having enough money, the version being unsupported, or rejecting the original PSBT. External errors are those that the sender can't recover from. e.g. the receiver wallet fails to read from its DB. These should return an opaque, generic message according to the v1/v2 protocol. In v1 these probably are 500 error code responses, and in v2 we probably lean entirely on the unavailable json error message.
  2. Further divide validation errors into V1/V2 variants siloed within their respective modules
  3. Serialize the errors appropriately according to the wire protocol and handle them (i.e. fix #468). I have a feeling we should not be using Display for this. A pattern needs to be built into the state machine to make it difficult for implementers not to return errors properly, which even we're doing improperly now in payjoin-cli/v2.

DanGould avatar Jan 08 '25 04:01 DanGould