feat: Send x-build-version header for every request

[bonomat]

We are sending x-build-version on every request and fail if the sdk version is too old

resolves #195

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• next-version

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6584f99e-6e78-4f42-8c12-411d72ef1ebb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/send-version

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[arkanaai]

Review: feat: Send x-build-version header for every request

Reviewed full diff + full file context + server-side interceptor in arkd. Not protocol-critical (no VTXO/signing/forfeit/exit paths touched). Findings below.

---

🔴 Dead code: from_tonic_status never called, Kind::VersionMismatch never constructed

ark-grpc/src/error.rs — from_tonic_status() is defined but zero call sites exist. Every gRPC call in ark-grpc/src/client.rs still uses .map_err(Error::request) (18 call sites). This means Kind::VersionMismatch is never constructed — it's dead code.

The is_version_mismatch() method does work regardless (it downcasts the source to tonic::Status and checks there), so the detection itself is fine. But the Kind::VersionMismatch variant and from_tonic_status serve no purpose as written. Either:

• Wire from_tonic_status into the call sites (replacing Error::request for tonic status errors), or
• Remove Kind::VersionMismatch and from_tonic_status entirely — is_version_mismatch() already works without them.

Same issue in ark-rest/src/error.rs: Kind::VersionMismatch added but never constructed.

🟡 ark-rest/src/client.rs:63 — expect() panics in a library

.expect("failed to build HTTP client");

Panicking in library code is poor practice. Yes, building a reqwest client with just default headers is unlikely to fail, but this is a public constructor. Consider returning Result<Self, Error> or using unwrap_or_default() for the client builder and documenting the invariant.

🟡 REST is_version_mismatch is fragile compared to gRPC version

ark-rest/src/error.rs — The REST version does:

source.to_string().contains("BUILD_VERSION_TOO_OLD")

This relies on the server's error message string surviving through reqwest's error chain as a .to_string(). Unlike the gRPC version (which properly downcasts to tonic::Status and checks code + message), this is fragile. If reqwest wraps the response body differently, or the server changes its error format, this silently breaks. Consider checking the HTTP status code (412 or 400?) alongside the body content.

🟡 No tests

No tests for:

• Version header actually being sent (could be an integration test with a mock server)
• is_version_mismatch() returning true when given a BUILD_VERSION_TOO_OLD status
• is_version_mismatch() returning false for other errors

The server-side interceptor is well-tested in arkd, but the client-side detection logic is untested.

✅ What looks good

• gRPC interceptor pattern is clean — VersionInterceptor with tonic::service::Interceptor is the right approach.
• Single channel, two clients — the refactor from two separate connect() calls to one shared channel is an improvement (fewer TCP connections).
• env!("CARGO_PKG_VERSION") — compile-time, zero-cost. Correct.
• from_static — avoids per-request allocation. Good.
• Manual Debug impl — necessary since InterceptedService doesn't derive Debug. Implementation is fine.
• Server behavior is safe — per arkd docs, missing header = request allowed, malformed version = request allowed. No risk of breaking existing clients.
• Cross-repo: ts-sdk, go-sdk, dotnet-sdk don't send this header yet — no breakage since server allows missing headers.

Summary

The core feature (sending the version header) is correct and well-implemented. The dead code (from_tonic_status, Kind::VersionMismatch) and the expect() panic should be cleaned up before merge. The lack of tests for is_version_mismatch() is a moderate concern — it's a public API method that callers will rely on.

[arkanaai]

Review: feat: Send x-build-version header for every request

Reviewed full diff + full file context + server-side interceptor in arkd. Not protocol-critical (no VTXO/signing/forfeit/exit paths touched). Findings below.

---

🔴 Dead code: from_tonic_status never called, Kind::VersionMismatch never constructed

ark-grpc/src/error.rs — from_tonic_status() is defined but zero call sites exist. Every gRPC call in ark-grpc/src/client.rs still uses .map_err(Error::request) (18 call sites). This means Kind::VersionMismatch is never constructed — it's dead code.

The is_version_mismatch() method does work regardless (it downcasts the source to tonic::Status and checks there), so the detection itself is fine. But the Kind::VersionMismatch variant and from_tonic_status serve no purpose as written. Either:

• Wire from_tonic_status into the call sites (replacing Error::request for tonic status errors), or
• Remove Kind::VersionMismatch and from_tonic_status entirely — is_version_mismatch() already works without them.

Same issue in ark-rest/src/error.rs: Kind::VersionMismatch added but never constructed.

🟡 ark-rest/src/client.rs:63 — expect() panics in a library

.expect("failed to build HTTP client");

Panicking in library code is poor practice. Yes, building a reqwest client with just default headers is unlikely to fail, but this is a public constructor. Consider returning Result<Self, Error> or using unwrap_or_default() for the client builder and documenting the invariant.

🟡 REST is_version_mismatch is fragile compared to gRPC version

ark-rest/src/error.rs — The REST version does:

source.to_string().contains("BUILD_VERSION_TOO_OLD")

This relies on the server's error message string surviving through reqwest's error chain as a .to_string(). Unlike the gRPC version (which properly downcasts to tonic::Status and checks code + message), this is fragile. If reqwest wraps the response body differently, or the server changes its error format, this silently breaks. Consider checking the HTTP status code (412 or 400?) alongside the body content.

🟡 No tests

No tests for:

• Version header actually being sent (could be an integration test with a mock server)
• is_version_mismatch() returning true when given a BUILD_VERSION_TOO_OLD status
• is_version_mismatch() returning false for other errors

The server-side interceptor is well-tested in arkd, but the client-side detection logic is untested.

✅ What looks good

• gRPC interceptor pattern is clean — VersionInterceptor with tonic::service::Interceptor is the right approach.
• Single channel, two clients — the refactor from two separate connect() calls to one shared channel is an improvement (fewer TCP connections).
• env!("CARGO_PKG_VERSION") — compile-time, zero-cost. Correct.
• from_static — avoids per-request allocation. Good.
• Manual Debug impl — necessary since InterceptedService doesn't derive Debug. Implementation is fine.
• Server behavior is safe — per arkd docs, missing header = request allowed, malformed version = request allowed. No risk of breaking existing clients.
• Cross-repo: ts-sdk, go-sdk, dotnet-sdk don't send this header yet — no breakage since server allows missing headers.

Summary

The core feature (sending the version header) is correct and well-implemented. The dead code (from_tonic_status, Kind::VersionMismatch) and the expect() panic should be cleaned up before merge. The lack of tests for is_version_mismatch() is a moderate concern — it's a public API method that callers will rely on.

[arkanaai]

Arkana follow-up — No new commits since my last review (bccdd016). My previous change requests still apply. Waiting on author to address before re-reviewing.