refactor: Refactors Payjoin URL generation and checkout integration - #125
refactor: Refactors Payjoin URL generation and checkout integration#125ValeraFinebits wants to merge 7 commits into
Conversation
fe903ab to
2b89303
Compare
There was a problem hiding this comment.
Pull request overview
Refactors Payjoin URL generation so validated, cached session URLs are merged server-side into checkout models, with improved concurrency, retry behavior, and settings-failure handling.
Changes:
- Extracts BIP21 merging, session servability, and cached URI reading.
- Adds persisted URI caching and optimistic concurrency.
- Narrows the checkout endpoint and rewrites client retry handling.
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Views/Shared/PayJoinBitcoinCheckoutEnd.cshtml |
Reworks checkout polling and mode synchronization. |
Services/RunTestPaymentService.cs |
Uses canonical absolute URIs. |
Services/PayjoinWalletOwnershipService.cs |
Handles unavailable settings. |
Services/PayjoinUriSessionService.cs |
Refactors session reuse, URI validation, and caching. |
Services/PayjoinUriResult.cs |
Adds retryability metadata. |
Services/PayjoinSwaggerProvider.cs |
Documents new API behavior. |
Services/PayjoinStoreSettingsUnavailableException.cs |
Adds a settings-read exception. |
Services/PayjoinStoreSettingsRepository.cs |
Distinguishes unreadable settings. |
Services/PayjoinSessionUriReader.cs |
Reads cached session URIs safely. |
Services/PayjoinSessionServability.cs |
Encapsulates session reuse decisions. |
Services/PayjoinReceiverSessionStore.cs |
Adds URI caching and concurrency retries. |
Services/PayjoinReceiverSessionState.cs |
Exposes cached URI and servability. |
Services/PayjoinReceiverSessionProcessor.cs |
Broadens per-session fault isolation. |
Services/PayjoinReceiverRelayRequestSender.cs |
Reports unreadable settings explicitly. |
Services/PayjoinReceiverPoller.cs |
Isolates reservation-cleanup failures. |
Services/PayjoinReceiverOutputBuilder.cs |
Handles unavailable settings. |
Services/PayjoinInvoicePaymentUrlService.cs |
Propagates retryability and standardized reasons. |
Services/PayjoinBitcoinCheckoutModelExtension.cs |
Builds Payjoin checkout URLs server-side. |
Services/PayjoinBip21.cs |
Centralizes BIP21 merging and validation. |
Services/IPayjoinStoreSettingsRepository.cs |
Makes settings reads nullable. |
Plugin.cs |
Registers the session URI reader and logging. |
Models/PayjoinUnavailableReasons.cs |
Centralizes unavailability reasons. |
Models/PayjoinCheckoutAvailabilityResponse.cs |
Defines the narrowed checkout response. |
Models/GetCheckoutBip21Response.cs |
Removes the old checkout response. |
Models/GetBip21Response.cs |
Adds retryability. |
Migrations/PayjoinPluginDbContextModelSnapshot.cs |
Records new session columns. |
Migrations/20260809084843_AddReceiverSessionPayjoinUriAndEventLogRevision.Designer.cs |
Defines the migration model. |
Migrations/20260809084843_AddReceiverSessionPayjoinUriAndEventLogRevision.cs |
Adds URI and revision columns. |
Data/PayjoinReceiverSessionData.cs |
Adds cached URI and revision fields. |
Data/PayjoinReceiverSessionConfiguration.cs |
Configures URI storage and concurrency. |
Controllers/UIStorePayjoinController.cs |
Surfaces unreadable settings. |
Controllers/UIPayjoinOverviewController.cs |
Reports settings failure status. |
Controllers/UIPayJoinController.cs |
Narrows the anonymous endpoint response. |
Controllers/GreenfieldPayjoinController.cs |
Returns 503 for unreadable settings. |
Tests/UIPayjoinOverviewControllerTests.cs |
Updates nullable settings mocks. |
Tests/UIPayJoinControllerTests.cs |
Tests the narrowed endpoint contract. |
Tests/TestSessionStates.cs |
Adds shared session-state fixtures. |
Tests/SessionStoreFixture.cs |
Adds shared in-memory store setup. |
Tests/Services/PayjoinReceiverSessionProcessorTests.cs |
Tests concurrency-failure isolation. |
Tests/Services/PayjoinReceiverSessionGuardTests.cs |
Reuses fixtures and tests servability. |
Tests/Services/PayjoinReceiverRelayRequestSenderTests.cs |
Tests unreadable settings failures. |
Tests/Services/PayjoinReceiverPollerTests.cs |
Updates shared fixture usage. |
Tests/Services/PayjoinReceiverInputSelectorTests.cs |
Updates session-store setup. |
Tests/Services/PayjoinAccountingBridgeServiceTests.cs |
Consolidates fixture usage. |
Tests/RelationalPluginTestSupport.cs |
Extends relational fault-injection support. |
Tests/PayjoinUriSessionServiceStatusTests.cs |
Tests reasons and retryability. |
Tests/PayjoinUriResultTests.cs |
Tests retryability defaults. |
Tests/PayjoinStoreSettingsTests.cs |
Tests missing versus unreadable settings. |
Tests/PayjoinSettlementFlowTests.cs |
Updates session creation. |
Tests/PayjoinSessionUriReaderFailureTests.cs |
Tests render-path database failures. |
Tests/PayjoinSessionServabilityTests.cs |
Tests session reuse decisions. |
Tests/PayjoinReplayedUriVerdictTests.cs |
Tests replayed URI validation. |
Tests/PayjoinReceiverSessionStoreTests.cs |
Tests caching and session lifecycle. |
Tests/PayjoinReceiverSessionStoreRelationalTests.cs |
Tests relational concurrency behavior. |
Tests/PayjoinReceiverSessionStateTests.cs |
Uses shared state fixtures. |
Tests/PayjoinInvoicePaymentUrlServiceTests.cs |
Updates result assertions. |
Tests/PayjoinCheckoutModelAssemblyTests.cs |
Tests server-side URL assembly. |
Tests/PayjoinCheckoutAvailabilityResponseTests.cs |
Tests response serialization. |
Tests/PayjoinBitcoinCheckoutModelExtensionTests.cs |
Tests checkout URL publication. |
Tests/PayjoinBip21Tests.cs |
Tests BIP21 parsing and merging. |
Tests/GetBip21ResponseTests.cs |
Tests response serialization. |
IntegrationTests/TestUtils/PayjoinInvoiceTestHelper.cs |
Uses nullable-safe settings helpers. |
IntegrationTests/TestUtils/PayjoinIntegrationTestSupport.cs |
Centralizes validated settings reads. |
IntegrationTests/PayjoinPluginIntegrationTests.cs |
Tests checkout and endpoint contracts. |
IntegrationTests/PayjoinMissingStoreSettingsIntegrationTests.cs |
Tests damaged settings behavior. |
IntegrationTests/PayjoinGreenfieldContractIntegrationTests.cs |
Tests Greenfield response contracts. |
Files not reviewed (1)
- BTCPayServer.Plugins.Payjoin/Migrations/20260809084843_AddReceiverSessionPayjoinUriAndEventLogRevision.Designer.cs: Generated file
Suppressed comments (1)
BTCPayServer.Plugins.Payjoin/Views/Shared/PayJoinBitcoinCheckoutEnd.cshtml:268
- An explicit
retryable: falsedoes not end the chain here:giveUpUntilis consumed after five minutes and the endpoint is queried again for up to 12 cycles. That contradicts both this comment and the new contract that says another request cannot produce a different answer, and it continues invoking the expensive anonymous bootstrap endpoint. Mark this invoice's retry state as stopped instead; changing invoices already resets that flag.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 70 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- BTCPayServer.Plugins.Payjoin/Migrations/20260809084843_AddReceiverSessionPayjoinUriAndEventLogRevision.Designer.cs: Generated file
Suppressed comments (2)
BTCPayServer.Plugins.Payjoin/Services/PayjoinReceiverSessionStore.cs:111
- Matching only the receiver address does not prove this is the session that produced
payjoinUri. If that row is removed and recreated for the same invoice/address between replay and this write (notably across app instances, since the build lock is process-local), an old session endpoint is cached on the replacement; later builds cannot correct it becausePayjoinUriis already non-null. Include the replayed session's generation identity (for exampleCreatedAtor a dedicated generation token) in this conditional write.
BTCPayServer.Plugins.Payjoin/Services/PayjoinUriSessionService.cs:243 ReplayReceiverEventLogcan throwReceiverReplayException, but this handler now catches only low-levelUniffiException. A corrupt persisted event log therefore bypasses the discard logic, reaches the outer generic fallback, and remains in storage, so every retry replays the same broken session. Keep the typed replay exception in this cleanup path (asPayjoinReceiverSessionGuardandPayjoinReceiverStateProcessordo).
There was a problem hiding this comment.
As a first pass, reviewed the session store, the concurrency work, and the endpoint contract. I did not read all 70 files.
EventLogRevision does not track the event log. AppendEventsWithAccountingUpdate adds events and sets UpdatedAt, but does not increment it. Only TryReserveContributedInput and RemoveAllSessionEvents do.
That is fine for the cached URI, because the URI comes from the session's initial state and does not change as events arrive. The name says otherwise, though, so the next person who uses it to detect log changes gets a wrong answer with no error.
It also makes the concurrency guard partial. EF puts the token in the WHERE clause, so a competing write that leaves the token alone still matches. RequestClose against TryConsumeInitializedPollAfterCloseRequest, or an append against either, both write the same value, both succeed, and WriteWithConcurrencyRetry never runs.
Two options. Increment it on every write that touches the row, or rename it to RowVersion and record in a comment which paths it guards. On Postgres, UseXminAsConcurrencyToken() gives a real row version that the database maintains, but the InMemory provider the unit tests use does not support xmin, so that creates a different problem.
Migration note: this and #113 both add a migration and rewrite PayjoinPluginDbContextModelSnapshot.cs. #113 is a draft and will rebase on top of this, so I will regenerate mine after this lands. Nothing needed from you.
|
The rate-limit TODO is worth an issue; probably should check if one exists. This isn't the first time, if I remember. The comment is exact: anonymous, runs BuildAsync, queries wallet UTXOs, bootstraps OHTTP against third-party relays, client-side limit only. A TODO does not stop a release. |
e2bdac0 to
4e26d1c
Compare
@chavic Good catch, the token never tracked the event log. Renamed it to |
Refactor Payjoin URL generation and checkout integration
This PR restructures how the payjoin payment URL is built and delivered to the checkout page:
PayjoinUriSessionService: BIP21 merge logic (PayjoinBip21), persisted-session reuse decisions (PayjoinSessionServability), and cached-URI reads on the render path (PayjoinSessionUriReader).GetInvoicePaymentUrlnow returns only{ status, retryable }instead of the payjoin URI. The merged payment URL is built server-side into the checkout model from the session store, so the payer never receives data the render path hasn't validated.PayjoinUri+EventLogRevision), so checkout renders don't replay the event log.EventLogRevisionacts as a concurrency token; store writes go through a bounded retry wrapper (WriteWithConcurrencyRetry).RemoveAllSessionEvents(which also clears the cached URI), andTryRemoveSessionUnlessNegotiatingrefuses to discard sessions with contributed inputs.retryable: falseverdict.GetAsyncreturnsnull), surfacing a non-retryable unavailability instead of silently falling back to defaults.Known follow-ups (marked with TODOs)