Improve error handling for URI parsing by Johnosezele · Pull Request #657 · payjoin/rust-payjoin

Johnosezele · 2025-04-16T14:14:33Z

This pr addresses #644

I introduced PayjoinUriError type to provide better error handling for URI parsing operations. This change:

Adds PayjoinUriError type to wrap bitcoin_uri::de::Error
Updates UriExt trait to use PayjoinUriError instead of Box
Improves error messages for unsupported payjoin URIs
Adds tests to validate the new error handling

This change improves error handling clarity and type safety in the URI parsing module.

Closes #644

Add unit tests for the [new_p2wpkh] and [new_p2tr] constructors within the `receive` module. These tests verify that the constructors correctly initialize the [InputPair] struct, ensuring both the `TxIn` and `psbt::Input` components match expected values. This improves code quality by confirming constructor behavior and guarding against regressions. Also addresses compilation errors found during testing: - Corrected call to [new_p2wpkh] to include the `sequence` argument. - Removed assertions checking non-existent fields on `psbt::Input` (e.g., `script_sig`, `tap_control_block`). - Renamed `tap_leaf_scripts` to the correct field `tap_scripts` in assertions.

Added comprehensive test coverage for critical payjoin features: Privacy Protection: - test_privacy_based_input_selection: Verifies UIH (Unnecessary Input Heuristic) avoidance by testing input selection with different amounts (5k vs 50k sats) - Ensures proper selection logic to maintain transaction privacy and resist blockchain analysis Fee Management: - test_fee_rate_based_selection: Validates fee rate enforcement via check_broadcast_suitability - Enforces minimum fee rate (2 sat/vB) to prevent probing attacks - Verifies rejection of below-minimum fee rate proposals Transaction Processing: - test_minimum_fee_requirements: Tests end-to-end fee handling during input contribution - Validates fee application within min/max ranges through finalize_proposal - Ensures receivers can properly cover their input costs All tests interact with public APIs while testing internal behaviors, maintaining proper encapsulation. This test suite provides coverage for core payjoin privacy and security features.

The changes include: - Reorder imports to fix trait resolution - Update test to use proper trait implementation syntax - Verify changes with full test suite across toolchains Tested: - Format check with nightly rustfmt - Lint check with contrib/lint.sh - Tests on MSRV (1.63.0) - Tests on stable - Tests on nightly

benalleng

I think a lot of the comments are a bit superfluous and will end up stale and can be removed. Any that you feel should stay should at least be turned into doc string comments. Also keep your eye out as sometimes llms cannot quite parse the purposes of some more pinpointed test as below.

benalleng · 2025-04-16T16:10:02Z

payjoin/src/receive/v2/mod.rs

+        // Verify key consistency through Value trait implementation
+        let key = <Receiver as Value>::key(&session);
+        assert_eq!(key.as_ref(), key.as_ref());


This change doesn't get at the purpose of the old assert here, It is fixed instead in #658

Thanks Benalleng, I will cleanup the extra comments, I initially left them to help me keep track. I will update the pr to revert the assertion change

I think the issue here is I didn't run git rebase, so previous conflicts from my former pr was layed on this pr. Linear history quite messy too. I will fix. This change was as a result of a merge conflict in my first pr when I pulled from upstream, to send this new pr, it seems I did not handle the merge properly, pls note that I did not add the test for receiver_ser_de_roundtrip(), if you look at my previous pr here: 641 you'd see all tests I added.

This isn't even some issue with using llms at all. I think if the pr does not satisfy the requirements to fix the issue at hand, it should be stated why.

arminsabouri

Havent looked too deeply into this one yet. Just a few comments.
But the one thing I would definetly recommend is to break this PR apart into smaller more digestable chunks. i.e do we need all these changes in receive/mod.rs to satisfy #644 ?

arminsabouri · 2025-04-16T16:43:31Z

payjoin/src/receive/mod.rs

+        }
+    }
+
+    #[cfg(feature = "v2")]


Is p2tr not supported in bip78? I think it should be

Also I would group all these methods together. new_p2tr shouldnt be seperated from the other ones.

arminsabouri · 2025-04-16T16:46:18Z

payjoin/src/receive/mod.rs

+/// Helper to construct a pair of (txin, psbtin) with some built-in validation
+/// Use with [`InputPair::new`] to contribute receiver inputs.


Copy pasta?

Also I think this error variant already exists. Do we need both?

arminsabouri · 2025-04-18T13:07:44Z

payjoin/src/uri/mod.rs

+            MaybePayjoinExtras::Supported(extras) => {
+                assert_eq!(extras.output_substitution, OutputSubstitution::Disabled)
+            }


Was this change made as a result of running fmt?

arminsabouri · 2025-04-18T13:11:43Z

payjoin/src/uri/error.rs

+            BadEndpointError::LowercaseFragment => {
+                write!(f, "Some or all of the fragment is lowercase")
+            }


Same thing here. The commit message indicated better error handing but I am just seeing formatting changes. Am I missing something?

I had to redo the pr, lots of unclean commit history

arminsabouri · 2025-04-18T13:15:05Z

payjoin/src/receive/mod.rs

+        Self { txin, psbtin }
+    }
+
+    pub fn new_p2pkh(


Typically applications that integrate PDK have a rich PSBT with this information already available. Is this mostly useful for testing? If so lets just create test utils. And since this change doesnt seem related to URI parsing it probably belongs in its own PR

I will address this in a new pr, thanks

Johnosezele · 2025-04-18T14:26:40Z

I'm closing this PR, it did not really cover the changes made, lots of changes from previous pr's meshed together and then merge conflict too. I had to redo it on a clean branch. pls check the right PR here #661

Johnosezele added 7 commits April 10, 2025 16:46

Merge remote-tracking branch 'upstream/master'

036dfaa

Remove test_new_p2wpkh and cleanup test comments in receive/mod.rs

b23e05d

apply nightly rustfmt

c9da47e

Improve error handling for URI parsing

c4317e3

Johnosezele mentioned this pull request Apr 16, 2025

Wrap URI errors in a payjoin type #644

Open

benalleng requested changes Apr 16, 2025

View reviewed changes

arminsabouri reviewed Apr 18, 2025

View reviewed changes

Johnosezele closed this Apr 18, 2025

		/// Helper to construct a pair of (txin, psbtin) with some built-in validation
		/// Use with [`InputPair::new`] to contribute receiver inputs.

Conversation

Johnosezele commented Apr 16, 2025

Uh oh!

benalleng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Johnosezele commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants