Replace SystemTime with bitcoin::absolute::Time

[nothingmuch]

Internally, introduce a newtype wrapping bitcoin::absolute::Time (which
is what BIP 77 specifies) instead of using std::time::SystemTime.

A fallible constructor Time::from_now(std::time::Duration) is used to
construct these values from a Duration, which is indirectly called by
with_expiration. No IntoTime trait analogous to IntoUrl is included in
this commit, but this could be added there as long as both the
re-exported and the new Time type implement it.

Introducing a newtype lets us add additional conversion and arithmetic
functionality later without breaking the API or relying on extension
traits on the bitcoin Time type. Since it is now only pub(crate), the
newtype wrapper's existence is now not entirely justified, as this no
longer re-exports a foreign type in our pub api (previous iterations of
the commit made it pub, replacing the use of Duration).

Replaces #1021

Depends on #1082

Pull Request Checklist

Please confirm the following before requesting review:

• 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.

[coveralls]

Pull Request Test Coverage Report for Build 17830680632

Details

• 110 of 123 (89.43%) changed or added relevant lines in 7 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.04%) to 84.625%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v2/mod.rs	28	29	96.55%
payjoin/src/core/send/v2/error.rs	1	2	50.0%
payjoin/src/core/time.rs	28	39	71.79%

Totals
Change from base Build 17810051467:	-0.04%
Covered Lines:	8102
Relevant Lines:	9574

---

💛 - Coveralls

[spacebear21]

@benalleng FYI this change should allow us to test boundaries with exact timestamps and remove the SystemTime exclusions in mutation tests.

[spacebear21]

Concept ACK

High-level bikeshed: I just noticed we mix the usage of "expiry" and "expiration" throughout our code. IMO we should just stick to "expiration" since that's the terminology used in BIP77.

[nothingmuch]

High-level bikeshed: I just noticed we mix the usage of "expiry" and "expiration" throughout our code. IMO we should just stick to "expiration" since that's the terminology used in BIP77.

there's many instances throughout so i am going to do this in a followup PR across the entire codebase

[DanGould]

ACK 90928d4

[DanGould]

My main concern is that this exposes Time to the public API when @nothingmuch you seem to prefer Duration (from_secs + Time::from_now specifically). In the public interface. Is this indeed the case?

[nothingmuch]

My main concern is that this exposes Time to the public API when @nothingmuch you seem to prefer Duration (from_secs + Time::from_now specifically). In the public interface. Is this indeed the case?

i think both are fine and we can support both with an IntoTime trait as previously discussed so that Duration can be specified wherever Time is currently required

having Time as the "lowest common denominator" for lack of a better term is simpler IMO since there's no implicit dependency on the system clock

[nothingmuch]

oh, one downside i forgot to mention, if we stick with with_expiry(Duration) instead of with_expiry(Time) this can have conversion errors, so either we need a Result return type, or we need to .expect (which is what i've just implemented as i update this, in line with previous behavior)

[benalleng]

The success in diff mutations after removing the exclusions are a bit deceiving as I don't think they are necessarily going to test the code blocks unless their source code was explicitly changed in this PR, I am manually running the weekly cargo mutants on these to see what we can do about them now if any still fail.

[benalleng]

The mutants that are no longer excluded seem to no longer exist or have been correctly caught though we still do have the default expiry const mutant to address or ignore.

static TWENTY_FOUR_HOURS_DEFAULT_EXPIRY: Duration = Duration::from_secs(60 * 60 * 24);

$ cargo mutants -f "payjoin/src/core/send/**/*
.rs" -j4
Found 138 mutants to test
ok       Unmutated baseline in 50.5s build + 24.1s test
 INFO Auto-set test timeout to 2m 1s
138 mutants tested in 5m 55s: 82 caught, 56 unviable

$ cargo mutants -f "payjoin/src/core/receive/*
*/*.rs" -j4
Found 157 mutants to test
ok       Unmutated baseline in 53.6s build + 24.3s test
 INFO Auto-set test timeout to 2m 2s
MISSED   payjoin/src/core/receive/v2/mod.rs:64:81: replace * with / in 6.1s build + 54.7s test
MISSED   payjoin/src/core/receive/v2/mod.rs:64:76: replace * with + in 3.1s build + 37.1s test
MISSED   payjoin/src/core/receive/v2/mod.rs:64:81: replace * with + in 3.8s build + 62.1s test
157 mutants tested in 9m 52s: 3 missed, 81 caught, 73 unviable

After running this again I did not get the same consistent mutation failures, I need to remember to take a closer look at why this might be the case

[DanGould]

I find it odd that Duration is re-exported as part of payjoin::receive::v2 namespace.

V1PjParam, V2PjParam were re-introduced unnecessarily and should probably be removed.

[DanGould]

I figured this would be renamed to from_now but it's not a critical todo. Can be cleaned up when expiry/expiration is made to be consistent.

[nothingmuch]

hmm, why? isn't with_expiration consistent with builder pattern and bip77 terminology? i have that lined up after this one

[arminsabouri]

FWIW this was renamed to with_expiration in #1087

[arminsabouri]

Ack 5e045ba

[spacebear21]

ACK d8d7f99

[DanGould]

LGTM

LGTMed