Extend tests for rest of receive module by benalleng · Pull Request #632 · payjoin/rust-payjoin

benalleng · 2025-04-02T20:20:44Z

This expands the mutant coverage for the rest of the receive module and expands the mutant job coverage over the entire receive module.

Unfortunately the SHARED_CONTEXT cannot reasonably be moved into test_utils as the external nature of this struct would then cause errors as the type would be of payjoin::SessionContext not SessionContext

coveralls · 2025-04-02T20:23:54Z

Pull Request Test Coverage Report for Build 14390878927

Details

68 of 69 (98.55%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 81.794%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v2/mod.rs	17	18	94.44%

Totals
Change from base Build 14387071460:	0.2%
Covered Lines:	5324
Relevant Lines:	6509

💛 - Coveralls

DanGould

I'd want @0xBEEFCAF3 to check out the multiparty before ACK.

I left some comments on one issue that for sure needs fixing and another that'd be nice.

Crushing deez receive mutants 🥾

DanGould · 2025-04-10T03:18:32Z

payjoin/src/receive/v2/mod.rs

+    fn maximum_expiry() {
+        let session = NewReceiver::new(
+            SHARED_CONTEXT.address.clone(),
+            SHARED_CONTEXT.directory.clone(),
+            SHARED_CONTEXT.ohttp_keys.clone(),
+            None,
+        );
+        let session_expiry =
+            session.unwrap().context.expiry.duration_since(SystemTime::now()).unwrap().as_secs();
+        let maximum_expiry = Duration::from_secs(86400);
+        if let Some(expected_expiry) = SystemTime::checked_add(&SystemTime::now(), maximum_expiry) {
+            assert_eq!(TWENTY_FOUR_HOURS_DEFAULT_EXPIRY, maximum_expiry);
+            assert_eq!(
+                session_expiry,
+                expected_expiry.duration_since(SystemTime::now()).unwrap().as_secs()
+            );
+        }
+    }


This test does not pass in my environment, perhaps because some time may pass between calls of SystemTime::now().

thread 'receive::v2::test::maximum_expiry' panicked at payjoin/src/receive/v2/mod.rs:724:13: assertion `left == right` failed left: 86399 right: 86400

I think we can use #[tokio::test(start_paused = true)] to pause time so the test is more consistent.

Next, checked_add call is a bit odd if it's using &self. Prefer SystemTime::now().checked_add(maximum_expiry)

When I tested this I also got an issue compiling with the start_paused change which made me bump tokio. Seems like 1.38.1 is 1.63 MSRV. Turned out the bump wasn't necessary but the tokio/test-util feature was. Not a bad time to bump to 1.38.1 in a separate commit, tho. Don't forget to update lock files in the commit(s) that change(s) Cargo.toml.

Lastly I'm not sure why thi stest is named maximum expiry. Isn't it default? Couldn't it be set to be longer?

Ok! Need to spend more time learning how to use tokio

DanGould · 2025-04-10T03:21:54Z

payjoin/src/receive/multiparty/mod.rs

+    fn multiparty_proposal_from_test_vector() -> v1::UncheckedProposal {
+        let pairs = url::form_urlencoded::parse("v=2&optimisticmerge=true".as_bytes());
+        let params =
+            Params::from_query_pairs(pairs, &[2]).expect("Could not parse from query pairs");


I stared at this &[2] for a while wondering what it was. Makes me wonder if we ought to have a Version enum.

DanGould · 2025-04-10T03:23:29Z

payjoin/src/receive/multiparty/mod.rs

+
+    use std::any::{Any, TypeId};
+
+    use payjoin_test_utils::{BoxError, PARSED_ORIGINAL_PSBT};


Doesn't seem like a big reason to import and use BoxError or results if the functions return Ok(()) and the ? isn't used anywhere.

going to keep these in favor of using some ? instead of only expect()

arminsabouri

Ack on the multi party front. 47d075f
Just had a couple small comments, no show stoppers.

Its great to see some test coverage on the proposal builders. The tests added are actually testing what I would classify as an edge case where we are building multi party proposal with duplicate inner proposals. In a follow up PR we should fix this and then follow up with some tests on the happy path as well (unique proposals with different hpke sessions).

Thanks for adding these 🚀

arminsabouri · 2025-04-10T13:11:13Z

payjoin/src/receive/multiparty/mod.rs

+        let unchecked_proposal = multiparty.build();
+        assert!(
+            unchecked_proposal.expect("Could not build multiparty proposal").contexts.len() == 2
+        );


This is working as expected but I do wonder if contexts here should be len == 1. Since both the hpke context and the proposal is the same it would make sense to me that UncheckedProposalBuilder.add() would disallow the caller to add duplicate sessions. I still think the ns1r session would succeed bc only one PSBT ends up on the directory for this hpke session but it still seems wrong and error prone. This is outside the scope of this PR.

Issue created: #640

arminsabouri · 2025-04-10T13:11:40Z

payjoin/src/receive/multiparty/mod.rs

+            .expect("Could not add propsal to multiparty");
+
+        finalized_multiparty.add(proposal_two).expect("Could not add propsal to multiparty");
+        assert_eq!(finalized_multiparty.v2()[0].type_id(), TypeId::of::<v2::UncheckedProposal>());


Mind as well check the second proposal as well.

Suggested change

assert_eq!(finalized_multiparty.v2()[0].type_id(), TypeId::of::<v2::UncheckedProposal>());

assert_eq!(finalized_multiparty.v2()[0].type_id(), TypeId::of::<v2::UncheckedProposal>());

assert_eq!(finalized_multiparty.v2()[1].type_id(), TypeId::of::<v2::UncheckedProposal>());

arminsabouri · 2025-04-10T13:14:59Z

payjoin/src/receive/multiparty/mod.rs

+        assert!(
+            unchecked_proposal.expect("Could not build multiparty proposal").contexts.len() == 2
+        );
+        Ok(())


It any case we should also have a test where we add different PSBTs with different sessions as this is the happy path. This can also be a follow up!

arminsabouri · 2025-04-10T13:16:43Z

payjoin/src/receive/multiparty/mod.rs

+        multiparty.add(proposal_one).expect("Could not add proposal to multiparty");
+        multiparty.add(proposal_two).expect("Could not add proposal to multiparty");


Can we use ? here instead of expect?

benalleng · 2025-04-10T13:29:41Z

In a follow up PR we should fix this and then follow up with some tests on the happy path as well (unique proposals with different hpke sessions).

Yeah that's definitely something I am trying to keep in mind, Mutant catches rarely seem to be easily tested with the happy path when they can't be caught with a simple assert.

DanGould

Nice changes, But the feature addition needs to be fixup!ed into the first commit or else the first commit will not compile

Please also change the second commit to change all tokio = <version> definitions to match the update.

DanGould · 2025-04-10T18:30:07Z

I'm still getting the local failure despite CI passing and tokio start_paused. Going to have to troubleshoot hmm

running 1 test
test receive::v2::test::default_expiry ... FAILED

successes:

successes:

failures:

---- receive::v2::test::default_expiry stdout ----

thread 'receive::v2::test::default_expiry' panicked at payjoin/src/receive/v2/mod.rs:725:13:
assertion `left == right` failed
  left: 86399
 right: 86400

benalleng · 2025-04-10T19:06:09Z

@DanGould give this recent one a shot, I tried just initializing let now = SystemTime::now() once and call it several times so maybe this will be more consistent

DanGould · 2025-04-10T20:40:51Z

This works! However it no longer depends on tokio::test(start_paused = true), so please one more change to switch back to #[test] and shut the tokio feature off. The tokio should stay, good to bring deps up to date.

This expands the coverage for the rest of the receive module and gives full mutant coverage for all the functions inside of receive.

DanGould

~~ACK 180f3084d2666fff6a6c0500063bb28266e6a80b~~

whoops, that's the rebase on master

ACK 9be9dc2

benalleng force-pushed the extend-receive-tests branch from 85cdc08 to 4fe7eb2 Compare April 2, 2025 20:48

benalleng marked this pull request as draft April 2, 2025 20:59

This comment was marked as resolved.

Sign in to view

benalleng force-pushed the extend-receive-tests branch from 4fe7eb2 to 47d075f Compare April 3, 2025 14:19

benalleng marked this pull request as ready for review April 3, 2025 14:29

DanGould reviewed Apr 10, 2025

View reviewed changes

arminsabouri approved these changes Apr 10, 2025

View reviewed changes

arminsabouri mentioned this pull request Apr 10, 2025

NS1R proposal builder should not accept duplicate proposal #640

Closed

benalleng force-pushed the extend-receive-tests branch from 47d075f to c0a9e16 Compare April 10, 2025 16:41

benalleng mentioned this pull request Apr 10, 2025

Duplicate proposals catch in ns1r #642

Merged

DanGould mentioned this pull request Apr 10, 2025

Make payjoin version enum, even if only for internal use #643

Closed

DanGould requested changes Apr 10, 2025

View reviewed changes

benalleng force-pushed the extend-receive-tests branch from c0a9e16 to 77fd801 Compare April 10, 2025 18:07

benalleng force-pushed the extend-receive-tests branch from 77fd801 to cbd6f3a Compare April 10, 2025 19:04

benalleng added 2 commits April 10, 2025 17:40

Extend tests for rest of receive module

a5be8fd

This expands the coverage for the rest of the receive module and gives full mutant coverage for all the functions inside of receive.

bump tokio to 1.38.1

9be9dc2

benalleng force-pushed the extend-receive-tests branch from cbd6f3a to 9be9dc2 Compare April 10, 2025 21:42

DanGould approved these changes Apr 14, 2025

View reviewed changes

DanGould merged commit 361c31e into payjoin:master Apr 14, 2025
7 checks passed


		use std::any::{Any, TypeId};

		use payjoin_test_utils::{BoxError, PARSED_ORIGINAL_PSBT};

		multiparty.add(proposal_one).expect("Could not add proposal to multiparty");
		multiparty.add(proposal_two).expect("Could not add proposal to multiparty");

Conversation

benalleng commented Apr 2, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 14390878927

Details

💛 - Coveralls

Uh oh!

This comment was marked as resolved.

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould Apr 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented Apr 10, 2025

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!

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

Uh oh!

Uh oh!

benalleng commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Apr 10, 2025

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Apr 2, 2025 •

edited

Loading

DanGould Apr 10, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

DanGould commented Apr 10, 2025 •

edited

Loading

benalleng commented Apr 10, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading