Resume multiple payjoins - #283
Conversation
4a3136d to
3c42333
Compare
eb58f41 to
836873c
Compare
|
@grizznaut This is not yet quite buttoned up for release but is ready for a review. Before it gets released I'd like the end-to-end test to run the directory and ohttp relay locally. However I think I will leave session expiration to a follow up PR to be completed by the end of next week for the sake of review simplicity. |
7ee2f66 to
8b2caa7
Compare
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
Love the flow for resuming all in-progress sessions without having to supply additional parameters. tACK with some minor comments.
| let session = initializer | ||
| .process_res(ohttp_response.bytes().await?.to_vec().as_slice(), ctx) | ||
| .map_err(|e| anyhow!("Enrollment failed {}", e))?; | ||
| self.db.insert_recv_session(session.clone())?; |
There was a problem hiding this comment.
Should it follow the same "get or create session" pattern used in send_payjoin here?
There was a problem hiding this comment.
I chose not to because the receive command is making a request with a new amount. This way you can start parallel receive sessions. Otherwise, if you wanted to receive from A and B you would need some additional option to resume or not. I'd rather just leave that to the other command. If you accidentally start a new session and get a new BIP21 it's no big cost, but I could see it being a point of confusion. Did you expect it to resume? Perhaps receive should instruct one to use resume when the signal to shut down is given.
send knows the exact bip21 including the amount, so that made more sense to me to proceed with given an exact bip21 uri match than receive.
There was a problem hiding this comment.
I didn't have expectations one way or another but was wondering about the different treatment for send vs. receive. This makes sense to me.
Perhaps receive should instruct one to use resume when the signal to shut down is given.
Yeah, that sounds nice (same with send if it's still waiting on a response when shut down).
| let session = initializer | ||
| .process_res(ohttp_response.bytes().await?.to_vec().as_slice(), ctx) | ||
| .map_err(|e| anyhow!("Enrollment failed {}", e))?; | ||
| self.db.insert_recv_session(session.clone())?; |
There was a problem hiding this comment.
I chose not to because the receive command is making a request with a new amount. This way you can start parallel receive sessions. Otherwise, if you wanted to receive from A and B you would need some additional option to resume or not. I'd rather just leave that to the other command. If you accidentally start a new session and get a new BIP21 it's no big cost, but I could see it being a point of confusion. Did you expect it to resume? Perhaps receive should instruct one to use resume when the signal to shut down is given.
send knows the exact bip21 including the amount, so that made more sense to me to proceed with given an exact bip21 uri match than receive.
The sender has RequestContext. Now the receiver has a SessionContext.
This API is precarious because downstream URIs get built from multiple sources of BIP21 URI "Extras". If there were a way to export and combine the extras a most robust interface would be possible.
This lets us create a static bitcoin URI consistent across sessions.
The new resume command resumes a single prior receive session.
Resume both send and receive sessions.
Remove payjoin-cli v2 CI builds because this tests v2 which also builds.
|
I just rebased on master so that tests could run once more |
2 participants
Fix #267
Send and receive now open sessions with the directory that can be persisted. This includes an e2e test in the UI. As of this PR each session is persisted forever, but a follow up will handle session expiration and archive past sessions for both sender and receiver
This also introduces a new
payjoin-cli resumecommand that continues all active send and receive sessions from the database.struct SessionContextThe
V2Contexthas been replacedSessionContextthat contains an address for the session's URI and an expiry time at which the session is no longer usable.Closes #205 as a side effect