Introduce directory feature module

[DanGould]

• Allow payjoin and payjoin-directory to share ShortId code.
• define directory::ENCAPSULATED_MESSAGE_BYTES: usize = 8192;

other constants (like tag size) seem more appropriate for bitcoin_hpke or bitcoin_ohttp to define than for the directory module.

Close #418

In order to allow directory to be used independetly, I needed to make the _core feature compile send and receive modules, and made v1 and v2 features export their feature modules explicitly. That way when directory is enabled explicitly with no-default-features no other code gets compiled.

Some exclusive v1 features are used for testing v2, so perhaps these should be exported in _test-util as well i.e. #[cfg(any(feature = "v1", feature = "_test-util")] in a follow up.

@nothingmuch This also revealed some indirection in the bech32 module, where some of it is used for directory and some for v2 OhttpKeys serialization. How essential is the bech32::nochecksum module? Perhaps the bech32 calls should be made directly with the NoChecksum generic parameter and the tests should be placed around OhttpKeys and directory::ShortId implementations directly.

I'm not completely sure why the coverage was reduced so much by placing some features in the exclusive submodule, but this will be covered when the json error serialization and error stabililzation issues are addressed.

[coveralls]

Pull Request Test Coverage Report for Build 12920617811

Details

• 133 of 147 (90.48%) changed or added relevant lines in 11 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+6.0%) to 78.105%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/directory.rs	24	25	96.0%
payjoin/src/receive/v1/mod.rs	8	9	88.89%
payjoin/src/receive/v2/error.rs	0	1	0.0%
payjoin/src/receive/v1/exclusive/error.rs	0	2	0.0%
payjoin-directory/src/lib.rs	9	12	75.0%
payjoin/src/receive/v1/exclusive/mod.rs	59	62	95.16%
payjoin/src/receive/v2/mod.rs	3	6	50.0%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/receive/error.rs	2	21.58%

Totals
Change from base Build 12874640319:	6.0%
Covered Lines:	3578
Relevant Lines:	4581

---

💛 - Coveralls

[spacebear21]

I'm not sure I really understand the motivation for splitting v1 modules into exclusive v. shared (adding complexity). Is there a significant impact from exporting those features when v1 is not explicitly enabled?

[spacebear21]

This would remove the ability to run integration tests for v2 only (or for v1 only).

Suggested change

	#[cfg(all(feature = "v1", feature = "v2"))]
	#[cfg(any(feature = "v1", feature = "v2"))]

Actually, this is probably the reason for the drastic coverage reduction.

[DanGould]

Hmm. I changed this because we're depending on payjoin::receive::v1::build_v1_pj_uri for both v1 and v2 tests. Why do you think this would reduce coverage? Because some runs would not check branches that they previously had, even though all of the same lines are still covered? That doesn't make sense to me.

[DanGould]

I see now, our coverage script:

cargo llvm-cov --no-report --no-default-features --features=v1,_danger-local-https
cargo llvm-cov --no-report --no-default-features --features=v2,_danger-local-https,io

Doesn't test the two in tandem since the integration/v2 test module now depends on the v1 feature since there are many v2 tests that test backwards compatibility.

We can combine these to use cargo llvm-cov --no-report --all-features now since everything is additive. That ensures the directory feature is tested too.

[spacebear21]

contrib/coverage.sh runs the tests like this:

cargo llvm-cov --no-report --no-default-features --features=v1,_danger-local-https
cargo llvm-cov --no-report --no-default-features --features=v2,_danger-local-https,io

Default features are disabled, so the first command wouldn't pass the cfg check.

Though now that features are additive maybe these two commands can be combined with --all-features.

(The same applies to lint and test scripts)

[spacebear21]

jinx

[spacebear21]

Note: with this change, UncheckedProposal::from_request is no longer covered by unit tests. It's still covered in v1 integration tests. We may want to consider adding a unit test to exclusive/mod.rs.

[DanGould]

Added a unit test

[DanGould]

I'm not sure I really understand the motivation for splitting v1 modules into exclusive v. shared (adding complexity). Is there a significant impact from exporting those features when v1 is not explicitly enabled?

My motivation was to isolate v1-specific public API so that it could be exposed as a default-off feature. I figured it made it even more obvious the v2 feature was first-class, and was also useful in organizing the _core feature so that directory could be a dependency without any of the _core payjoin typestate machinery

Is this overly complex and contrived? It did feel a bit leaky to use the v1 stuff to test exclusively v2 and not depend on v1 to test the v2_to_v1 / v1_to_v2 stuff

[DanGould]

Coverage fixed. Looks like once errors are stabilized and tested we might hit 80% coverage from that alone

[spacebear21]

ACK 6580519

It looks like overall coverage went up because it stopped tracking payjoin-cli v1 files altogether. This can be addressed later because cli v1 coverage is broken already anyway and currently reports 0% (#499).