Rewrite send::error::ResponseError docs - #307
Conversation
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
Helpful changes. Thanks!
Can you be more specific with your issue however? What do you mean when you say documentation is "kind of bad"
| /// `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 ?
| /// `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
9bf956c to
4a29240
Compare
4a29240 to
68c2e4f
Compare
|
@DanGould revisited issue body plus addressed your comments and added new commit to resolve the |
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
Thank you. So much cleaner than before. Excellent.
| /// `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
2 participants
noticed the docs are kind of bad while reviewing #304
resolves #306