Only derive JsonError for errors that can return Json

[DanGould]

• 1st commit de-duplicates
• 2nd commit: Only some errors are actually recoverable and must return JSON. Fix receive::v2::SessionError variants are not recoverable and must not return JSON #522.

Re: #403

[coveralls]

Pull Request Test Coverage Report for Build 13208094848

Details

• 135 of 170 (79.41%) changed or added relevant lines in 9 files are covered.
• 10 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.3%) to 79.276%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v1/exclusive/error.rs	0	1	0.0%
payjoin/src/receive/v2/mod.rs	35	36	97.22%
payjoin/src/receive/v1/exclusive/mod.rs	27	29	93.1%
payjoin-cli/src/app/v1.rs	21	27	77.78%
payjoin-cli/src/app/v2.rs	14	20	70.0%
payjoin/src/receive/v1/mod.rs	17	23	73.91%
payjoin/src/receive/error.rs	6	19	31.58%

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/app/v2.rs	1	75.9%
payjoin-cli/src/app/v1.rs	4	78.46%
payjoin/src/receive/error.rs	5	16.91%

Totals
Change from base Build 13203244614:	0.3%
Covered Lines:	4028
Relevant Lines:	5081

---

💛 - Coveralls

[DanGould]

My refactor separates replyable from unreplyable errors. The biggest hurdle I've run into is when the payload gets parsed as a string before validate_payload. That prevents the PayloadError::Utf8 from being shared between v1 and v2 payload parsing even though the receiver has the sender's reply key at this point and could in fact reply to the sender. Going to think for a moment about whether or not a different format could be used to serialize the psbt || \n || query payload that'd get around this problem. Perhaps even serializing the query as a field of a strict byte length in message_a instead of in the string payload.

[spacebear21]

What do you think about naming the variants RecoverableError/UnrecoverableError instead of Un/Replyable? My brain keeps auto-correcting that to "replayable".

[spacebear21]

on line 182 before this change, there is a log::debug! that is now duplicated in validate_payload.

[spacebear21]

For brevity I prefer importing ReplyableError::{V1, Implementation} and using the variants directly (like it's done in app/v2.rs).

[DanGould]

I've changed the design again.

This time, Error is dividend into non_exhaustive Recoverable and V2 variants. I note that I do still prefer Replyable since some of the V2 variants are recoverable in the sense that they just require different configuration, they don't cause the program to crash. Jsonable or ReturnToSender names could also work.

I removed the Unrecoverable variant since it only covered v2::SessionError types, and the receive::Error type is almost certainly going to be extended with more multiparty variants.

I'm not sure if it makes sense for the v1 module to return RecoverableError, as it does now, since all the errors are indeed recoverable, or if it should be wrapped in the single receive::Error layer since that would let it match the other v2 module.

@spacebear21 please let me know what you think. I do indeed believe this is much closer to a stable reality based on actual hierarchy than it was before, especially since the fundamental difference between errors that may be returned to the sender is separated.

[spacebear21]

I'm ok with ReturnToSender or maybe Returnable.

I'm not sure if it makes sense for the v1 module to return RecoverableError

I'm not sure exactly where you mean - in extract_proposal_from_v1? I'm fine with returning the lowest common denominator (RecoverableError) in v1 functions and letting callers map to Error if needed.

On another note, I wonder if we should box a ImplementationError = Box<dyn Error + Send + Sync> type so that the receiver function signatures can be more semantically correct:

e.g.

        pub fn identify_receiver_outputs(
        self,
        is_receiver_output: impl Fn(&Script) -> Result<bool, ImplementationError>,
    ) -> Result<WantsOutputs, RecoverableError> {

[spacebear21]

This looks like it could be removed too in favor of:

#[cfg(feature = "v1")]
V1(crate::receive::v1::RequestError),

[DanGould]

Regarding names, I took a happy medium keeping ReplyableError but using the ReplyToSender variant.

Re:

we should box a ImplementationError = Box<dyn Error + Send + Sync> type so that the receiver function signatures can be more semantically correct:

This is an incredible idea! I was playing around with returning Box from these functions yesterday but didn't quite discover that an alias is how to make this semantically correct. Added it as a third commit and it looks great, way easier to implement the receive state machine properly by using ImplementationError::from.

Now I'm wondering how to handle the straggling receive errors: SelectionError, OutputSubstitutionError, InputContributionError. If unrecoverable, they're probably all ReplyableError variants with an unavailable or not enough money error code. I think I know how to address it in a follow up.

[spacebear21]

ACK 1358c50

Now I'm wondering how to handle the straggling receive errors: SelectionError, OutputSubstitutionError, InputContributionError. If unrecoverable, they're probably all ReplyableError variants with an unavailable or not enough money error code. I think I know how to address it in a follow up.

I think these are all technically recoverable from the receiver side, e.g. "try again with a different input selection". If the receiver can't handle them then they become unavailable in most (all?) cases. I'm not sure we should use the not-enough-money error code for InternalInputContributionError::ValueTooLow because BIP78 says it specifically applies to "could not bump the fee of the payjoin proposal. "