Add PartialEq/Eq to errors for easier comparison by w3irdrobot · Pull Request #768 · payjoin/rust-payjoin

w3irdrobot · 2025-06-13T19:12:53Z

This one escalated quickly. There were many enums where I needed to implement PartialEq/Eq manually. I used false in those implementations when the wrapped error did not implement these traits. The biggest change here is the change of payjoin::ImplementationError from a type alias to a newtype. This update required adding some implementations to ImplementationError from other errors we use. To prevent adding a bunch of dependencies to payjoin, I also opted for manually wrapping errors using .map_err(ImplementationError::new) in many places instead of implementing From<SomeNewDependecyError> for ImplementationError. It's not necessarily the cleanest; it exposes some places for improvements in error handling. However, I assume #403 will tighten some of this up.

Closes #645

coveralls · 2025-06-13T21:37:49Z

Pull Request Test Coverage Report for Build 16426338362

Details

73 of 99 (73.74%) changed or added relevant lines in 9 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.09%) to 85.706%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/db/error.rs	0	1	0.0%
payjoin/src/core/receive/error.rs	0	1	0.0%
payjoin/src/core/error.rs	9	11	81.82%
payjoin/src/core/send/v2/session.rs	1	4	25.0%
payjoin/src/core/receive/v2/session.rs	1	5	20.0%
payjoin-cli/src/app/v1.rs	29	44	65.91%

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/app/v1.rs	1	76.84%
payjoin/src/core/send/v2/session.rs	1	86.84%

Totals
Change from base Build 16416253790:	-0.09%
Covered Lines:	7741
Relevant Lines:	9032

💛 - Coveralls

DanGould

Thank you for giving this issue significant attention @w3irdrobot. This work is definitely on the path to more stable, usable public error API. However I do request some changes.

I used false in those implementations when the wrapped error did not implement these traits

It seems to me that the way to test error matching in cases where inner errors are not themselves PartialEq is to use match on the variants. In those cases implementing PartialEq for these types is simply not possible because getting a completely correct equality without having [Partial]Eq implemented on the underlying types is impossible. I think the best we can do is to keep PartialEq implementations only where all of the underlying errors are also PartialEq.

The biggest change here is the change of payjoin::ImplementationError from a type alias to a newtype

This seems to be an independent change from PartialEq/Eq implementation that merits its own commit, and perhaps its own PR, since it is distinct from the Eq implementation as far as I can tell. I welcome this change. It fixes a number of problems, including our inability to impl functions on the alias and the difficulty to generate FFI bindings.

To prevent adding a bunch of dependencies to payjoin, I also opted for manually wrapping errors using .map_err(ImplementationError::new) in many places instead of implementing From for ImplementationError.

I agree that this is the appropriate course of action. In fact, I would rather not even introduce anyhow to the core payjoin crate, nor implement the other From conversions for ImplementationError. I think we're better off doing manual maps where the interface requires an ImplementationError to be returned to explicitly mark those conversions.

It may well be that I wrote #645 before I understood that using match and matches! (or assert_matches! in newer versions of rust) to test error variants without PartialEq implementations would be the most appropriate, and that some variants are simply ineligible for PartialEq implementation. Your efforts here have furthered our collective knowledge about these mechanics and are not in vain.

DanGould · 2025-06-14T22:44:51Z

payjoin/src/core/error.rs

+#[derive(Debug)]
+pub struct ImplementationError(Box<dyn error::Error + Send + Sync>);
+
+impl ImplementationError {
+    pub fn new(e: impl error::Error + Send + Sync + 'static) -> Self {
+        ImplementationError(Box::new(e))
+    }
+}


While I do believe such an ImplementationError is appropriate, I'm doubt that all of the From implementations, which are all part of the public API are when the simple Implementation::new type can be used.

DanGould · 2025-06-14T22:46:15Z

payjoin/src/ohttp.rs

+impl PartialEq for DirectoryResponseError {
+    fn eq(&self, other: &Self) -> bool {
+        match (self, other) {
+            (DirectoryResponseError::InvalidSize(s1), DirectoryResponseError::InvalidSize(s2)) =>
+                s1 == s2,
+            (
+                DirectoryResponseError::OhttpDecapsulation(_),
+                DirectoryResponseError::OhttpDecapsulation(_),
+            ) => true,
+            (
+                DirectoryResponseError::UnexpectedStatusCode(s1),
+                DirectoryResponseError::UnexpectedStatusCode(s2),
+            ) => s1 == s2,
+            _ => false,
+        }
+    }
+}
+
+impl Eq for DirectoryResponseError {}


It seems to me that the way to test error matching in cases where inner errors are not themselves PartialEq is to use match, and implementing PartialEq for these types is simply not possible because getting a completely correct implementation is not available without being able to compare the underlying types.

OK. So I should remove the manual implementations of PartialEq/Eq from the error types that can't just derive it?

w3irdrobot · 2025-06-15T15:06:33Z

@DanGould I've updated this PR to remove the manual equality trait implementations. I also removed most of the From implementations from payjoin::ImplemnentationError

DanGould · 2025-07-11T17:18:12Z

@w3irdrobot Thanks for updating this. May you please rebase your changes on master so that we can avoid a merge commit?

w3irdrobot · 2025-07-11T18:30:34Z

@DanGould done. 💪

DanGould

I think this is ok, but most of the actual +/- changes in this PR are formatting changes which makes me uncertain whether or not I've checked all of the semantic changes. May those be removed so that this PR is limited to the actual PartialEq/Eq-related changes?

DanGould · 2025-07-15T16:09:34Z

payjoin-cli/src/app/v2/mod.rs

