Make payjoin-cli v1 / v2 features additive by DanGould · Pull Request #538 · payjoin/rust-payjoin

DanGould · 2025-02-13T22:04:41Z

Following @nothingmuch's suggestion:

make appconfig more hierarchical, and support both v1 and v2 specific options in sub-structures, that are not feature gated. also move bitcoind related stuff to its own substructure

out of appconfig, generate helper stuff, again i was focused on a struct representing bitcoind (or more generally wallet) capabilities for payjoin, to avoid needing to impl the bitcoind method in the trait impls and to hide the details (e.g. error checking, psbt parsing, etc) from the actual App impls, optimizing readability to correspond high level description of the spec

adding an explicit --prefer-bip-78 or option, or something similar, for payjoin-cli compiled with both. due to the backwards compatibility of bip77 that probably requires more than one flag or something that is not just boolean valued, e.g. as a sender be able to send bip78 only even to a bip77 receiver and even if v2 support is compiled can be useful for e2e testing with a single build

I think we're much closer to @nothingmuch's goal:

my goal was making sure both app/v{1,2}.rs kind of give a high level overview of both the protocol and the code, de-emphasizing the details like how to talk to bitcoind and emphasizing its role as a reference implementation for bip 77 specifically.

Perhaps some network details could still be abstracted away to make this even more clear. But in terms of repaying tech debt and fixing the devex by making features additive, this should be complete.

Looking for a concept ACK

spacebear21

concept ACK, skimmed the changes and left some thoughts.

spacebear21 · 2025-02-13T22:31:43Z

