Rewrite send::error::ResponseError docs
#307
Conversation
payjoin/src/send/error.rs
Outdated
| /// `WellKnown` errors following the BIP78 spec | ||
| /// https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#user-content-Receivers_well_known_errors | ||
| /// These errors are displayed to end users. | ||
| /// Errors that are defined in the [`BIP78`] spec. | ||
| /// | ||
| /// The `WellKnownError` represents `errorCode` and `message`. | ||
| /// It is safe to display these errors to end users. | ||
| /// | ||
| /// [`BIP78`]: https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#user-content-Receivers_well_known_errors |
There was a problem hiding this comment.
I prefer that the original explicitly said "WellKnown errors". There are lots of errors defined in the bip78 spec, these are explicitly called "Well-known" errors
I do also prefer your wording "It is safe to display these errors to end users."
There was a problem hiding this comment.
fixed.
I was looking into the spec and could not find another errors, could you be please share a link to errors beside the ReceiverWellKnown ?
payjoin/src/send/error.rs
Outdated
| /// `Unrecognized` errors are errors that are not well known and are only displayed in debug logs. | ||
| /// They are not displayed to end users. | ||
| /// Errors out of the scope of the [`BIP78`] spec. | ||
| /// | ||
| /// These errors are only displayed in debug logs. | ||
| /// | ||
| /// [`BIP78`]: https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#user-content-Receivers_well_known_errors |
There was a problem hiding this comment.
I prefer the original first sentence. It would be good for this to mirror the first change you made
"They are not displayed to end users." -> "Unrecognized errors are not safe to display to end users because they could be used maliciously to phish a non technical user. Only display them in debug logs."
Adding the link is a great idea.
| /// They are only displayed in debug logs. | ||
| /// Errors caused by malformed responses. | ||
| /// | ||
| /// These errors are only displayed in debug logs. |
There was a problem hiding this comment.
The comment change is fine but "These errors are only displayed in debug logs" is false statement impl Display for ResponseError writes the ValidationError in the match arm for ResponseError::Validation
Consider fixing the Display implementation to only write a fixed string.
rust-payjoin/payjoin/src/send/error.rs
Line 354 in 1f87e82
jbesraa
force-pushed
the
2024-06-26-response-error-docs
branch
2 times, most recently
from
9bf956c
to
4a29240
Compare
jbesraa
force-pushed
the
2024-06-26-response-error-docs
branch
from
4a29240
to
68c2e4f
Compare
|
@DanGould revisited issue body plus addressed your comments and added new commit to resolve the |
| /// `WellKnown` errors following the BIP78 spec | ||
| /// https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#user-content-Receivers_well_known_errors | ||
| /// These errors are displayed to end users. | ||
| /// `WellKnown` Errors are defined in the [`BIP78::ReceiverWellKnownError`] spec. |
There was a problem hiding this comment.
I find the use of :: to identify the receiver spec a bit odd since it's not referencing a rust module element but it does get the point across
noticed the docs are kind of bad while reviewing #304
resolves #306