Supply & parse receiver response errors#110
Conversation
ed3f782 to
51e7fd8
Compare
| bip21 = "0.3.1" | ||
| log = { version = "0.4.14"} | ||
| rand = { version = "0.8.4", optional = true } | ||
| serde = { version = "1.0.107", features = ["derive"] } |
There was a problem hiding this comment.
do we have better options than serde?
I have some reservations in adding it to such a project for the following reasons:
- a couple of months ago this thing happened https://www.bleepingcomputer.com/news/security/rust-devs-push-back-as-serde-project-ships-precompiled-binaries/
it was eventually reverted but it is a red flag - serde does a great job but it also makes compilation/building the project much slower
There was a problem hiding this comment.
We only need to parse and produce 2 lines of valid json without anything fancy like nesting. What do you suggest as an alternative, and why?
The json crate would require us to lift more for deserialization but is lightweight in comparison to serde https://github.com/maciejhirsz/json-rust
We should pay attention to how rust-bitcoin desides to handle this even though their use case is optional vs. protocol-critical here rust-bitcoin/rust-bitcoin#2009
There was a problem hiding this comment.
we could consider https://github.com/rhysd/tinyjson
- its well documented, tested and the code is quite readable
- its really tiny(we can also clone it into payjoin repo and strip out any code we dont need, we just parse string really)
disadvantages: last update was a few months ago, but that's not necessarily a red flag
I think optimal scenario is to clone tinyjson, clean it up so it has only functionality we need and maintain it ourselves.
maybe we can also leave serde and put it behind a flag for now.
it really depends on how fast we want this pr to be merged
*I am intentionally not mentioning performance here because we are not dealing with large/nested json structures so that should be a problem
There was a problem hiding this comment.
regarding json-rust, its not maintained and the last update was 3 years ago, an alternative crates were developed(like rust-json)
There was a problem hiding this comment.
I think optimal scenario is to clone tinyjson, clean it up so it has only functionality we need and maintain it ourselves.
This is definitely overkill to parse 4 lines of JSON that should only ever encode / decode strings with errorCode and message fields. We could honestly write an even simpler parser and forego a JSON dependency without a problem.
There was a problem hiding this comment.
We might miss some edge cases, but we could simplify ser/de do the following
fn serialize_error(error_code: &str, message: &str) -> String {
format!(r#"{{"errorCode": "{}", "message": "{}"}}"#, error_code, message)
}above is how we've already been serializing errors
fn deserialize_error(json: &str) -> Option<(&str, &str)> {
let error_code_start = json.find("\"errorCode\": \"")? + "\"errorCode\": \"".len();
let error_code_end = json.find("\", \"message\": \"")?;
let message_start = error_code_end + "\", \"message\": \"".len();
let message_end = json.rfind("\"}")?;
let error_code = &json[error_code_start..error_code_end];
let message = &json[message_start..message_end];
Some((error_code, message))
}
51e7fd8 to
f535666
Compare
|
I started to add |
| log::error!("Response contains error: {}", e); | ||
| log::debug!("Response contains error: {:?}", e); |
There was a problem hiding this comment.
Maybe we could have the definition of debug execute all the logs of error as well and then we wont have to add double log lines.
for example, if we have these levels of logs:
- Debug <- show everything Trace is showing + some more logs
- Trace <- show everything Error is showing + some more logs
- Error <- show only errors
| impl WellKnownError { | ||
| pub fn from_str(s: &str) -> Option<Self> { | ||
| match s { | ||
| "unavailable" => Some(WellKnownError::Unavailable), |
There was a problem hiding this comment.
validated the errors and their description against https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-well-known-errors
looks good!
| use tinyjson::{JsonParser, JsonValue}; | ||
|
|
||
| let parsed: JsonValue = json.parse().unwrap(); | ||
| //.unwrap_or_else( |_| ResponseError::Validation(InternalValidationError::Parse.into())); |
| } | ||
|
|
||
| pub enum ResponseError { | ||
| // Well known errors with internal message for logs as String |
There was a problem hiding this comment.
should we add a bit more info here and maybe link to bip78 for reference?
also, should we use /// in the comments so it shows also on docs.rs ?
| // Don't display unknowns to end users, only debug logs | ||
| Unrecognized(String, String), | ||
|
|
||
| Validation(ValidationError), |
There was a problem hiding this comment.
missing some docs here, I feel that Validation is quite broad
|
ah just saw your comment regarding |
|
closed for #120 |
2 participants
Uh oh!
There was an error while loading. Please reload this page.