Update select_first_candidate to return InternalSelectionError::Empty

[pseudoramdom]

Fix #535

Update select_first_candidate to return InternalSelectionError::Empty when there are no candidates available for selection

[pseudoramdom]

Hi there! I'm new here. I didn't see this covered in any existing tests, unless you'd like me to add one?

[DanGould]

If you are up to adding a test that'd be a welcome addition

[coveralls]

Pull Request Test Coverage Report for Build 14044611583

Details

• 32 of 33 (96.97%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.08%) to 81.424%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v1/mod.rs	32	33	96.97%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/receive/v1/mod.rs	1	95.27%

Totals
Change from base Build 14043365381:	0.08%
Covered Lines:	5089
Relevant Lines:	6250

---

💛 - Coveralls

[spacebear21]

Also, currently we actually check for an empty candidates set twice: the first one is here.

So this specific code change would be unreachable unless calling select_first_candidate directly, but it's a private function so I'm not sure that would a be a useful test. Perhaps we should remove the empty check in try_preserving_privacy and have it in avoid_uih and select_first_candidate only?

[DanGould]

Yes please remove the empty check in try_... and leave it to avoid_uih and select_first_candidate only

[pseudoramdom]

I made the requested changes.
I was able to add a test but InternalSelectionError has an access level of pub(crate).
I had to resort to a string check . Let me know if that works.

[arminsabouri]

@pseudoramdom Do you mind squashing your commits into one?

[benalleng]

Just a small nit to keep the test signatures consistent

[benalleng]

The test looks good

[spacebear21]

ACK 5066eef

Thanks @pseudoramdom and reviewers!

nit: looks like all previous commit messages got squashed into this one, it would be nice to clean up the commit message before merging this. We generally follow the commit guidelines established in https://cbea.ms/git-commit/

[DanGould]

LGTM other than the commit messages. Please follow the seven rules from cbeams.

Thank you for your contribution 😎

[pseudoramdom]

Thanks for the reviews folks! Fixed the commit message. Let me know if it looks good.

[DanGould]

Thanks for the update. I think there is one sincere issue with the test, as discovered by @elnosh in another PR that I've repeated in my comments below.

I would like to understand the rationale of the change in the PR body, in complete sentences, rather than just a repeat of the title. The commit message is closer but I still need it to follow the seven rules, namely 2, 6, and 7, seem broken to me. Much more important than the what is the why, or rationale for the change.

[DanGould]

ACK b37bdce

[spacebear21]

Fixed the commit message and removed the redundant assert in the unit test.