Chain service error

[roeierez]

fixed #983

This PR address two issues:

1. Ensures we only parse the json in case of success status code (2xx)
2. log any chain service errors with the corresponding url and associated error returned.
3. When attempting to lookup for closing tx onchain don't fail the sync process if the tx was not found (which technically could happen in case of zero conf).

[dangeross]

Is it worth logging the response body in unsuccessful requests or is the http code enough to diagnose potential issues?

[roeierez]

I think it worth since it shouldn't happen too much and when it happens like in the associated issue it helps identify the problem.

[ok300]

Unfortunately this would break error handling for LNURL.

Some specs (like LNURL but maybe others too) define errors in terms of how the response payload looks like, regardless of HTTP status.

If we now throw Err here in the GET response handling in case of HTTP error code, this means we will not return anymore the LNURL error JSON payload

{"status": "ERROR", "reason": "error details..."}

which means we lose the info on the error cause indicated by the server (not enough funds, etc).

IMO we need to return both the HTTP status and the raw payload, then let the caller decide. For mempool API calls, HTTP error code means error. For LNURL, the HTTP error code is only one mode of failure, the other is the error payload (can be returned with HTTP 200 or HTTP error).

[ok300]

I tested it and indeed:

with the PR, trying to LNURL-withdraw from a wallet with no funds returns

Error: Service connectivity: Service connectivity: GET request https://stacker.news/api/lnwith?k1=ce529c69344c7ae... failed with status: 400 Bad Request

whereas on main the same LNURL call returns

{
  "ErrorStatus": {
    "data": {
      "reason": "insufficient funds"
    }
  }
}

These LNURL error objects are useful to apps integrating with us, as they can surface the error reason to the end-user.

[ok300]

How about #990? IMO it covers both cases.

[dangeross]

I think it would be good to add test cases for the two ways a response is handled

[JssDWt]

HTTP status has (should have) no meaning at all in LNURL: https://github.com/lnurl/luds/blob/luds/01.md#http-status-codes-and-content-type

[roeierez]

@dangeross added test. Now both cases are tested.

[ok300]

Resolved conflicts with main.

[erdemyerebasmaz]

I think we can match once and return the result as tuple here:

let (response_body, status) = match &return_lnurl_error {
    None => (expected_lnurl_withdraw_data, 200),
    Some(err_reason) => (
        ["{\"status\": \"ERROR\", \"reason\": \"", &err_reason, "\"}"].join(""),
        400,
    ),
};

or we can use response_body to set status to avoid matching twice:

let status = if return_lnurl_error.is_none() {
    200
} else {
    400
};

[erdemyerebasmaz]

@roeierez cargo clippy has this warning:

error: this expression creates a reference which is immediately dereferenced by the compiler
    --> sdk-core/src/input_parser.rs:1081:60
     |
1081 |                 ["{\"status\": \"ERROR\", \"reason\": \"", &err_reason, "\"}"].join(""),
     |                                                            ^^^^^^^^^^^ help: change this to: `err_reason`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
     = note: `-D clippy::needless-borrow` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`

[erdemyerebasmaz]

Changes look good
Have a recommendation regarding redundant match on &return_lnurl_error

[erdemyerebasmaz]

According to documentation, I think it makes much more sense to rename this parse_get_and_log_response

[dangeross]

Looks good, except the clippy error