payjoin-cli/src/app/v1.rs

        let mut interrupt = self.interrupt.clone();
        tokio::select! {
            res = self.start_http_server() => { res?; }
-            _ = interrupt.changed() => {


This is used a few times in v2.rs, we simply muted the warning with #[allow(clippy::incompatible_msrv)] over there.

and this change didn't fix it either. I'm not sure why the lint is showing up now but isn't on master. This code is in master, too

edit: Ah, lint uses --all-features (at the workspace level) and may have skipped v1 since it was not(features = "v2") before

payjoin-cli/src/app/config.rs

coveralls · 2025-02-16T02:14:35Z

Pull Request Test Coverage Report for Build 13426414002

Details

370 of 430 (86.05%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 79.298%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/main.rs	55	57	96.49%
payjoin-cli/src/app/v1.rs	24	31	77.42%
payjoin-cli/src/app/wallet.rs	112	119	94.12%
payjoin-cli/src/app/v2.rs	52	65	80.0%
payjoin-cli/src/app/config.rs	122	153	79.74%

Totals
Change from base Build 13418333922:	0.3%
Covered Lines:	4156
Relevant Lines:	5241

💛 - Coveralls

spacebear21

I have some usage notes before I dive deeper into the last commit (which makes the features additive):

It doesn't detect my v2 config when I attempt to run a v2 receiver (I think it doesn't parse the mode config with either version, we just happen to provide defaults for v1 so it doesn't cause an error).

# mode = { version = "v1", port = 3000, pj_endpoint = "https://localhost:3000" }
mode = { version = "v2", pj_directory = "https://payjo.in", ohttp_relay = "https://ohttp.achow101.com" }

[bitcoind]
rpcuser="payjoin"
rpcpassword="payjoin"
rpchost="http://localhost:18443/wallet/receiver"

❯ cargo run -- receive 1000
   Compiling payjoin-cli v0.0.9-alpha (/Users/spacebear/Projects/rust-payjoin/payjoin-cli)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.77s
     Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-cli receive 1000`
Error: V2 configuration is required for BIP77 mode

It does work if I pass the --ohttp-relay argument in the command.

Perhaps we should have a test covering config from file vs config from arguments?
Also, maybe we should set a default ohttp-relay if none is specified?

It doesn't compile with --no-default-features:

❯ cargo run --no-default-features -- receive 1000
   Compiling payjoin-cli v0.0.9-alpha (/Users/spacebear/Projects/rust-payjoin/payjoin-cli)
...
error: could not compile `payjoin-cli` (bin "payjoin-cli") due to 14 previous errors; 4 warnings emitted

If only the v1 feature is enabled, it tries to use BIP77 by default and fails. It should default to BIP78 if only v1 is enabled.

❯ cargo run --no-default-features --features=v1 -- receive 1000
   Compiling payjoin-cli v0.0.9-alpha (/Users/spacebear/Projects/rust-payjoin/payjoin-cli)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.68s
     Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-cli receive 1000`
Error: BIP77 (v2) support is not enabled in this build. Recompile with --features v2

spacebear21 · 2025-02-16T13:47:13Z

payjoin-cli/src/app/v1.rs

+        println!(
+            "Debug - Config: port={}, pj_endpoint={}",
+            self.config.v1.port, self.config.v1.pj_endpoint
+        );
+        println!("Debug - URI about to be printed: {}", pj_uri_string);


Leftover debug prints

payjoin-cli/example.config.toml

DanGould

Thanks very much for the thorough review on my push with limited testing. I think I've addressed your concerns.

It doesn't detect my v2 config

I left the example wrong. You can just use [v1] or [v2] sections as you specified and they'll be parsed into the mode struct of the config

It doesn't compile with --no-default-features

I don't think it should, since without v1 or v2 you have a completely useless binary. Perhaps a compile_error! macro message can address this concern.

If only the v1 feature is enabled, it tries to use BIP77 by default and fails. It should default to BIP78 if only v1 is enabled.

I fixed this with some switching of defaults in main.rs

payjoin-cli/example.config.toml

spacebear21 · 2025-02-17T19:00:25Z

I don't think it should, since without v1 or v2 you have a completely useless binary. Perhaps a compile_error! macro message can address this concern.

Yeah, there is a runtime check for this but it cannot be reached because it fails to compile. Alternatively I was able to reach it by requiring the "_core" feature in Cargo.toml:

payjoin = { version = "0.22.0", default-features = false, features = ["_core"] }

Then compiling with --no-default-features results in a bunch of warnings and:

     Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-cli receive 1000`
Error: No valid version available - must compile with v1 or v2 feature

payjoin-cli/src/app/config.rs

payjoin-cli/ohttp-keys

payjoin-cli/src/app/v1.rs

DanGould · 2025-02-18T21:25:42Z

I applied the compile_error! in cases where either payjoin or payjoin-cli would otherwise have no practical use.

Use version-specific substructures. This organization gets closer to additive payjoin-cli features.

Avoid inline comments by using helpers with docstrings instead.

Give an implementer an idea of the abstract functions the wallet needs to implement. This can also become an abstract class in bindings libraries to make it even easier to implement. Hide PSBT parsing, error handling, etc.

Make use of the AppTrait and a `--bip78` cli argument to allow both to be compiled into the same binary and a user to choose the protocol at runtime. Parse only one config version "mode" at runtime so that errors in a version unused at runtime will not cause errors. Use a determine_version fn in config so that the default behavior without an explicit version flag runs the highest version available. Use argument matches in main to reduce presedence if multiple versions are available.

Certain features must be present for the compilation to have any use at all.

DanGould · 2025-02-21T15:11:27Z

Rebased on master

spacebear21

ACK bfa282f

DanGould force-pushed the additive-cli-features branch from 743e363 to 14d595d Compare February 13, 2025 22:07

DanGould requested a review from spacebear21 February 13, 2025 22:07

DanGould force-pushed the additive-cli-features branch from 14d595d to f8494ec Compare February 13, 2025 22:17

spacebear21 reviewed Feb 13, 2025

View reviewed changes

DanGould force-pushed the additive-cli-features branch 2 times, most recently from 9a6d444 to cbd95a5 Compare February 16, 2025 02:09

DanGould requested a review from spacebear21 February 16, 2025 02:42

DanGould force-pushed the additive-cli-features branch from cbd95a5 to 0e46f80 Compare February 16, 2025 02:43

spacebear21 reviewed Feb 16, 2025

View reviewed changes

DanGould force-pushed the additive-cli-features branch from 0e46f80 to de4b03f Compare February 17, 2025 03:09

DanGould commented Feb 17, 2025

View reviewed changes

payjoin-cli/example.config.toml Outdated Show resolved Hide resolved

spacebear21 reviewed Feb 17, 2025

View reviewed changes

payjoin-cli/src/app/config.rs Show resolved Hide resolved

payjoin-cli/ohttp-keys Outdated Show resolved Hide resolved

spacebear21 reviewed Feb 17, 2025

View reviewed changes

payjoin-cli/src/app/v1.rs Outdated Show resolved Hide resolved

DanGould force-pushed the additive-cli-features branch from de4b03f to 15ee20b Compare February 18, 2025 21:14

DanGould requested a review from spacebear21 February 19, 2025 17:04

DanGould added 5 commits February 19, 2025 20:48

Separate v1, v2 App via Boxed dyn trait

1eb7be2

Make AppConfig hierarchical

a376880

Use version-specific substructures. This organization gets closer to additive payjoin-cli features.

Rename AppConfig app::Config

e18962e

Break up config into helper functions

9e1ad81

Avoid inline comments by using helpers with docstrings instead.

Abstract bitcoind functions into wallet mod

7ddcc58

Give an implementer an idea of the abstract functions the wallet needs to implement. This can also become an abstract class in bindings libraries to make it even easier to implement. Hide PSBT parsing, error handling, etc.

DanGould force-pushed the additive-cli-features branch from 340acea to bfa282f Compare February 20, 2025 02:14

DanGould added 2 commits February 19, 2025 21:14

Show compile_error if unusable feature combination is used

bfa282f

Certain features must be present for the compilation to have any use at all.

spacebear21 approved these changes Feb 25, 2025

View reviewed changes

spacebear21 merged commit 8777c16 into payjoin:master Feb 25, 2025
6 checks passed

spacebear21 mentioned this pull request Feb 25, 2025

refactor payjoin-cli App::bitcoind #478

Closed

spacebear21 mentioned this pull request Apr 2, 2025

Release payjoin-cli 0.1.0 #463

Closed

17 tasks

Conversation

DanGould commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13426414002

Details

💛 - Coveralls

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacebear21 commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanGould commented Feb 18, 2025

Uh oh!

DanGould commented Feb 21, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DanGould commented Feb 13, 2025 •

edited

Loading

DanGould Feb 13, 2025 •

edited

Loading

coveralls commented Feb 16, 2025 •

edited

Loading

spacebear21 commented Feb 17, 2025 •

edited

Loading