Rework receive::v2, send::v2 submodule into additive crate::v2 and crate::v1 submodules features
#392
Closed
5 tasks done
Labels
Milestone
Comments
DanGould
changed the title
receive::v2 submodule into crate::v2 and crate::v1 submodules feature gatedreceive::v2 submodule into additive crate::v2 and crate::v1 submodules features
DanGould
added a commit
that referenced
this issue
Jan 2, 2025
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: 1. housekeeping 2. Introduce specific SenderBuilder errors 3. make CreateRequestError v2 only since v1 can only make have sender builder errors 4. separate the modules to reduce feature flags 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
DanGould
changed the title
receive::v2 submodule into additive crate::v2 and crate::v1 submodules featuresreceive::v2, send::v2 submodule into additive crate::v2 and crate::v1 submodules features
|
@nothingmuch I believe making |
|
although i initially started with PjUriBuilder due to #416, i it's way up my "call stack", as it were, feel free to proceed with such changes, at worst it will be a minor disruption for my changes to payjoin-cli |
spacebear21
added a commit
that referenced
this issue
Jan 8, 2025
1. kills the PjUriBuilder pattern which was a feature gating hell
2. make separate receive::{v1,v2} modules to enable error separation and
additive use
3. Pass `supported_versions` from the calling module since that's where
version comes from
4. Explicitly removes non-additive not(feature = "v2") so integration
tests can all run on --all-features
We've got a ways to go to separate modules for readability, and then to
make the features in payjoin-cli additive, but we're getting there.
This is work toward #392
This can be followed by separating the error variants into distinct
receive, receive::v1, receive::v2 types and making the v1/v2 features in
payjoin-cli additive
DanGould
added a commit
that referenced
this issue
Jan 16, 2025
This big move isolates v1/v2 module-specific error variants into clean module v1/v2 error modules. There are probably too many `impl<From>` for error variants, but this addresses the module structure first with this PR and will be followed by taking a magnifying glass to the way specific variants are handled. This still also leaves `OutputSubstitutionError`, `SelectionError` and `InputContributionError` from producing JSON error responses to cancel a session with a v2 receiver. We're going to need to address that as part of #403. v2 Error variants also improperly produce JSON errors, but I think the way this was done by abusing `fmt::Display` is also an antipattern to fix at the same time that's addressed. I'd like to leave the JSON receiver error fixes to a follow up since what gets revealed is potentially sensitive and we should be reviewing that change with extra scrutiny. See: #312, #392, #403
DanGould
added a commit
to DanGould/rust-payjoin
that referenced
this issue
Jan 20, 2025
This way you can enable just the one you're interested in without the additional noise of details you will not use See payjoin#392
DanGould
added a commit
that referenced
this issue
Jan 20, 2025
- first patch closes #393 - second patch hard-closes #392 hard with an explicit `v1` feature - make `payjoin/v2` a default feature - Fix warning for module docs not found with conditional compilation - Lint previously un-linted v1 payjoin-cli/e2e tests - Close #499 (Configure v1 checks to use _danger-local-https) In making these changes I also considered an organizational structure that looks more like this: `payjoin::receive::{InputPair, Error...}` `payjoin::v1::receive`, `payjoin::v2::receive` But I think that's more trouble than it's worth, and also breaks the hierarchy that's most effective for producing and maintaining this software, so I'm inclined to leave it as is, leaving `send`, `receive` modules abstraction with version-specific submodule implementations. This makes it easier for us to effectuate #500
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I don't really love how the features are laid out inside receive especially. Send is not as big of a deal because the state machine has been flattened. If there were separate modules for separate features I think they'd be easier to reason about from docs.rs
Consider removing the
not(feature = v2)feature since we want to discourage its use and one can replicate v1 soverignty by running both a directory and payjoin client on the same machine. Rust features should be additive.send::{v1, v2}modules Abstract send feature error variants into separate feature-specific abstractions #464receivev1 typestate machine intoreceive::v1module with feature-gated visibilityreceive::{v1, v2}errors (related: Review usage ofcrate::receive::Error#312,) Separate receive::{v1,v2} error modules #482optional_parameters::SUPPORTED_VERSIONSaccording to calling module, not feature flag #490follow ups:
#500
The text was updated successfully, but these errors were encountered: