Add max_fee_rate() method to Config to encapsulate default

[codaMW]

See #1482

Adds a Config::max_fee_rate() method that encapsulates the
unwrap_or(FeeRate::BROADCAST_MIN) default, removing that detail
from the AppTrait impl and improving readability of the call site.

Pull Request Checklist

• I have disclosed my use of AI in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

Disclosure: I consulted Claude to understand the codebase structure,
but the solution was authored by myself.

[coveralls]

Coverage Report for CI Build 24809175609

Coverage remained the same at 84.953%

Details

• Coverage remained the same as the base build.
• Patch coverage: No coverable lines changed in this PR.
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	13371
Covered Lines:	11359
Line Coverage:	84.95%
Coverage Strength:	400.75 hits per line

---

💛 - Coveralls

[codaMW]

The Lint-FFI failure is a pre-existing clippy issue in payjoin-ffi unrelated to this PR. Opened #1491 to fix it separately.

[nothingmuch]

concept ACK

i would prefer if this PR went a bit further, and made things consistent between v1 and v2

i think there's three ways this could happen:

• either the new method could be removed and None passed in ensuring that everything is still correct with the hard coded min feerate in `payjoin::core::receive::common::WantsFeeRange::calculate_psbt_with_fee_range

• v1 CLI would provide this default for consistency with v2, and this would be redundant with the unwrap_or in calculate_psbt_with_fee_range, it's just that both CLI implementations never pass None

• we remove the Option wrapping of FeeRate in the payjoin api and require both v1 and v2

the first is the simplest, and i think my preference but it would need to be verified to ensure everything still behaves correctly, i didn't verify that it's sound, but max_fee_rate: None seems like there is no maximum enforced which can look wrong/scary.

the second is a bit meh.

the third is arguably the most misuse resistant, but breaks the API, and although more rigid since we treat None very conservatively and forcing wallet developers to pick a value explicitly may actually increase risk

i think my preference is the first option now that i know about this inconsistency and that the payjoin crate enforces the same default. perhaps there should be a comment in the payjoin-cli crate to clarify that when with_max_fee_rate
is given None it's actually a very low max?

another idea to clear that wrong connotation up: maybe that can be bikeshed to make it a bit better like enum OptionalFeerate { Some(FeeRate), MinRelayFee }

[nothingmuch]

i was not aware, but apparently v1 passes None all the way down and unwrap_or(FeeRate::BROADCAST_MIN) happens in payjoin/src/core/receive/common/mod.rs instead of in payjoin-cli

[codaMW]

Thanks for the context @nothingmuch. I verified that payjoin/src/core/receive/common/mod.rs already handles None with unwrap_or(FeeRate::BROADCAST_MIN) on both min and max, so passing None from v2 is safe and consistent with v1's existing behavior.
I'll go with option 1, remove the max_fee_rate() method and pass self.config.max_fee_rate directly from v2 (as Option), with a comment clarifying that None delegates the default to the payjoin crate. This makes v1 and v2 consistent.

[codaMW]

Updated. After checking the signatures, ReceiverBuilder::with_max_fee_rate() in v2 requires FeeRate directly (not Option), unlike v1's apply_fee_range which accepts Option and defaults to BROADCAST_MIN internally. So option 1 isn't directly applicable to v2 the unwrap_or must happen in the CLI layer.
Instead I've added a comment at the v2 call site explaining this API difference and why the explicit default is needed there but not in v1. The max_fee_rate() method has been removed from Config.

[nothingmuch]

ah right, if you don't call with_max_feerate it will stay None internally and then [edit: not sure what i was saying there i think just leftover from earlier comment] the lower level method will still receive an explicit value, given SessionContext is initialized to BROADCAST_MIN in ReceiverBuilder::new.

changing the default value imo is a breaking change, so perhaps the reasoning for this is not very strong as the default value is very unlikely to change without significant other changes, but it just feels really off to me to have it applied in 3 redundant places in the v2 code path, that's confusing.

it's not the prettiest, but consider:

if let Some(max_fee_rate) = self.config.max_fee_rate {
    receiver_builder = receiver_builder.with_max_fee_rate(max_fee_rate);
}

IMO that makes more sense than unwrapping with BROADCAST_MIN as a default, since then the default value entirely up to the payjoin crate, as it is in v1 app impl, which improves consistency.

that would bring the redundant default fallback count down to 2, once internally in calculate_psbt_with_fee_range, the only instance used in the v1 path and also used in the v2 path, and once in the SessionContext default value in ReceiverBuilder::new only for v2.

then in a followup PR, since calculate_psbt_context_with_fee_range is pub(super) it can be changed to require a concrete FeeRate instead of Option<FeeRate>, moving the responsibility for this fallback value up the call chain, that would leave one default value in SessionContext in the v2 code path, and one unwrap_or in v1 WantsFeeRange::apply_fee_range which should still accept Option.

i think this would be an improvement even though it's just shuffling code around, because in the v2 code path it's FeeRate, not Option<FeeRate> throughout, and there is more symmetry in where Option<FeeRate> transitions to just a FeeRate in both, simplifying the internal function signatures slightly.

this is very bikesheddy, so someone else should concept ACK this suggestion before you go ahead with this

[caarloshenriq]

so someone else should concept ACK this suggestion before you go ahead with this

Concept ACK on the if let approach.

Agreed that letting the payjoin crate own the default is the right call. It also brings v1 and v2 into alignment, which was the point.

If it’s helpful, I’m happy to take on the followup PR to update calculate_psbt_context_with_fee_range to require a concrete FeeRate instead of Option once this lands.

[codaMW]

Updated to use if let, with_max_fee_rate is only called when a value is explicitly configured, letting the payjoin crate own the default (BROADCAST_MIN in SessionContext). This is consistent with how v1 handles the absence of a configured max fee rate.

[caarloshenriq]

tACK b87f16b
Ran cargo test -p payjoin and cargo test -p payjoin-cli, all tests pass. Change is exactly what was discussed, with_max_fee_rate only called when explicitly configured, default ownership stays in the payjoin crate, consistent with v1.