Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions crates/client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,114 @@ impl From<tonic::Status> for ClientError {
ClientError::Status(Box::new(status))
}
}

impl ClientError {
/// Parse the structured `{ code, message, meta }` object Guardian attaches
/// to a gRPC `Status.details` (feature `009-human-readable-errors`).
/// Returns `None` for non-Status errors or Status errors without details.
fn guardian_error_details(&self) -> Option<serde_json::Value> {
match self {
ClientError::Status(status) => {
let details = status.details();
if details.is_empty() {
return None;
}
serde_json::from_slice(details).ok()
}
_ => None,
}
}

/// Stable, machine-readable Guardian error code (e.g. `account_paused`),
/// when this error originated from a Guardian gRPC `Status`. Branch on
/// this rather than on the message text.
pub fn guardian_code(&self) -> Option<String> {
self.guardian_error_details()?
.get("code")?
.as_str()
.map(str::to_owned)
}

/// Short, end-user-safe message safe to show in a wallet UI, when this
/// error originated from a Guardian gRPC `Status`. Falls back to the
/// `Status.message` (also the user-safe message) for Status errors
/// without a details object.
pub fn user_message(&self) -> Option<String> {
if let Some(message) = self
.guardian_error_details()
.and_then(|d| d.get("message")?.as_str().map(str::to_owned))
{
return Some(message);
}
match self {
ClientError::Status(status) => Some(status.message().to_owned()),
_ => None,
}
}

/// The structured `meta` block (`retryable`, `retry_after_secs`, …) from a
/// Guardian gRPC `Status`, when present.
pub fn guardian_meta(&self) -> Option<serde_json::Value> {
self.guardian_error_details()?.get("meta").cloned()
}

/// Whether this error means "the requested record does not exist". Handles
/// both the gRPC `Status` path (gRPC `NotFound`, feature 009) and the
/// legacy in-band `ServerError` message. Callers use this instead of
/// substring-matching the message text.
pub fn is_not_found(&self) -> bool {
match self {
ClientError::Status(status) => status.code() == tonic::Code::NotFound,
ClientError::ServerError(msg) => msg.contains("not found"),
_ => false,
}
}
}

#[cfg(test)]
mod tests {
use super::*;

fn guardian_status() -> tonic::Status {
// Mirrors the object the server attaches to Status.details (feature 009).
let details = serde_json::json!({
"code": "account_paused",
"message": "This account is paused and can't approve transactions right now.",
"meta": { "retryable": false }
})
.to_string()
.into_bytes();
tonic::Status::with_details(
tonic::Code::FailedPrecondition,
"This account is paused and can't approve transactions right now.",
details.into(),
)
}

#[test]
fn accessors_parse_guardian_status_details() {
let err: ClientError = guardian_status().into();
assert_eq!(err.guardian_code().as_deref(), Some("account_paused"));
assert_eq!(
err.user_message().as_deref(),
Some("This account is paused and can't approve transactions right now.")
);
assert_eq!(err.guardian_meta().unwrap()["retryable"], false);
assert!(!err.is_not_found());
}

#[test]
fn user_message_falls_back_to_status_message_without_details() {
let err: ClientError = tonic::Status::new(tonic::Code::Unavailable, "fallback msg").into();
assert_eq!(err.guardian_code(), None);
assert_eq!(err.user_message().as_deref(), Some("fallback msg"));
}

#[test]
fn is_not_found_detects_grpc_not_found_and_legacy_message() {
let status: ClientError = tonic::Status::new(tonic::Code::NotFound, "x").into();
assert!(status.is_not_found());
assert!(ClientError::ServerError("Delta not found for account".into()).is_not_found());
assert!(!ClientError::ServerError("boom".into()).is_not_found());
}
}
9 changes: 5 additions & 4 deletions crates/miden-multisig-client/src/client/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ use std::collections::HashSet;

