Make max fee rate sticky per payjoin-cli session

[Johnosezele]

When a payjoin-cli receive session is created, the max_fee_rate is now persisted with the session and reused when the session is resumed, instead of defaulting to the global configuration value.

Core Library (payjoin/src/core/receive/v2/mod.rs)

• Add max_fee_rate as non-optional field to SessionContext
• Remove session_max_fee_rate() accessor method (no longer needed after FFI simplification)
• Maintain all fee rate capping logic in apply_fee_range() method

FFI Layer (payjoin-ffi/src/receive/mod.rs)

• Simplify fee rate handling by removing duplicate capping logic
• Convert Option<u64> parameters to Option<FeeRate> and pass directly to core
• Eliminate architectural duplication between FFI and core layers

CLI Changes

• Persist max_fee_rate in session storage with backward compatibility
• Allow CLI override with --max-fee-rate flag when resuming sessions

Backward Compatibility

• Ensured through serde defaults - existing session files without max_fee_rate will use FeeRate::BROADCAST_MIN as default.

Resolves: #897

[coveralls]

Pull Request Test Coverage Report for Build 17264256919

Details

• 86 of 88 (97.73%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 85.902%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v2/mod.rs	74	76	97.37%

Totals
Change from base Build 17255673359:	0.1%
Covered Lines:	7994
Relevant Lines:	9306

---

💛 - Coveralls

[arminsabouri]

Thanks for taking this on! There is couple things here that don't seem quite right to me. I've provided some concrete suggestions to get this work on the right path.

[arminsabouri]

@Johnosezele Do you mind updating the ffi integration and unit tests? looks like you are just missing the new max-fee-rate param in those tests.

[arminsabouri]

This is definitely on the right track! Had a couple questions and misc. code clean up comments.

[Johnosezele]

@Johnosezele Do you mind updating the ffi integration and unit tests? looks like you are just missing the new max-fee-rate param in those tests.

Thanks, I have done this, the CI failure still persists looks like a test timeout issue

Edit:
I think reason for failure is CI environment is likely using pre-built Python bindings.

[arminsabouri]

The bindings should be generated on CI and this looks like a different error than before. I would try building the bindings locally and running the tests. I can also help debug later on today/tomorrow

[arminsabouri]

I noticed what I think is a regression in the python integration test. And I had a question about using min() vs a overwrite of previous fee prefrences. I plan to pull this down an run some manual tests.

One more thing @Johnosezele Do you mind squashing your commits into one. And adding a description for your commit. FWIW I think your PR description does a great job describing the problem and how you solved it -- although its a bit outdated. Do you mind adding something similar to the commit message. Thank you

[Johnosezele]

I see these weird conflicts with my branch and master, sadly I'm 'already up to date' with master on my local, so I'm not able to resolve these conflicts locally. When I resolve on GitHub I have some CI issues. @arminsabouri pls how do I proceed?

[arminsabouri]

I see these weird conflicts with my branch and master, sadly I'm 'already up to date' with master on my local, so I'm not able to resolve these conflicts locally. When I resolve on GitHub I have some CI issues. @arminsabouri pls how do I proceed?

Hmm thats odd. I pull master and rebased your branch. Can't say I had the same issue.

[arminsabouri]

@Johnosezele Last push cleans up the tests a bit further.

[arminsabouri]

Something odd I noticed. This certainly is outside the scope of this PR but wanted to note it

#[test]
    fn test_apply_fee_range_overrides_max() {
        let original_max = FeeRate::from_sat_per_vb_unchecked(1000);
        let context = SessionContext { max_fee_rate: original_max, ..SHARED_CONTEXT.clone() };
        let receiver = Receiver { state: create_wants_fee_range_with_context(context) };

        let new_max = Some(FeeRate::from_sat_per_vb_unchecked(2));
        let result = receiver
            .apply_fee_range(None, new_max)
            .save(&NoopSessionPersister::default())
            .expect("Noop persister shouldn't fail");

        let payjoin_psbt = &result.state.psbt_context.payjoin_psbt;
        let payjoin_fee = payjoin_psbt.fee().expect("PSBT should have fee");
        let new_max = new_max.expect("is some");
        let actual_fee_rate = payjoin_fee
            / payjoin_psbt
                .clone()
                .extract_tx_unchecked_fee_rate()
                .weight();

        println!("actual_fee_rate: {}", actual_fee_rate);

        assert!(
            actual_fee_rate <= original_max,
            "Fee rate {actual_fee_rate} should not exceed session maximum {original_max}"
        );
        assert!(
            actual_fee_rate <= new_max,
            "Fee rate {actual_fee_rate} should not exceed session maximum {new_max}"
        );
        assert_eq!(result.session_context.max_fee_rate, original_max);
    }

Fails bc it creates a tx with a FeeRate of 502 sat/kwu . It should be less than or equal to 500. When I set the new max fee rate to a large value (eg. 42) it passes. I will test but this should fail in master as well.

[Johnosezele]

The .clone() calls are required because create_session takes ownership of address, directory, and ohttp_keys. Since these variables are used later in the test, cloning is necessary to maintain test functionality. I think this is Rust ownership requirement. I was wrong about the .clone() calls being required for ownership. The variables remain usable after the function call, so .clone() isn't necessary.

[arminsabouri]

utACK 69eb26c

[arminsabouri]

Paging @nothingmuch as you requested changes

[nothingmuch]

ACK

minor nit: payjoin-cli/src/app/{main.rs,cli.mod} have some noop changes, namely Commmand::Resume is now an empty struct variant instead just an enum variant with no contents but i don't think it's worth reverting this if we expect a followup PR that intergrates this change into the CLI interface and would modify those lines anyway

[nothingmuch]

Paging @nothingmuch as you requested changes

since CLI stuff was postponed to later PR my suggestions my suggestions are no longer relevant for this PR

[Johnosezele]

I can see that lint failure of MaybeBadInitInputsTransition is because of its removal at 89dd32f#diff-18cf7bb103784dcb37dc0fec32cfa07512954cbf5ad6998d0c4df1dea1d73c21. Replacing with NextStateTransition as advised.

[Johnosezele]

Mostly lint fixes that I have just pushed compare

[arminsabouri]

I spotted regressions in ffi. Additionally please update your cargo nightly toolchain. I see some unnecessary clippy fixes.

[arminsabouri]

Suggested change

	.map_err(|e| {
	ImplementationError::from(format!(
	"Session creation failed for directory {}: {}",
	directory, e
	))
	})
	.map_err(IntoUrlError::from)

[arminsabouri]

cACK. There are still some cosmetic changes but I'd like to move this PR along. And I'd like to merge #998 first.

[DanGould]

Superseded by #1033