Refactor Receiver Replyable Error

[spacebear21]

I think this would be better left as a follow up. There is a refactor we would need to do to get error types persisted into terminal session events and remove the JSON replies all together and this can get resolved as part of that follow up. Leaving this comment open so can make an follow up issue

Originally posted by @0xBEEFCAF3 in #750 (comment)

Edit:
AC: Refactor receiver replayable error types to be serializable, include debug messages and the ability to extract a json reply. Once this is achieved, we can remove JSON reply from the receiver error session event log and have it derived from the primary information

[arminsabouri]

There is a refactor we would need to do to get error types persisted into terminal session events and remove the JSON replies all together

Just adding derive for serde serialize/deserialize to the reciever error types is proving to be untenable. There are foreign types in the sub variants. In order to decouple the error and json reply session event we could start by saving them in seperate session events. That would atlease remove the ugly tuple and simplify the down stream code.

rust-payjoin/payjoin/src/receive/v2/session.rs

Line 162 in f6d4a61

SessionInvalid(String, Option<JsonReply>),

should we consider a error type that that perhaps wraps the others that is meant to be ser/deser'd? Either way there will be information loss bc we'll need to rely on a to and from string methods. If our primarly goal at this point in the project is to display why a session is closed (i.e book keeping) then saving a string is fine. In future multiparty constructions we may want take a specific action during replays if some error variant occured. When we cross that line I think we can either save a specific error variants as their own session event or a general session invalid event type that wraps specific errors that impl serde.

[DanGould]

I think we have information overload at the moment wrt our errors.

There are foreign types in the sub variants.

Part of #403 ought to be to remove the foreign types and sub-variants except where strictly necessary. I think the rust for rustaceans Error handling chapter is useful here. I want to make sure we're addressing the fundamental architectural problem rather than switch from one hack to another. Off the top of my head, it seems like the important information for a ReplyableError is

• well known error type from which the user-facing display string can be derived
• the debug message to pass
• and maybe a specific enum variant with some semblance of an error source

Other than that, we're probably carrying too much data at this time which is where the lossyness comes from. Key point being that losing data we don't care about is not actually a loss, and we shouldn't keep it in the first place in that case.

[arminsabouri]

Relevant comment #981 (comment)

[DanGould]

we can remove JSON reply from the receiver error session event log and have it derived from the primary information

In order to remove the JSON reply from the receiver error event, we'd need to EITHER

1. Save a ~ReplyableError minus the implementation / transient variants OR
2. have all of the primary information including the not-necessarily-serializable input to a function that makes it fail such as:

• Receiver<Initialized>::process_response(&mut self, body: &[u8] ,context: ohttp::ClientResponse) fails. We'd need to save body + context
• Receiver<UncheckedProposal>::check_broadcast_suitability has a non-implementation error
• Receiver<MaybeInputsOwned>::check_inputs_not_owned has a non-implementation error
• Receiver<MaybeInputsSeen>::check_no_inputs_seen_before has a non-implementation error
• Receiver<OutputsUnknown>::identify_receiver_outputs has a non-implementation error
• Reciever<WantsFeeRange>::apply_fee_range fails (this does not seem like a necessarily fatal error, but it is because the calculation only errors if the additional_input_weight can't be found rn).

Therefore, it seems like easier to save an error type. If we can ignore the Implementation error variants as NOT replyable in a v2 context (and only replyable in a v1 context) then I suppose we could save them to be serializable.
We probably also want a way to know if we had a successful reply or not as an event in and of itself. If you have an error reply but drop network, you'll want to try again.

[DanGould]

@arminsabouri must extract_err_req and process_err_res return transitions of their own and be brought into the state machine in the case of terminal failure and saving the fact that a response has been produced successfully?

[arminsabouri]

@arminsabouri must extract_err_req and process_err_res return transitions of their own and be brought into the state machine in the case of terminal failure and saving the fact that a response has been produced successfully?

That seems like an ergonomic solution. FatalTransitions will then return a needs error reply typestate where extract_err_req and process_err_res are called. Once process_err_res is saved the Error can be the returned. This may be alot of boiler plate for caller of any state transition method since it could produce two different typestates. But if you are looping and matching on typestates that should be "fine" -- hard to tell with out getting into it.

Alternatively we could have a session persister like trait. ReplyErrorHandler with a method that instantiates a http agent, sends the http payload out and processes the response. If you have a fatal error you can pass the err_req to the trait methods.

[DanGould]

@DanGould:

I recall you writing somewhere that v1/v2 aren't actually replying with errors. Do you have any recollection of where you wrote this down?

@nothingmuch:

1. reply_key not having been used to reply means that v2 sender doesn't check for replyable errors where receiver would send it until recently, that indicates a gap in our coverage

2. for v2 receiver, v2 sender, it only makes sense to send a replyable error after parsing a valid reply key, so not all fatal errors are repliable

I think that means we make some MustTryReply that attempts a reply (and itself can fail). Another type of error that can't be replied is an expiration.