Skip to content

Fix PR #812 review issues#822

Closed
Mshehu5 wants to merge 2 commits into
payjoin:masterfrom
Mshehu5:Pr_812_followup
Closed

Fix PR #812 review issues#822
Mshehu5 wants to merge 2 commits into
payjoin:masterfrom
Mshehu5:Pr_812_followup

Conversation

@Mshehu5

@Mshehu5 Mshehu5 commented Jun 30, 2025

Copy link
Copy Markdown
Contributor

This PR addresses the review comments from @nothingmuch and @0xBEEFCAF3 on PR #812 (Deduplicate payjoin URI creation logic)

Changes Made

  • Import Cleanup: Add direct import of bitcoin_uri::Uri instead of using relative imports
  • Output Substitution Fix: Fix output substitution handling to properly default to enabled for v2 and disabled for v1

Implementation Details

The pj_uri function in SessionHistory now follows a clearer flow:

  1. Find the session context: First locates the session context from the Created event in the history
  2. Determine version and set output substitution: Checks if there's an UncheckedProposal event to determine the protocol version, then sets output substitution to disabled for v1 and enabled for v2
  3. Generate the URI: Creates the Payjoin URI with the appropriate context and output substitution setting

Mshehu5 added 2 commits June 30, 2025 14:04
- Add direct import of `bitcoin_uri::Uri` instead of using relative imports
- Fix output substitution handling to properly default to enabled for v2 and disabled for v1

This addresses the review comments from @nothingmuch and @0xBEEFCAF3.
- Add direct import of `bitcoin_uri::Uri` instead of using relative imports
- Fix output substitution handling to properly default to enabled for v2 and disabled for v1

This addresses the review comments from @nothingmuch and @0xBEEFCAF3.
@Mshehu5 Mshehu5 closed this Jun 30, 2025
@Mshehu5
Mshehu5 deleted the Pr_812_followup branch October 27, 2025 16:36
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.

1 participant