Rename Receiver<Monitor>::check_for_broadcast to check_for_transaction, etc.#1684
Merged
spacebear21 merged 1 commit intoJun 26, 2026
Merged
Conversation
"broadcast" over-claims what this checks. The caller's closure, not the method, decides the condition that counts as success. Mempool presence, n confirmations, etc. Naming the method after one point on that spectrum is misleading because different environments have different success conditions. Rename the Receiver<Monitor> method to the neutral check_for_transaction and reword its doc to say the closure defines the found condition. Update the payjoin-ffi and payjoin-cli callsites to match. Also rename the closure from transaction_exists to find_transaction (and the payjoin-ffi trait TransactionExists to TransactionFinder): it returns the Option<Transaction> the method then uses to extract the sender witnesses, so it is a lookup, not an exists predicate. Pre-1.0 (rc.3), so no deprecation shim is needed.
Collaborator
Coverage Report for CI Build 28235658451Coverage remained the same at 85.517%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #1656
& rename the predicate
transaction_existstofind_transaction, because it's not returning a bool it's returning a TxID.This change is done because broadcast is one possible interpretation of the success condition. Really, it's up to the wallet to determine at what point a transaction = a success for Payjoin, so the name reflects that.
But thinking about this revealed deeper problems (and some solutions to past problems) about the state machine design here which is that we're even making the check based on
TxIdin the first place because it limits detection to segwit transactions and we're just skipping detection otherwise (see #1214 , #1218). I don't see a good reason to force that disability to fit a particular monitor shape. There's another problem with this which is that our design treats detection from this method as the final verdict when even after broadcast detection of a payjoin it's still possible the fallback or another transaction double-spends the same outpoint used as receiver input.The non-segwit detection issue can be solved by monitoring sender and receiver input outpoints as well as the fallback & payjoin Txids (if predictable via segwit). The monitor would then return
SettlementStatus. See details below. This would let the wallet deal with reorgs and consistently return the correct SettlementStatus computed according to the wallet state, and wouldn't go stale by serializing event that's the result of a wallet snapshot as the permanent status of a particular payjoin, which is where this stands even after my proposal here.A proposal for payjoin-2.0 monitor design
The wallet reports raw facts; the payjoin library classifies
I doubt that this updated follow-up
Receiver<Monitor>change is critically necessary for 1.0. What we've shipped seems good enough for mobile wallets to get status. it seems like a critical necessity for automated exchange/service use cases to be robust enough to go prod, but ultimately I think it depends on how much bikeshedding the correct monitor requires and how much we're willing to push back an already long-passed deadline.Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.
Yes indeed this was co-authored by Claude Code