use base64::Engine;
use guardian_client::{
AuthConfig, ClientError as GuardianClientError, MidenEcdsaAuth, MidenFalconRpoAuth,
TryIntoTxSummary, auth_config::AuthType,
AuthConfig, MidenEcdsaAuth, MidenFalconRpoAuth, TryIntoTxSummary, auth_config::AuthType,
};
use guardian_shared::SignatureScheme;
use miden_client::account::Account;
Expand Down Expand Up @@ -393,8 +392,10 @@ impl MultisigClient {
.await
{
Ok(resp) => resp,
Err(GuardianClientError::ServerError(msg)) if msg.contains("not found") => {
// No new deltas since current nonce - this is not an error
// No new deltas since current nonce — not an error. Errors now
// arrive as a gRPC Status (feature 009); branch on the not-found
// signal rather than substring-matching the message.
Err(e) if e.is_not_found() => {
return Ok(());
}
Comment on lines +398 to 400

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP 'fn get_delta_since' -A30 crates
rg -nP 'DeltaNotFound|AccountNotFound|StateNotFound' -A3 crates/server/src/error.rs
rg -nP 'fn code\b' -A40 crates/server/src/error.rs | rg -n 'not_found|delta|account'

Repository: OpenZeppelin/guardian

Length of output: 17079


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the client-side delta handling and the server-side error path.
sed -n '360,430p' crates/miden-multisig-client/src/client/account.rs

echo '---'
sed -n '1,220p' crates/server/src/services/get_delta_since.rs

echo '---'
rg -n 'resolve_account|AccountNotFound|DeltaNotFound|guardian_code|is_not_found' crates/server/src -A4 -B4

Repository: OpenZeppelin/guardian

Length of output: 50377


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the server's gRPC error conversion and any helper the client can branch on.
sed -n '140,230p' crates/server/src/error.rs

echo '---'
rg -n 'guardian_code|error_code|tonic::Status|metadata' crates/miden-multisig-client crates/client crates/server/src -A3 -B3

Repository: OpenZeppelin/guardian

Length of output: 50378


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the server's gRPC error conversion and any helper the client can branch on.
sed -n '140,230p' crates/server/src/error.rs

echo '---'
rg -n 'guardian_code|error_code|tonic::Status|metadata' crates/miden-multisig-client crates/client crates/server/src -A3 -B3

Repository: OpenZeppelin/guardian

Length of output: 50378


Differentiate delta_not_found from other NotFound errors.
get_delta_since can return both DeltaNotFound (no newer canonical deltas) and AccountNotFound from resolve_account; both become gRPC NotFound, so this branch also hides a missing-account error. Switch on the stable Guardian error code (delta_not_found vs account_not_found) instead of the transport-level not-found signal.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/miden-multisig-client/src/client/account.rs` around lines 398 - 400,
The `get_delta_since` error handling in `AccountClient` is treating all
transport-level NotFound errors the same, which can mask a missing-account case
from `resolve_account`. Update the match branch around `Err(e) if
e.is_not_found()` to distinguish by the stable Guardian error code
(`delta_not_found` versus `account_not_found`) instead of `is_not_found()`, so
only the no-newer-deltas case returns `Ok(())` and the account-missing path is
handled separately.

Err(e) => {
Expand Down
8 changes: 4 additions & 4 deletions crates/server/src/api/dashboard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1275,11 +1275,11 @@ mod tests {
"expected stable code on {path}",
);
assert_eq!(
body["missing_permissions"],
body["meta"]["missing_permissions"],
serde_json::json!(["dashboard:read"]),
"expected missing_permissions on {path}",
);
assert_eq!(body["retryable"], serde_json::Value::Bool(false));
assert_eq!(body["meta"]["retryable"], serde_json::Value::Bool(false));
}
}

Expand Down Expand Up @@ -1433,7 +1433,7 @@ mod tests {
let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
assert_eq!(body["code"], "GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION");
assert_eq!(
body["missing_permissions"],
body["meta"]["missing_permissions"],
serde_json::json!(["accounts:pause"])
);

Expand Down Expand Up @@ -1912,7 +1912,7 @@ mod tests {
let body: serde_json::Value = read_json(response).await;
assert_eq!(body["code"], "GUARDIAN_INSUFFICIENT_OPERATOR_PERMISSION");
assert_eq!(
body["missing_permissions"],
body["meta"]["missing_permissions"],
serde_json::json!(["accounts:pause"])
);
}
Expand Down
66 changes: 9 additions & 57 deletions crates/server/src/api/grpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,7 @@ impl Guardian for GuardianService {
ack_commitment: response.ack_commitment,
error_code: String::new(),
})),
Err(e) => Ok(Response::new(ConfigureResponse {
success: false,
message: e.to_string(),
ack_pubkey: String::new(),
ack_commitment: String::new(),
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand Down Expand Up @@ -122,13 +116,7 @@ impl Guardian for GuardianService {
ack_sig: Some(response.delta.ack_sig),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(PushDeltaResponse {
success: false,
message: e.to_string(),
delta: None,
ack_sig: None,
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand All @@ -154,12 +142,7 @@ impl Guardian for GuardianService {
delta: Some(delta_to_proto(&response.delta)),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(GetDeltaResponse {
success: false,
message: e.to_string(),
delta: None,
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand All @@ -185,12 +168,7 @@ impl Guardian for GuardianService {
merged_delta: Some(delta_to_proto(&response.merged_delta)),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(GetDeltaSinceResponse {
success: false,
message: e.to_string(),
merged_delta: None,
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand All @@ -215,12 +193,7 @@ impl Guardian for GuardianService {
state: Some(state_to_proto(&response.state)),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(GetStateResponse {
success: false,
message: e.to_string(),
state: None,
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand Down Expand Up @@ -268,13 +241,7 @@ impl Guardian for GuardianService {
commitment: response.commitment,
error_code: String::new(),
})),
Err(e) => Ok(Response::new(PushDeltaProposalResponse {
success: false,
message: e.to_string(),
delta: None,
commitment: String::new(),
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand All @@ -297,12 +264,7 @@ impl Guardian for GuardianService {
proposals: response.proposals.iter().map(delta_to_proto).collect(),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(GetDeltaProposalsResponse {
success: false,
message: e.to_string(),
proposals: vec![],
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand All @@ -326,12 +288,7 @@ impl Guardian for GuardianService {
proposal: Some(delta_to_proto(&response.proposal)),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(GetDeltaProposalResponse {
success: false,
message: e.to_string(),
proposal: None,
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand Down Expand Up @@ -360,12 +317,7 @@ impl Guardian for GuardianService {
delta: Some(delta_to_proto(&response.delta)),
error_code: String::new(),
})),
Err(e) => Ok(Response::new(SignDeltaProposalResponse {
success: false,
message: e.to_string(),
delta: None,
error_code: e.code().to_string(),
})),
Err(e) => Err(Status::from(e)),
}
}

Expand Down
44 changes: 25 additions & 19 deletions crates/server/src/api/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,6 @@ pub struct ConfigureResponse {
pub code: Option<&'static str>,
}

#[derive(Serialize, utoipa::ToSchema)]
pub struct ErrorResponse {
pub success: bool,
pub code: &'static str,
pub error: String,
}

/// Configure (register) an account with its authorization set and
/// initial state. Requires the signed `x-pubkey` / `x-signature` /
/// `x-timestamp` auth headers.
Expand All @@ -141,14 +134,18 @@ pub async fn configure(
let request_payload = match request_payload_from_serializable(&payload) {
Ok(request_payload) => request_payload,
Err(e) => {
// Log the raw detail; return only the user-safe message + stable
// code (feature 009-human-readable-errors).
let err = GuardianError::InvalidInput(e);
tracing::debug!(code = err.code(), detail = %err, "configure request invalid");
return (
StatusCode::BAD_REQUEST,
Json(ConfigureResponse {
success: false,
message: e,
message: err.user_message().to_string(),
ack_pubkey: None,
ack_commitment: None,
code: None,
code: Some(err.code()),
}),
);
}
Expand All @@ -168,16 +165,25 @@ pub async fn configure(
code: None,
}),
),
Err(e) => (
e.http_status(),
Json(ConfigureResponse {
success: false,
message: e.to_string(),
ack_pubkey: None,
ack_commitment: None,
code: Some(e.code()),
}),
),
Err(e) => {
// The diagnostic Display string is logged, not returned; the wire
// carries only the user-safe message + stable code (feature 009).
if e.http_status().is_server_error() {
tracing::error!(code = e.code(), detail = %e, "configure failed (5xx)");
} else {
tracing::debug!(code = e.code(), detail = %e, "configure rejected (4xx)");
}
(
e.http_status(),
Json(ConfigureResponse {
success: false,
message: e.user_message().to_string(),
ack_pubkey: None,
ack_commitment: None,
code: Some(e.code()),
}),
)
}
}
}

Expand Down
Loading
Loading