Skip to content

Handle response status - #284

Merged
DanGould merged 2 commits into
payjoin:masterfrom
DanGould:handle-response-status
Jun 19, 2024
Merged

Handle response status#284
DanGould merged 2 commits into
payjoin:masterfrom
DanGould:handle-response-status

Conversation

@DanGould

@DanGould DanGould commented Jun 11, 2024

Copy link
Copy Markdown
Member

Oblivious HTTP wraps an HTTP message that contains content and control information. We were ignoring content information like status even though it is essential to the Payjoin V2 protocol. Handle it.

I noticed this issue when trying to send async payjoin. Even if the directory returns a 202 ACCEPTED message to wait for the receiver's proposal, the polling stops because we try to decrypt the content (which is empty, and errors) rather than short circuit the function and return None

@DanGould
DanGould force-pushed the handle-response-status branch 2 times, most recently from 2290152 to 91f1a3b Compare June 11, 2024 18:19
@DanGould
DanGould marked this pull request as ready for review June 11, 2024 18:26
@DanGould DanGould added receive receiving payjoin send sending payjoin api labels Jun 12, 2024
@DanGould
DanGould force-pushed the handle-response-status branch from 91f1a3b to 94fbadb Compare June 12, 2024 18:53
@DanGould
DanGould requested a review from spacebear21 June 13, 2024 15:26
@DanGould
DanGould force-pushed the handle-response-status branch from 94fbadb to 11adc2d Compare June 17, 2024 16:13

@spacebear21 spacebear21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with "serde" added back into the dependencies list, works fine otherwise.

Comment thread payjoin/src/.DS_Store Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Mac things

Comment thread payjoin/Cargo.toml Outdated
@DanGould
DanGould force-pushed the handle-response-status branch from 11adc2d to 616dc82 Compare June 18, 2024 15:11

@spacebear21 spacebear21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK. It appears the latest hyper-rustls breaks MSRV though.

@DanGould

Copy link
Copy Markdown
Member Author

Yeah, I was a bit sloppy on my last push trying to rebase on master and missed the serde dependency.

I'm working on #192 which will include a transition to hyper-1.0 transition to fix msrv without the band-aid that is pinning, but it's going to take a little while. Perhaps I'll get it done today 🤞

@DanGould
DanGould force-pushed the handle-response-status branch 2 times, most recently from 5199daf to 52a8b4d Compare June 18, 2024 17:47
@DanGould

Copy link
Copy Markdown
Member Author

While #192 does need to be addressed, that's actually not related to this exact problem, which is that reqwest 0.12.5 broke MSRV. I believe my pin is the necessary fix

@DanGould
DanGould requested a review from spacebear21 June 18, 2024 18:02
DanGould added 2 commits June 18, 2024 16:57
Oblivious HTTP wraps an HTTP message that contains content and control
information. We were ignoring content information like status even
though it is essential to the Payjoin V2 protocol. Handle it.
There's nothing to do with the response except check the status code. Do that
in the process_res function.
@DanGould
DanGould force-pushed the handle-response-status branch from 52a8b4d to 4db618c Compare June 18, 2024 20:57
@DanGould

Copy link
Copy Markdown
Member Author

I removed had the "pin reqwest" commit since it was merged by #294

@DanGould
DanGould merged commit 209f33e into payjoin:master Jun 19, 2024
@DanGould
DanGould deleted the handle-response-status branch June 19, 2024 14:35
@DanGould
DanGould restored the handle-response-status branch June 26, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api receive receiving payjoin send sending payjoin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants