Skip to content

send: return error for v1 URIs in v2 sender - #1870

Open
vbrekher wants to merge 1 commit into
payjoin:masterfrom
vbrekher:fix/v1-sender-uri-error
Open

send: return error for v1 URIs in v2 sender#1870
vbrekher wants to merge 1 commit into
payjoin:masterfrom
vbrekher:fix/v1-sender-uri-error

Conversation

@vbrekher

@vbrekher vbrekher commented Sep 9, 2026

Copy link
Copy Markdown

SenderBuilder::new in the v2 sender currently panics when it receives a v1 Payjoin URI.

Make the constructor fallible instead and return a normal build error for unsupported v1 URIs. The FFI constructor propagates the same failure through SenderInputError::Build.

This keeps the v1 sender behavior unchanged while making v2 safe to use with untrusted Payjoin URIs.

AI assistance

ChatGPT was used to inspect the codebase, develop the patch, add tests, run validation, and draft this PR description.

Closes #1849

@coveralls

coveralls commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 34412200999

Coverage increased (+0.02%) to 86.663%

Details

  • Coverage increased (+0.02%) from the base build.
  • Patch coverage: 1 uncovered change across 1 file (24 of 25 lines covered, 96.0%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
payjoin/src/core/send/error.rs 2 1 50.0%
Total (3 files) 25 24 96.0%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 16571
Covered Lines: 14361
Line Coverage: 86.66%
Coverage Strength: 341.41 hits per line

💛 - Coveralls

@chavic

chavic commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Hi @vbrekher , thanks for picking this up. The added type UnsupportedVersion probably needs to match the same gating as the arm that uses it. Without that properly gated, I expect clipy to fail when only v2 or v1 is enabled

vbrekher commented Sep 9, 2026

Copy link
Copy Markdown
Author

Good catch, thanks. I gated UnsupportedVersion and the corresponding Display/Error::source match arms with #[cfg(feature = "v1")] so they match the constructor arm that uses the variant. Pushed the fix; the new Actions runs are currently waiting for approval.

@chavic

chavic commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

@vbrekher, logs show another unrelated error; you'll need to update the JS assertion too

vbrekher commented Sep 9, 2026

Copy link
Copy Markdown
Author

Thanks, found it in the JS integration log. The v1 URI now fails at SenderBuilder construction with the typed SenderInputError.Build, so the old /RuntimeError/ assertion was stale. I updated the assertion to expect SenderInputError.Build and removed the now-unreachable buildRecommended(...) call from that check. Pushed as d94e568.

@chavic

chavic commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Good catch, thanks. I gated UnsupportedVersion and the corresponding Display/Error::source match arms with #[cfg(feature = "v1")] so they match the constructor arm that uses the variant. Pushed the fix; the new Actions runs are currently waiting for approval.

And you'll actually want all(feature = "v1", feature = "v2"), since only having one should still fail. Can edit that commit rather than adding a new one. f85b369, which is the space removal, also belongs to an edit to a commit rather than being its own commit

@vbrekher
vbrekher force-pushed the fix/v1-sender-uri-error branch from d94e568 to ec30fce Compare September 9, 2026 22:25
Signed-off-by: v ₿ <valentin.brekher@gmail.com>
@vbrekher
vbrekher force-pushed the fix/v1-sender-uri-error branch from ec30fce to 34cc3a4 Compare September 9, 2026 22:26

vbrekher commented Sep 9, 2026

Copy link
Copy Markdown
Author

Yep, fixed. The gate is now all(feature = "v1", feature = "v2"), and I cleaned up the branch history so the follow-up fixes are folded into the original commit. I also rebased it onto the current master, so the PR is back to one commit and no longer drifted. New Actions runs are waiting for approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FFI sender doesn' support v1 payjoin URIs

3 participants