feat(client): return response body in deserialization errors

[cbeck88]

this adds a new pattern for handling errors when json bodies don't match the rust schema, so that the errors include more detail of what didn't deserialize. this helped me figure out what was wrong when the get_assets endpoint failed when monad was added

Added feedback

[extremeandy]

Another idea could be to just use tracing and log the body, if this is more for debug.

[cbeck88]

Another idea could be to just use tracing and log the body, if this is more for debug.

yeah, that's a good idea too, and it's less noise in the API.

happy to rewrite the PR that way

[extremeandy]

Another idea could be to just use tracing and log the body, if this is more for debug.

yeah, that's a good idea too, and it's less noise in the API.

happy to rewrite the PR that way

That'd be my preference but not sure what others think :)

[cbeck88]

done, lmk what you think

[cbeck88]

rebased on master and resolved conflicts

[cbeck88]

CI failure looks unrelated:

0s
Run cargo fmt --all --check
cargo fmt --all --check
shell: /usr/bin/bash -e {0}
env:
CARGO_HOME: /home/runner/.cargo
CARGO_INCREMENTAL: 0
CARGO_TERM_COLOR: always
CACHE_ON_FAILURE: false
Error: An error occurred trying to start process '/usr/bin/bash' with working directory '/home/runner/work/bpx-api-client/bpx-api-client/rust'. No such file or directory
Run cargo fmt --all --check
cargo fmt --all --check
shell: /usr/bin/bash -e {0}
env:
CARGO_HOME: /home/runner/.cargo
CARGO_INCREMENTAL: 0
CARGO_TERM_COLOR: always
CACHE_ON_FAILURE: false
Error: An error occurred trying to start process '/usr/bin/bash' with working directory '/home/runner/work/bpx-api-client/bpx-api-client/rust'. No such file or directory

[extremeandy]

Thanks for this, the helper is the right extraction and the debugging motivation is clear. A few things before merging:

Blockers

1. Panic on truncation. &body[..2000] in client/src/lib.rs slices a String at a byte index. If byte 2000 falls inside a multi-byte UTF-8 character this panics, so the error-reporting path can crash the caller. Use body.floor_char_boundary(2000) or collect body.chars().take(2000).

2. Branch is behind master. client/src/routes/vault.rs on master has four res.json().await.map_err(Into::into) calls (lines 28, 62, 72, 85) that this PR doesn't cover, so the pattern lands inconsistently. Merge is clean, so it would slip through silently. Please rebase and sweep.

Suggestions

3. Put the body in the error, not only the log. The PR description says "the errors include more detail of what didn't deserialize", but the returned value is a bare Error::SerdeJson with no body. The body only appears in a tracing::error! event, which library consumers without a subscriber never see. The repo already has a precedent for attaching the response body to the error value: process_response builds Error::BpxApiError { status_code, message } from the failed body. A sibling variant would keep the two failure paths symmetric and satisfy the stated goal:

   /// Response body could not be deserialized into the expected type.
   #[error("Failed to deserialize API response: {source}")]
   Deserialize {
       #[source]
       source: serde_json::Error,
       body: Box<str>,
   },

   The tracing call can stay alongside it if you want the log too, though debug! or warn! is more typical for a library on a path that already returns Err.

4. Visibility. json_with_context is pub while its sibling process_response is private. Nothing outside the crate needs it, and once public it's a semver commitment. pub(crate) seems right.

5. Changelog note for the variant shift. Decode failures previously surfaced as Error::Reqwest (with is_decode() true) and now surface as Error::SerdeJson. Not a compile-time break, and arguably an improvement since the serde error carries line and column, but any downstream match on the old variant stops matching for this case. Worth a line in CHANGELOG.md. If you go with (3), the note applies to the new variant instead.

6. Drop the Blockchain::Stable change. It isn't mentioned in the PR and is already on master, so it's a no-op after merge but doesn't belong in this commit.

7. Naming, minor. json_with_context never returns context to the caller, only logs it. If you adopt (3) the name becomes accurate; otherwise something like deserialize_json_logging_body says what it does.

fmt and lib clippy pass on the branch. The --all-targets clippy failure is in client/tests/integration-tests.rs, which this PR doesn't touch and which already fails at the base.

[extremeandy]

@cbeck88 I pushed two commits to this branch to address the review above so it can move along:

• a merge of master (no history rewrite), and
• fix(client): carry response body in deserialization errors, which adds an Error::Deserialize { source, body } variant, fixes the UTF-8 slice panic in the preview (with unit tests), makes the helper pub(crate) as deserialize_json, sweeps the call sites added on master since you branched (vault.rs, order.rs, markets.rs), removes the duplicate Blockchain::Stable the merge produced, and drops the log to debug! since the error now carries the body.

Feel free to drop both commits and do it your own way if you'd prefer. Thanks for the PR.

[extremeandy]

@cbeck88 CI is green but the merge is blocked because master requires signed commits, and 57af0ed (your original commit) is unsigned. The two follow-up commits are signed.

Two ways forward, your pick:

1. You sign it. Amend 57af0ed with a GPG or SSH signature and force-push the branch. The two follow-up commits will need to be rebased onto the amended one, so the simplest is git rebase --exec 'git commit --amend --no-edit -S' origin/master on your branch (or equivalent), then git push --force-with-lease.
2. We rewrite it. Say the word and I'll rebase the branch onto master, sign all three commits with my key, and force-push to your fork branch. Your authorship on the original commit is preserved; only the committer and signature change.

Happy either way, just let me know.

[cbeck88]

I'll try to sign it, thank you