Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Produce receive::JsonError accurately so that send can properly handle it #506

Merged
merged 4 commits into from
Jan 25, 2025

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Jan 23, 2025

This change could also introduce const values for well known error codes for both send and receive to share to prevent slipped typos during maintenance.

In reviewing the error types for v2, I realize we may want to introduce new well-known error types for the SessionError variants. In particular, Expired should have a template string to return to sender clients. One other variants seems to approximately fit original-psbt-rejected payload errors (Hpke), but UnexpectedResponseSize, OhttpEncapsulation, and UnexpectedStatusCode appear to be directory issues that are unrecoverable and cannot return json to a sender.

Seems like feerate configuration errors are also a special class of recoverable original-psbt-rejected error that might be worth classifying to potentially handle by clever senders.

related: #403

@coveralls
Copy link
Collaborator

coveralls commented Jan 23, 2025

Pull Request Test Coverage Report for Build 12966955577

Details

  • 22 of 91 (24.18%) changed or added relevant lines in 5 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 78.485%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/send/error.rs 5 9 55.56%
payjoin/src/receive/v2/error.rs 0 8 0.0%
payjoin/src/receive/v1/exclusive/error.rs 0 16 0.0%
payjoin/src/receive/error.rs 16 57 28.07%
Files with Coverage Reduction New Missed Lines %
payjoin/src/receive/error.rs 3 22.22%
Totals Coverage Status
Change from base Build 12966739100: -0.2%
Covered Lines: 3648
Relevant Lines: 4648

💛 - Coveralls

@DanGould DanGould marked this pull request as ready for review January 23, 2025 16:20
@DanGould DanGould force-pushed the json-error branch 3 times, most recently from 2a6a042 to bbc06bb Compare January 24, 2025 17:55
@DanGould
Copy link
Contributor Author

Elided the explicit <Error as JsonError> qualification, introduced const error_codes and re-classifeid one error to return NOT_ENOUGH_MONEY to communicate feerate mismatch

NOT_ENOUGH_MONEY does not return as much useful information to match feerates as I would like, but it's still on the right track and we can follow up when we classify the v2 errors. I think this is ready to go in.

@DanGould
Copy link
Contributor Author

this bare url lint failure wasn't caught when I ran ./contrib/lint.sh on local

Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9c770f1

@DanGould
Copy link
Contributor Author

Rebased to fix feerate/fee_rate conflict

@DanGould DanGould requested a review from spacebear21 January 25, 2025 17:20
Some errors need to be returned to the sender as JSON. This concern is separate
from the `Display` concern and isolated into a `JsonError:to_json` function
defined in a trait.
The other codes are unknown and won't be displayed to clients.
Well-known error codes are used in both send and receive modules. Using constants makes it harder
for a maintainer to accidentally produce or parse the wrong code.
Perhaps it can be extended to reveal preferences about appropriate feerates with a template, but for now this
at least uses the well known error code as intended.
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK a158cf2

@spacebear21 spacebear21 merged commit 27f7813 into payjoin:master Jan 25, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants