Remove unneeded PSBT fields when extracting request#907
Remove unneeded PSBT fields when extracting request#907arminsabouri merged 2 commits intopayjoin:masterfrom
Conversation
`PsbtContextBuilder` stores a Psbt with [unneeded fields removed](https://github.com/payjoin/rust-payjoin/blob/57ec41b1ebfd325ded9fc216096ec3930ed40b4b/payjoin/src/core/send/mod.rs#L524). Our API later attempts to restore _some_ of these fields after we process the response from the receiver. https://github.com/payjoin/rust-payjoin/blob/57ec41b1ebfd325ded9fc216096ec3930ed40b4b/payjoin/src/core/send/mod.rs#L411 However, since the original psbt was already stripped of "unneeded" fields there is nothing to restore. This commit removes the extranous PSBT fields when we are extracting the POST request.
Pull Request Test Coverage Report for Build 16604779713Details
💛 - Coveralls |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
tACK, confirmed that the newly added test_restore_original_utxos fails on master.
It would be great to add another test validating the clearing behavior in sender requests (see comment below) in this PR if it's not a big lift.
| ); | ||
| let body = psbt_ctx.original_psbt.to_string().as_bytes().to_vec(); | ||
| let mut sanitized_psbt = psbt_ctx.original_psbt.clone(); | ||
| clear_unneeded_fields(&mut sanitized_psbt); |
There was a problem hiding this comment.
It would be nice to actually validate this behavior in a test, I can comment out this line and all tests are still green despite it violating the BIP78 spec. We could augment test_clear_unneeded_fields so it checks the request body instead of just validating the function internals?
There was a problem hiding this comment.
Or we could augment the new test that I have introduced. Eitherway happy to make this change
There was a problem hiding this comment.
I think we could add some assertions in our v1 integration test for the most minimal testing.
rust-payjoin/payjoin/tests/integration.rs
Lines 88 to 91 in 1f39170
There was a problem hiding this comment.
Actually what you suggested may be easier
There was a problem hiding this comment.
I just realized that might not be the right scratch that it looks good I got all my wires crossedcreate_v1_post_request method? ... they're all public
Add some coverage to the higher level function where we actually clear the unneeded fields.
|
Ack on a276767 makes sense to just call |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
reACK pending passing checks
4 participants
PsbtContextBuilderstores a Psbt with unneeded fields removed. Our API later attempts to restore some of these fields after we process the response from the receiver.rust-payjoin/payjoin/src/core/send/mod.rs
Line 411 in 57ec41b
However, since the original psbt was already stripped of "unneeded" fields there is nothing to restore. This commit removes the extranous PSBT fields when we are extracting the POST request but leaves the fields in the
original_psbtuntouched.Related ticket: #906