+                ReceiveSession::Initialized(proposal) => {
+                    self.read_from_directory(proposal, persister).await
+                }


It seems there are quite a few of these formatting changes that are unrelated to the semantic changes this PR wants to address. Would it be possible to remove them from the commit @w3irdrobot

I can remove them but they were just made by running cargo fmt. I didn't manually make them.

This happens when using the stable rust toolchain, you can either change your default to nightly with rustup default nightly or specify per command with cargo +nightly fmt (see https://github.com/payjoin/rust-payjoin/blob/master/.github/CONTRIBUTING.md#commits)

i'm pretty sure i ran it with nightly but there's a chance i didn't.

looks like that was the issue. ran it with nightly and it put things back. should be good now.

DanGould · 2025-07-15T16:09:55Z

payjoin-ffi/python/scripts/generate_macos.sh

-        target/aarch64-apple-darwin/release-smaller/$LIBNAME \
-        target/x86_64-apple-darwin/release-smaller/$LIBNAME
+  target/aarch64-apple-darwin/release-smaller/$LIBNAME \
+  target/x86_64-apple-darwin/release-smaller/$LIBNAME


I don't think any of this has to do with PartialEq/Eq

this was to fix, if I remember correctly, being able to run the nix checks against the repo. the formatter was blowing up. I can remove them.

DanGould · 2025-07-16T17:01:14Z

I should have said that you should please feel free to include in a separate commit. If the cargo fmt passes in CI but not on local there must be a version discrepancy.

I prefer that commit comes before the semantic changes that way each and every commit passes all of CI.

w3irdrobot · 2025-07-16T17:02:13Z

@DanGould ok. i'll get them fixed up later tonight.

DanGould · 2025-07-16T17:09:48Z

Thanks for the perseverance. Almost there!

w3irdrobot · 2025-07-21T13:52:16Z

@DanGould okay. got the tests and lint passing finally.

spacebear21

ACK 6078a9e, though there is a .Cargo.lock.flock file that snuck in, and I believe the Cargo-*.lock changes are unnecessary for this PR. Otherwise looks good to me.

w3irdrobot · 2025-07-21T19:23:47Z

@spacebear21 i can clean that up if desired. i think some of that happened when i was trying to get lints to pass about a month ago.

spacebear21 · 2025-07-21T19:30:13Z

@w3irdrobot please do, I'll merge this PR once that is cleaned up.

This allows testing against specific error variants without using string matching.

w3irdrobot · 2025-07-21T19:34:57Z

@spacebear21 i removed that random flock file. the Cargo-*.lock changes are actually needed because the most recent version actually added the self.into_boxed_dyn_error() that is used in this PR to allow anyhow to easily be used with ImplementationError::new

spacebear21 · 2025-07-21T19:39:58Z

Oh I see, that makes sense. Thanks for removing the other flock file. Will re-ACK/merge pending passing CI.

Addressed comments re: unrelated changes

spacebear21

re-ACK f18c736, thanks for keeping this alive @w3irdrobot!

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from 60d5658 to 605fdfd Compare June 13, 2025 19:22

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from 605fdfd to 6ba5c92 Compare June 14, 2025 19:13

DanGould requested changes Jun 14, 2025

View reviewed changes

w3irdrobot requested a review from DanGould June 15, 2025 15:05

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from 05b0c07 to 6363121 Compare June 18, 2025 14:17

DanGould force-pushed the master branch from 61351cb to 9f403f1 Compare June 20, 2025 20:17

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from 21103cf to 8992982 Compare July 11, 2025 17:32

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from 8992982 to 5fb2184 Compare July 15, 2025 01:34

DanGould previously requested changes Jul 16, 2025

View reviewed changes

w3irdrobot force-pushed the implement-err-partial-eq-eq branch 3 times, most recently from 12bfecc to deeb848 Compare July 19, 2025 20:51

w3irdrobot requested review from DanGould and spacebear21 July 19, 2025 21:14

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from deeb848 to 6078a9e Compare July 20, 2025 20:34

spacebear21 approved these changes Jul 21, 2025

View reviewed changes

Add PartialEq/Eq to errors for easier comparison

f18c736

This allows testing against specific error variants without using string matching.

w3irdrobot force-pushed the implement-err-partial-eq-eq branch from 6078a9e to f18c736 Compare July 21, 2025 19:32

spacebear21 approved these changes Jul 21, 2025

View reviewed changes

spacebear21 merged commit 0c1b931 into payjoin:master Jul 21, 2025
15 checks passed

w3irdrobot deleted the implement-err-partial-eq-eq branch July 21, 2025 20:10

spacebear21 mentioned this pull request Jul 21, 2025

Evaluate making ImplementationError an actual type #751

Closed

Conversation

w3irdrobot commented Jun 13, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 16426338362

Details

💛 - Coveralls

Uh oh!

DanGould 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!

w3irdrobot commented Jun 15, 2025

Uh oh!

DanGould commented Jul 11, 2025

Uh oh!

w3irdrobot commented Jul 11, 2025

Uh oh!

DanGould 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!

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!

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

Uh oh!

Uh oh!

w3irdrobot commented Jul 16, 2025

Uh oh!

DanGould commented Jul 16, 2025

Uh oh!

w3irdrobot commented Jul 21, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

w3irdrobot commented Jul 21, 2025

Uh oh!

spacebear21 commented Jul 21, 2025

Uh oh!

w3irdrobot commented Jul 21, 2025

Uh oh!

spacebear21 commented Jul 21, 2025

Uh oh!

spacebear21 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 Jun 13, 2025 •

edited

Loading

DanGould commented Jul 16, 2025 •

edited

Loading

spacebear21 left a comment •

edited

Loading