Demonstrate failure handling with fallback TX broadcast #1164
Conversation
Pull Request Test Coverage Report for Build 21247767675Details
💛 - Coveralls |
zealsham
force-pushed
the
handle-fallback-tx
branch
from
9597ed0 to
b25f2a6
Compare
|
@arminsabouri i implemented error downcast method for v2 in this PR, the downside of this method is having to repeat the same block of code for the the different state of the senders session. I only did it for the "pooling proposal" phase of the sender session, i'll have to do it for the rest of the phases but just want to know your input on this before i head that direction. i'm also simutaneously working on the "session outcome" based solution with the draft PR from you . |
zealsham
force-pushed
the
handle-fallback-tx
branch
from
3d44e43 to
44354f1
Compare
zealsham
force-pushed
the
handle-fallback-tx
branch
from
59fd8f2 to
1c4175e
Compare
As a refrence implementation , payjoin-cli needs to demonstrate proper fallback transaction handling in the case of payjon failure. From what i see , both sender and receiver can broadcast the fall back transaction , This pr starts by handling the sender side of things and would eventually progress to the receiver . this addresses payjoin#1156 fix unneccesary check handle fallback for propsal failing handle fallback for propsal failing handle fallback for propsal failing fix format issue handle fallback for propsal failing
zealsham
force-pushed
the
handle-fallback-tx
branch
from
1c4175e to
069fa34
Compare
zealsham
changed the title
| self.process_pj_response(psbt)?; | ||
| Ok(()) | ||
| } | ||
| Err(e) => { |
There was a problem hiding this comment.
I'm not sure we should be falling back on any Error. We should be doing something similar to what you did below and only fallback on a FatalError -- an irrecoverable protocol deviation.
| Err(e) => { | ||
| // Check if it's a PersistedError with an API error | ||
| if let Some(persisted_error) = e.downcast_ref::<PersistedError< | ||
| EncapsulationError, |
There was a problem hiding this comment.
Why downcast specifically to a EncapsulationError? Are there not other Fatal error types?
Regardless, if we want to go this route, we should explore the changes neccecary in the API to make this more ergonomic. and idiomatic
e.is_fatal() or e.should_fallback().
Alternatively we could explore integrating this change into the typestate it self. I.e the error statemachine scrutinizes the error type and decides what to do. Leaving the developer entirely out of it.
If we want to developer to decide how to react then we should explore including Outcome in close() #1181
I would like to talk through the options a bit more before commiting to this route. Let me know your thoughts @zealsham
cc @spacebear21
There was a problem hiding this comment.
i think the developer should ultimately decide what to do if a fatal error occurs. from the BIP
At any point, either party may choose to broadcast the fallback transaction described by the Original PSBT instead of proceeding
I believe some implementers might want the sender doing that or the receiver doing that, exposing outcome in close() handles that properly, automatically broadcasting fallback TX doesn't seem like what most devs will want.
Also i'll have to go through #1181 again , as i've lost most of the context.
3 participants
As a refrence implementation , payjoin-cli needs to demonstrate proper fallback transaction handling in the case of payjon failure. From what i see , both sender and receiver can broadcast the fall back transaction , This pr starts by handling the sender side of things and would eventually progress to the receiver .
This is still a work in progress
this addresses #1156 .
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.