Refactor config to use proc macros over builder by thebrandonlucas · Pull Request #703 · payjoin/rust-payjoin

thebrandonlucas · 2025-05-19T23:11:34Z

Refactors the code to use proc macros over the "legacy" builder method as per clap's recommendation.

The macros allow the code to be more maintainable, readable, and reusable moving forward.

See
here for more details on proc macro vs builder method rationales.

Discussion preceding this PR in #684 and here

thebrandonlucas · 2025-05-20T04:19:57Z

NOTE: This commit allowed the cli to pass multiple OHTTP relays as an arg so I needed to rebase + resolve conflicts.

thebrandonlucas · 2025-05-20T05:01:02Z

@DanGould I may need your advice with getting CI to pass. I ran ./contrib/test_local.sh and everything passed. I saw a failing CI, and figured I should run lint via ./contrib/lint.sh, which failed locally. I fixed that, reset my commit and re-committed (since each commit should pass CI), and force-pushed only to find CI broken again. I reread the README and am trying to apply what I think it's telling me to do.

I'm now running commands in this order:

nix develop                                   # for rust nightly shell
./payjoin-cli/contrib/test.sh       # I checked docker is running, all passes
./payjoin-cli/contrib/lint.sh        # passes
./payjoin-cli/contrib/rustfmt      # seems to hang infinitely?

I'm unsure what set of commands I have to run locally to be confident that CI will pass. I'll also note that it may be worthwhile to setup pre-commit hooks via something like husky for anything that we expect to happen before a commit, instead of relying on the developer to know which commands to use. Consistently pushing -> seeing a failing CI -> resetting + recommitting is tedious (I'm probably missing something and this isn't what others are having to do).

DanGould · 2025-05-20T16:17:01Z

FWIW e2e tests are failing for me locally, so CI appears to be sane.

./payjoin-cli/contrib/rustfmt # seems to hang infinitely?

There is no ./payjoin-cli/contrib/rustfmt file. Use rustfmt with cargo fmt making sure your cargo version matches CI.

./payjoin-cli/contrib/test.sh # I checked docker is running, all passes
./payjoin-cli/contrib/lint.sh

These may pass, but the top level test is ./contrib/test.sh and ./contrib/lint.sh and if any logic has changed those will probably fail / make changes.

it may be worthwhile to setup pre-commit hooks via something like husky for anything that we expect to happen before a commit

You can also use act or read the CI .yaml files to see exactly what's missing. It'd be tedious to replicate the full CI with all versions on local, so we've got to strike a balance between what's fast enough to iterate and then CI doing the most thorough check that could be manually triggered on your machine.

Even easier than adding a dep, just set up a local pre-commit git hook script. That's what I have on my local repo at .git/hooks/pre-commit:

#!/usr/bin/env bash
set -euo pipefail

# -------- 1. Rustfmt (nightly toolchain) --------
echo "▶  cargo +nightly fmt --check"
cargo +nightly fmt --all -- --check

# -------- 2. Project-specific linter --------
echo "▶  ./contrib/lint.sh"
./contrib/lint.sh

# -------- 3. Fast local test suite --------
echo "▶  ./contrib/test_local.sh"
./contrib/test_local.sh

echo "✓  Pre-commit hook passed"

thebrandonlucas · 2025-05-21T02:17:32Z

Thank you for the help. cargo fmt was indeed erroring and I had to fix that.

Interesting that you're failing CI locally, everything is still passing even when running the top-level ./contrib/test.sh or ./contrib/test_local.sh in the root directory. I ensured it's on nightly by running nix develop in the root, and then cargo -V gave me:

cargo 1.85.0-nightly (d73d2caf9 2024-12-31)

I'm also rebased with the latest commit: 3f0b6cc. If you are available sometime soon it may be helpful if we could pair to knock it out. In the meantime I'll pivot.

DanGould · 2025-05-21T17:08:03Z

1.85.0 is not the current nightly (which is 1.89.0 as of writing https://doc.rust-lang.org/nightly/nightly-rustc/cargo/)

The flake needs to be kept up to date and has many open issues. It's apparently not a reliable dev environment yet.

coveralls · 2025-05-27T20:17:52Z

Pull Request Test Coverage Report for Build 15351670294

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

113 of 127 (88.98%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.3%) to 84.288%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/config.rs	80	86	93.02%
payjoin-cli/src/cli/mod.rs	24	32	75.0%

Totals
Change from base Build 15306099048:	-0.3%
Covered Lines:	6400
Relevant Lines:	7593

💛 - Coveralls

thebrandonlucas · 2025-05-27T20:32:55Z

payjoin-cli/src/app/config.rs

+        if cli.flags.bip78.unwrap_or(false) {
+            if selected_version.is_some() {
+                return Err(ConfigError::Message(
+                    "Multiple version flags specified. Please use only one of: --bip77, --bip78"


I aimed to not change any language or behavior here, but the difference between the v1/v2 features and bip77/bip78 would be helpful. Is bip77 a "version" flag? If not, I can update the language in another PR.

thebrandonlucas · 2025-05-27T20:38:39Z

payjoin-cli/src/app/config.rs

+    // Set default values
+    let config = config
        .set_default("bitcoind.rpchost", "http://localhost:18443")?
-        .set_override_option(
-            "bitcoind.rpchost",
-            matches.get_one::<Url>("rpchost").map(|s| s.as_str()),
-        )?
        .set_default("bitcoind.cookie", None::<String>)?
-        .set_override_option(
-            "bitcoind.cookie",
-            matches.get_one::<String>("cookie_file").map(|s| s.as_str()),
-        )?
        .set_default("bitcoind.rpcuser", "bitcoin")?
-        .set_override_option(
-            "bitcoind.rpcuser",
-            matches.get_one::<String>("rpcuser").map(|s| s.as_str()),
-        )?
-        .set_default("bitcoind.rpcpassword", "")?
-        .set_override_option(
-            "bitcoind.rpcpassword",
-            matches.get_one::<String>("rpcpassword").map(|s| s.as_str()),
-        )
+        .set_default("bitcoind.rpcpassword", "")?;


The missing defaults were what was causing the test failures before, so the tests caught a behavioral change I made. It didn't really make sense to me to provide defaults for these since rpcuser and rpcpassword don't have sensible defaults the way rpchost does, so it felt weird to give one here. I can take a look at refactoring these out of the tests later if we want.

good issue to follow up with. The way you solved it for now works since it doesn't regress.

thebrandonlucas · 2025-05-27T20:41:02Z

@DanGould These are passing CI now

DanGould

Neat and tidy. So good to see this pass.

May you please add a commit that uses the Version enum for the determine_version function, and another to remove the unwrap() there in the return that could panic. This is my first pass review to unblock progress on this, but I want to take a closer second pass over the use of feature flags and shorthand rpc names to make sure everything matches up. I think our tests have very good coverage for the feature flags at least, and don't forsee problems on the second pass.

payjoin-cli/src/app/config.rs

DanGould · 2025-05-28T15:39:52Z

payjoin-cli/src/app/config.rs

+    // Set default values
+    let config = config
        .set_default("bitcoind.rpchost", "http://localhost:18443")?
-        .set_override_option(
-            "bitcoind.rpchost",
-            matches.get_one::<Url>("rpchost").map(|s| s.as_str()),
-        )?
        .set_default("bitcoind.cookie", None::<String>)?
-        .set_override_option(
-            "bitcoind.cookie",
-            matches.get_one::<String>("cookie_file").map(|s| s.as_str()),
-        )?
        .set_default("bitcoind.rpcuser", "bitcoin")?
-        .set_override_option(
-            "bitcoind.rpcuser",
-            matches.get_one::<String>("rpcuser").map(|s| s.as_str()),
-        )?
-        .set_default("bitcoind.rpcpassword", "")?
-        .set_override_option(
-            "bitcoind.rpcpassword",
-            matches.get_one::<String>("rpcpassword").map(|s| s.as_str()),
-        )
+        .set_default("bitcoind.rpcpassword", "")?;


good issue to follow up with. The way you solved it for now works since it doesn't regress.

payjoin-cli/src/cli/mod.rs

Refactors the code to use proc macros over the "legacy" builder method as per `clap`'s recommendation. The macros allow the code to be more maintainable, readable, and reusable moving forward. See [here](https://docs.rs/clap/latest/clap/_faq/index.html#when-should-i-use-the-builder-vs-derive-apis) for more details on proc macros vs builder method rationales. Discussion preceding this PR in payjoin#684 and [here](#1)

Use the `Version` enum for the `determine_version` function instead of `u8`'s.

Removes `return Ok(selected_version.unwrap())` in favor of short circuiting to check if the version is `Some()` explicitly to avoid potential panics caused by the `.unwrap()`.

DanGould · 2025-05-29T21:50:05Z

payjoin-cli/src/main.rs

+            let fee = fee_rate
+                .unwrap_or_else(|| FeeRate::from_sat_per_vb(2).unwrap_or(FeeRate::BROADCAST_MIN));


Is this a new default? It also seems like from_sat_per_vb(2) unwraps the Broadcast min (which will never get hit. I don't think we want this behavior change.

This should error if it's missing, not default.

DanGould · 2025-05-29T21:52:16Z

Getting failure running

RUST_LOG=debug cargo run --bin payjoin-cli -- --ohttp-relays="https://pj.benalleng.com,https://pj.bobspacebkk.com" receive 22222

which results in

[2025-05-29T21:52:47Z DEBUG payjoin_cli::app::config] App config: Config { db_path: "payjoin.sled", max_fee_rate: Some(FeeRate(2)), bitcoind: BitcoindConfig { rpchost: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(38332), path: "/wallet/alice", query: None, fragment: None }, cookie: Some("/Users/dan/f/Mutinynet/signet/.cookie"), rpcuser: "bitcoin", rpcpassword: "" }, version: Some(V2(V2Config { ohttp_keys: None, ohttp_relays: [Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("pj.benalleng.com,https")), port: None, path: "//pj.bobspacebkk.com", query: None, fragment: None }], pj_directory: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("payjo.in")), port: None, path: "/", query: None, fragment: None } })) }
[2025-05-29T21:52:47Z DEBUG sled::pagecache::iterator] ordering before clearing tears: {0: 0}, max_header_stable_lsn: 0
[2025-05-29T21:52:47Z DEBUG sled::pagecache::iterator] in clean_tail_tears, found missing item in tail: None and we'll scan segments {0: 0} above lowest lsn 0
[2025-05-29T21:52:47Z DEBUG sled::pagecache::iterator] filtering out segments after detected tear at (lsn, lid) 7892
[2025-05-29T21:52:47Z DEBUG sled::pagecache::iterator] hit max_lsn 7892 in iterator, stopping
[2025-05-29T21:52:47Z DEBUG sled::pagecache::snapshot] zeroing the end of the recovered segment at lsn 0 between lids 7893 and 524287
[2025-05-29T21:52:47Z DEBUG sled::pagecache::blob_io] gc_blobs removing any blob with an lsn above 7893
[2025-05-29T21:52:47Z DEBUG sled::pagecache::segment] SA starting with tip 524288 stable -1 free {}
[2025-05-29T21:52:47Z DEBUG sled::pagecache::iobuf] starting log at recovered active offset 7893, recovered lsn 7893
[2025-05-29T21:52:47Z DEBUG sled::pagecache::iobuf] starting IoBufs with next_lsn: 7893 next_lid: 7893
[2025-05-29T21:52:47Z DEBUG sled::pagecache] load_snapshot loading pages from 0..6
[2025-05-29T21:52:47Z DEBUG bitcoincore_rpc] JSON-RPC request: getblockchaininfo []
[2025-05-29T21:52:47Z DEBUG bitcoincore_rpc] JSON-RPC request: getnetworkinfo []
[2025-05-29T21:52:47Z DEBUG bitcoincore_rpc] JSON-RPC request: getnewaddress [null,null]
[2025-05-29T21:52:47Z DEBUG bitcoincore_rpc] JSON-RPC request: getblockchaininfo []
[2025-05-29T21:52:47Z DEBUG bitcoincore_rpc] JSON-RPC request: getnetworkinfo []
Bootstrapping private network transport over Oblivious HTTP
[2025-05-29T21:52:47Z DEBUG reqwest::connect] starting new connection: https://payjo.in/
[2025-05-29T21:52:47Z DEBUG reqwest::connect] proxy(https://pj.benalleng.com,https) intercepts 'https://payjo.in/'
[2025-05-29T21:52:47Z DEBUG payjoin_cli::app::v2::ohttp] Failed to connect to relay: https://pj.benalleng.com,https//pj.bobspacebkk.com, Internal(InternalError(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("payjo.in")), port: None, path: "/.well-known/ohttp-gateway", query: None, fragment: None }, source: hyper_util::client::legacy::Error(Connect, Custom { kind: Other, error: "invalid dnsname" }) })))
[2025-05-29T21:52:47Z DEBUG sled::pagecache::logger] IoBufs dropped
Error: No valid relays available

from this tip which otherwise succeeds on master (16f0c20)

Edit: It wouldn't hurt to include a failure ohttp-relay to test this behavior in e2e test by passing the valid url & "https://example.com"

DanGould · 2025-05-29T22:07:04Z

payjoin-cli/src/cli/mod.rs

+    #[arg(long = "ohttp-relays", help = "One or more ohttp relay URLs, comma-separated", value_parser = value_parser!(Url))]
+    pub ohttp_relays: Option<Vec<Url>>,


Suggested change

#[arg(long = "ohttp-relays", help = "One or more ohttp relay URLs, comma-separated", value_parser = value_parser!(Url))]

pub ohttp_relays: Option<Vec<Url>>,

#[arg(long = "ohttp-relays", help = "One or more ohttp relay URLs, comma-separated", value_parser = value_parser!(Url), value_delimiter = ',', action = clap::ArgAction::Append)]

pub ohttp_relays: Option<Vec<Url>>,

I think this returns to previously working behavior

DanGould

Thanks for the updates. I did a second thorough pass of each and every line, checking help strings etc. I think there are 2 more changes to be made.

Don't default a minfeerate. Especially not of 2 sat/vB. That'll have people spending more than they anticipate. Please error as was the prior behavior.
Add my suggestion and verify that enumerated ohttp-relays separated by commas work in --ohttp-relays like how cargo --features works.

DanGould · 2025-05-29T22:11:25Z

payjoin-cli/src/cli/mod.rs

+    #[arg(
+        long = "cookie-file",
+        short = 'c',
+        num_args(1),


I think num_args(1) is default behavior for ArgActions::Set and may be elided in a follow up PR. Thanks for keeping it to duplicatae the behavior for now.

DanGould · 2025-05-29T22:11:41Z

payjoin-cli/src/cli/mod.rs

+        long,
+        short = 'r',
+        num_args(1),
+        help = "The URL of the Bitcoin RPC host, e.g. regtest default is http://localhost:18443"


good help clarification here.

Passing multiple `ohttp-relays` from the CLI was failing due to missing `value_delimiter = ','` and `clap::ArgAction::Append` to the proc macro.

`--fee-rate` was given a default when it shouldn't have been. This reverts that change.

thebrandonlucas · 2025-05-30T17:00:17Z

Thanks for the feedback and tips @DanGould. I've implemented those final changes and tested them:

Passing without --fee-rate reverts to erroring, e.g.:

RUST_LOG=debug cargo run --bin payjoin-cli -- --ohttp-relays="https://pj.benalleng.com" send "bitcoin:bcrt1qhg9dsqcmle93ek7wlr7vqhgl9vsd0rek4ufclc?amount=0.00022222&pj=HTTPS://PAYJO.IN/K0ANSDFKYDAWX%23RK1QDL98KV7FZKYL7EYDQMWHR25ZMMFSTY82KWD5YVDKLGDZNY83JWDK+OH1QYP87E2AVMDKXDTU6R25WCPQ5ZUF02XHNPA65JMD8ZA2W4YRQN6UUWG+EX1556RK6Q"
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.24s
     Running `/home/blu/src/rust-payjoin/target/debug/payjoin-cli '--ohttp-relays=https://pj.benalleng.com' send 'bitcoin:bcrt1qhg9dsqcmle93ek7wlr7vqhgl9vsd0rek4ufclc?amount=0.00022222&pj=HTTPS://PAYJO.IN/K0ANSDFKYDAWX%23RK1QDL98KV7FZKYL7EYDQMWHR25ZMMFSTY82KWD5YVDKLGDZNY83JWDK+OH1QYP87E2AVMDKXDTU6R25WCPQ5ZUF02XHNPA65JMD8ZA2W4YRQN6UUWG+EX1556RK6Q'`
error: The following required arguments were not provided:
  --fee-rate <FEE_RATE>

Usage: payjoin-cli send --fee-rate <FEE_RATE> <BIP21>

Passing in multiple ohttp-relays, comma-separated as you did, works with your suggestion (as does only passing one).

DanGould

tACK c666882

OK for this time but next time fixes within a PR must amend the commit they fix. It doesn't make sense to commit mistakes and then follow with "Fix" commits in the same PR when you still have the power to make a clean history.

You can do this with minimal intrusion using git rebase -i <branch> and then fixup (or f for short) the commits into the relevant parent where the mistakes were made.

thebrandonlucas · 2025-05-31T21:49:23Z

tACK c666882

OK for this time but next time fixes within a PR must amend the commit they fix. It doesn't make sense to commit mistakes and then follow with "Fix" commits in the same PR when you still have the power to make a clean history.

You can do this with minimal intrusion using git rebase -i <branch> and then fixup (or f for short) the commits into the relevant parent where the mistakes were made.

Thanks for the review. Will do in future.

thebrandonlucas requested a review from DanGould May 19, 2025 23:11

This was referenced May 19, 2025

Add derive macros to cli config #684

Closed

Refactor config builder to derive macros thebrandonlucas/rust-payjoin#1

Closed

thebrandonlucas marked this pull request as draft May 20, 2025 00:13

thebrandonlucas force-pushed the refactor-builder-to-derive branch from 3bad990 to 0c78caa Compare May 20, 2025 04:12

thebrandonlucas force-pushed the refactor-builder-to-derive branch from 0c78caa to 5cf73aa Compare May 20, 2025 04:39

thebrandonlucas force-pushed the refactor-builder-to-derive branch 2 times, most recently from f9b7ac3 to 2709877 Compare May 20, 2025 23:43

thebrandonlucas force-pushed the refactor-builder-to-derive branch from 49fe8d8 to f8766de Compare May 27, 2025 20:13

thebrandonlucas commented May 27, 2025

View reviewed changes

thebrandonlucas marked this pull request as ready for review May 27, 2025 20:40

DanGould requested changes May 28, 2025

View reviewed changes

DanGould reviewed May 28, 2025

View reviewed changes

payjoin-cli/src/cli/mod.rs Show resolved Hide resolved

thebrandonlucas force-pushed the refactor-builder-to-derive branch from f8766de to c75d61d Compare May 29, 2025 11:05

thebrandonlucas added 3 commits May 29, 2025 08:21

Use Version enum

4afdaaf

Use the `Version` enum for the `determine_version` function instead of `u8`'s.

Remove unwrap() in determine_version

5537853

Removes `return Ok(selected_version.unwrap())` in favor of short circuiting to check if the version is `Some()` explicitly to avoid potential panics caused by the `.unwrap()`.

thebrandonlucas force-pushed the refactor-builder-to-derive branch from 123cd1c to 5537853 Compare May 29, 2025 12:27

thebrandonlucas changed the title ~~Refactor config to use derive macros over builder~~ Refactor config to use proc macros over builder May 29, 2025

thebrandonlucas requested a review from DanGould May 29, 2025 16:42

DanGould reviewed May 29, 2025

View reviewed changes

DanGould requested changes May 29, 2025

View reviewed changes

thebrandonlucas added 2 commits May 30, 2025 12:22

Fix passing multiple ohttp-relays in CLI flag

51bf7ab

Passing multiple `ohttp-relays` from the CLI was failing due to missing `value_delimiter = ','` and `clap::ArgAction::Append` to the proc macro.

Make --fee-rate required again

c666882

`--fee-rate` was given a default when it shouldn't have been. This reverts that change.

thebrandonlucas requested a review from DanGould May 30, 2025 17:00

DanGould approved these changes May 31, 2025

View reviewed changes

DanGould merged commit 0d76068 into payjoin:master Jun 1, 2025
7 checks passed

thebrandonlucas mentioned this pull request Jun 10, 2025

Better codify contribution rules via CONTRIBUTING.md and create issue templates #758

Closed

		let fee = fee_rate
		.unwrap_or_else(\|\| FeeRate::from_sat_per_vb(2).unwrap_or(FeeRate::BROADCAST_MIN));

		#[arg(long = "ohttp-relays", help = "One or more ohttp relay URLs, comma-separated", value_parser = value_parser!(Url))]
		pub ohttp_relays: Option<Vec<Url>>,

Conversation

thebrandonlucas commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thebrandonlucas commented May 20, 2025

Uh oh!

thebrandonlucas commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented May 20, 2025

Uh oh!

thebrandonlucas commented May 21, 2025

Uh oh!

DanGould commented May 21, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 15351670294

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

thebrandonlucas May 27, 2025

Choose a reason for hiding this comment

Uh oh!

thebrandonlucas May 27, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould May 28, 2025

Choose a reason for hiding this comment

Uh oh!

thebrandonlucas commented May 27, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanGould May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanGould May 29, 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!

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

Uh oh!

Uh oh!

DanGould May 29, 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!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould May 29, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould May 29, 2025

Choose a reason for hiding this comment

Uh oh!

thebrandonlucas commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould left a comment • 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!

thebrandonlucas commented May 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thebrandonlucas commented May 19, 2025 •

edited

Loading

thebrandonlucas commented May 20, 2025 •

edited

Loading

coveralls commented May 27, 2025 •

edited

Loading

DanGould May 29, 2025 •

edited

Loading

DanGould commented May 29, 2025 •

edited

Loading

DanGould May 29, 2025 •

edited

Loading

thebrandonlucas commented May 30, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading