Separate send version modules (part 1)#453
Merged
DanGould merged 4 commits intopayjoin:masterfrom Jan 2, 2025
Merged
Conversation
Collaborator
Pull Request Test Coverage Report for Build 12586246418Details
💛 - Coveralls |
DanGould
force-pushed
the
separate-send-versions-p1
branch
from
6f7cae1 to
fb946e9
Compare
nothingmuch
approved these changes
Jan 2, 2025
Collaborator
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
3rd commit message has a typo ("it's" instead of "its")
i did not carefully scrutinize the big moves so may have missed small differences if those were introduced
everything else looks good to me, and there is some overlap with stuff i haven't PR'ed yet which is a good indication that we have the same ideas for how the payjoin-cli code can be improved
This error is the first one that appears in the state machine.
SenderBuilder was using vestigial CreateRequestError even though it was not creating any requests. It only produces errors when building Sender. Define a BuildSenderError and Internal counterpart separate from CreateRequestError. Use best practice `new` instead of `from_request_and_uri`, and provide an accurate docstring.
DanGould
force-pushed
the
separate-send-versions-p1
branch
from
fb946e9 to
8480c3c
Compare
Its variants only apply to extracting v2 requests.
This reduces the number of feature gates and works toward a crate where the features are additive and potentially both available at the same time. Right now they're still mutually exclusive (v1 is enabled by not(v2))
Contributor
Author
|
Rebased on master and commit message amended to fix spelling error. Going to merge if tests pass |
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
3 participants
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.
The goal of this PR is to expose descriptive, accurate send errors so that we can switch on them in implementations and mark payjoin sessions in persistent storage accurately. We only want to spawn sessions that are capable of making forward progress and drop those that cannot.
Patch overview:
This separation is incomplete because I'm still uncertain what to do with send::ValidationError's feature gated variants. Do I make a send::ValidationError and a send::v2::ValidationError separate? How do those get handled in ResponseError? Does ResponseError get split into two versions, or do I just leave a feature gated variant? That's left for the next PR predicated on this design being an appropriate one.
Note this pays back some tech debt but leaves some slop in payjoin-cli. Rather than making this PR 10 commits to review I left combining FeeRate parsing to a later PR (#452).
This error puts us on the path to #392 and #403