Improve receive error heirarchy

[DanGould]

I delete a few error types, refine function definitions, and ultimately change the receive module error heirarchy so that the top level receive::Error is the only one that contains the Implementation variant, where previously these could be defined in multiple places across the stack.

In order to solve #793 and #403 two (currently conflated) problems must be solved: Error Handling and error classification. This addresses the latter.

I'll need to follow this up to address #793, but getting the hierarchy of classification right, including the removal of superfluous errors was my first step.

Error handling involves a response to the counterparty and reporting to the client. In V1, that means serializing the error as JSON and making the network call to respond as well as printing a log message. That's demonstrated here in the 5th commit. We'll need to introduce some testing around this as it firms up. In v2, errors need to be persisted. They can be either fatal or transient in nature, with basically all errors being Implementation errors. It is not yet clear to me whether all ImplementationError types imply transient errors for certain, but for now that is how we make the distinction. if this is not the case, we must come up with some enumeration for ImplementationErrors that let us better categorize them so that the history replay can handle them appropriately.

Implementation Error classification

For example, this was an LLM generated suggestion of such classification:

pub enum TransientImplementationError {
    StorageTemporaryFailure(String),    // Serialized error message
    NetworkFailure(String),
    WalletUnavailable(String),
    DatabaseConnection(String),
}

pub enum FatalImplementationError {
    InvalidConfiguration(String),
    WalletSigningFailure(String),
    PsbtProcessingFailure(String),
    SessionExpired(String),
}

I think all error states, fatal or transient, need to be persisted in v2+ that way on replay a client can be presented with the error as the current state, even if transient. Fatal errors may need to be replayed and marked as to whether or not they have been communicated to the counterparty or if further action is required at a later resumption. In order to serialize these errors I believe we want to expand on the ErrorCode type to be a SerializableError that includes the error kind with fixed messages for presentation. Perhaps more details may be logged at the time of conversion.

In order to replay them I think we need to replace the SessionEvent::SessionInvalid variant with Transient and Fatal variants that just hold this SerializableError instead of the current TODO marked type SessionInvalid(String, Option<JsonReply>),

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[DanGould]

this is intentionally left in to be fixed when SessionInvalid is swapped for more specific classification.

[coveralls]

Pull Request Test Coverage Report for Build 17412954810

Details

• 125 of 155 (80.65%) changed or added relevant lines in 8 files are covered.
• 5 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.01%) to 85.902%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v1/error.rs	0	1	0.0%
payjoin/src/core/receive/v1/mod.rs	16	17	94.12%
payjoin/src/core/receive/error.rs	40	46	86.96%
payjoin/src/core/receive/v2/mod.rs	39	46	84.78%
payjoin-cli/src/app/v1.rs	6	21	28.57%

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/app/v1.rs	1	71.37%
payjoin/src/core/receive/v2/error.rs	1	27.27%
payjoin/src/core/receive/error.rs	3	45.9%

Totals
Change from base Build 17337532308:	-0.01%
Covered Lines:	8171
Relevant Lines:	9512

---

💛 - Coveralls

[spacebear21]

Concept ACK - The Protocol v. Implementation error distinction is way more meaningful than ReplyToSender v. V2.

In V1, that means serializing the error as JSON and making the network call to respond as well as printing a log message. That's demonstrated here in the 5th commit. We'll need to introduce some testing around this as it firms up

I think introducing a basic test for this would fix the failing cargo-mutants check in CI.

I think all error states, fatal or transient, need to be persisted in v2+ that way on replay a client can be presented with the error as the current state, even if transient. Fatal errors may need to be replayed and marked as to whether or not they have been communicated to the counterparty or if further action is required at a later resumption.

This sounds right to me.

[spacebear21]

Error string and docstring need update too

[spacebear21]

Here too

[spacebear21]

The docstring for this enum too

[spacebear21]

We may want to implement impl From<&SessionError> for JsonReply even if all variants are handled in the same way (this is currently the case for v1::RequestError)

[DanGould]

Because such From impl is part of the public api, I figured this one liner was a simpler solution. Are yo opposed to handling RequestError that way instead here too, in contrast to your suggestion?

[spacebear21]

Handling RequestError here instead sounds good

[spacebear21]

I realize this is just a code move, but I wonder if we should be more descriptive about why this error occurs e.g. "this is not the expected Payjoin transaction"

[spacebear21]

code ACK 59dad2e, I can re-ACK if you want to address my newer comments, or they can be handled in a follow-up.