Skip to content

(draft) use n0-error #3511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

(draft) use n0-error #3511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
68466be
feat(n0-error): introduce local n0-error path dep and start migration…
Frando Oct 7, 2025
9476e69
chore: fix n0-error path to ../../n0-error; cargo check and continue …
Frando Oct 7, 2025
765f80c
updates
Frando Oct 8, 2025
135b357
chore(n0-error): adapt to new #[error(...)] attributes\n\n- Replace #…
Frando Oct 8, 2025
5ef4d8f
chore(n0-error): consolidate attributes and use top-level defaults wh…
Frando Oct 8, 2025
b78ecce
updates
Frando Oct 8, 2025
8333928
fmt
Frando Oct 8, 2025
face59e
use context
Frando Oct 8, 2025
e305b70
wip
Frando Oct 8, 2025
8107605
chore: fmt
Frando Oct 8, 2025
abdf366
cleanup
Frando Oct 8, 2025
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
86 changes: 79 additions & 7 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions iroh-base/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ url = { version = "2.5.3", features = ["serde"], optional = true }
postcard = { version = "1", default-features = false, features = ["alloc", "use-std", "experimental-derive"], optional = true }
rand_core = { version = "0.9.3", optional = true }
serde = { version = "1", features = ["derive", "rc"] }
snafu = { version = "0.8.5", features = ["rust_1_81"], optional = true }
n0-snafu = "0.2.2"
n0-error = { path = "../../n0-error", optional = true }
nested_enum_utils = "0.2.0"

[dev-dependencies]
Expand All @@ -44,15 +43,15 @@ key = [
"dep:ed25519-dalek",
"dep:url",
"dep:derive_more",
"dep:snafu",
"dep:n0-error",
"dep:data-encoding",
"dep:rand_core",
"relay",
]
relay = [
"dep:url",
"dep:derive_more",
"dep:snafu",
"dep:n0-error",
]

[package.metadata.docs.rs]
Expand Down
23 changes: 9 additions & 14 deletions iroh-base/src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@ use std::{
use curve25519_dalek::edwards::CompressedEdwardsY;
pub use ed25519_dalek::{Signature, SignatureError};
use ed25519_dalek::{SigningKey, VerifyingKey};
use nested_enum_utils::common_fields;
use rand_core::CryptoRng;
use serde::{Deserialize, Serialize};
use snafu::{Backtrace, Snafu};

/// A public key.
///
Expand Down Expand Up @@ -203,25 +201,22 @@ impl Display for PublicKey {
}

/// Error when deserialising a [`PublicKey`] or a [`SecretKey`].
#[common_fields({
backtrace: Option<Backtrace>,
#[snafu(implicit)]
span_trace: n0_snafu::SpanTrace,
})]
#[derive(Snafu, Debug)]
#[n0_error::add_location]
#[derive(n0_error::Error)]
#[error(from_sources, std_sources)]
#[allow(missing_docs)]
#[snafu(visibility(pub(crate)))]
#[non_exhaustive]
pub enum KeyParsingError {
/// Error when decoding.
#[snafu(transparent)]
#[error(transparent)]
Decode { source: data_encoding::DecodeError },
/// Error when decoding the public key.
#[snafu(transparent)]
#[error(transparent)]
Key {
source: ed25519_dalek::SignatureError,
},
/// The encoded information had the wrong length.
#[snafu(display("invalid length"))]
#[display("invalid length")]
DecodeInvalidLength {},
}

Expand Down Expand Up @@ -353,14 +348,14 @@ fn decode_base32_hex(s: &str) -> Result<[u8; 32], KeyParsingError> {
let input = s.to_ascii_uppercase();
let input = input.as_bytes();
if data_encoding::BASE32_NOPAD.decode_len(input.len())? != bytes.len() {
return Err(DecodeInvalidLengthSnafu.build());
return Err(KeyParsingError::decode_invalid_length());
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be good if we can restrict these methods to be crate only, to not make them part of the public api

Copy link
Contributor

Choose a reason for hiding this comment

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

thinking about this some more, we will want to be able to be able to configure visibility on a variant level, eg this error

enum MyError {
  #[error(visibility(pub(crate))]
  InternalOne,
  // only visible in this module by default
  InternalTwo,
  // usable externally
  #[error(visibility(pub))]
  User { source: Box<dyn Error> },
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this means rust-analyzer "find-all-references" on KeyParsingError::DecodeInvalidLength is not going to find anything?

That's one of the main usability issues I have with snafu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is true unfortunately. And I agree that fixing this would be a major improvement.
Unfortunately this has tradeoffs. We want to have a location (or backtrace or similar) field be included in all errors. So we have, I fear, only these options:

  • Have the field be part of each enum variant (current situation). Then we can initialize..
    • with a function (current PR) or a separate struct (eg the ...Snafu structs). Both are not find-all-references compatible.
    • manually with e.g. KeyParsingError:::DecodeInvalidLength { location: Default::default() }. This works with find-all-references but is more verbose at each error construction site.
  • Wrap errors in a generic outer err. So e.g. fn parse_key() -> Result<Key, Err<KeyParsingError>>. Then the location context can be tracked in the outer error and could be initialized like KeyParingsError::DecodeInvalidLength.build(). However, IIRC this leads to trouble with the questionmark operator in some cases (can maybe be solved - I'll give it another try) and of course the signatures look more complex throughout the board.

I am undecided yet. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

First note that I'm spouting ideas without much knowledge of how things need to be wired up. So I might be suggesting nonsense.

Maybe 1.2 is workable. I think you can also write this as KeyParsingError::DecodeInvalidLength { ..Default::default() } which is nice if you have other fields to initialise on the enum.

Maybe another option could be to use yet another macro to create the structs itself, like Err!(KeyParsingError::DecodeInvalidLength).

It seems like 2 is much more the "Error + ErrorKind" type of approach. At first it does look appealing. But yeah, depends on the ergonomics of all the edge cases. Also matching on errors could become a little more awkward, I really like being able to write match statements without having to do the nested match err.kind() that std::io::Error makes you do.

Copy link
Member Author

@Frando Frando Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

1.2 could also be MyError::Foo { field, loc: loc() } with use n0_error::loc if we aim for more brevity in the common case.

The generic error matching wouldn't be too bad: we could define it as struct Err<K>(pub K, Location) then you could match on err.0. I'll give it a try some time soon.

}
data_encoding::BASE32_NOPAD.decode_mut(input, &mut bytes)
};
match res {
Ok(len) => {
if len != PublicKey::LENGTH {
return Err(DecodeInvalidLengthSnafu.build());
return Err(KeyParsingError::decode_invalid_length());
}
}
Err(partial) => return Err(partial.error.into()),
Expand Down
13 changes: 7 additions & 6 deletions iroh-base/src/relay_url.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{fmt, ops::Deref, str::FromStr, sync::Arc};

use n0_error::{ResultExt, StackErrorExt};
use serde::{Deserialize, Serialize};
use snafu::{Backtrace, ResultExt, Snafu};
use url::Url;

/// A URL identifying a relay server.
Expand Down Expand Up @@ -39,11 +39,12 @@ impl From<Url> for RelayUrl {
}

/// Can occur when parsing a string into a [`RelayUrl`].
#[derive(Debug, Snafu)]
#[snafu(display("Failed to parse"))]
#[n0_error::add_location]
#[derive(n0_error::Error)]
#[display("Failed to parse")]
pub struct RelayUrlParseError {
source: url::ParseError,
backtrace: Option<Backtrace>,
#[error(from, std_err)]
parse_error: url::ParseError,
}

/// Support for parsing strings directly.
Expand All @@ -54,7 +55,7 @@ impl FromStr for RelayUrl {
type Err = RelayUrlParseError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let inner = Url::from_str(s).context(RelayUrlParseSnafu)?;
let inner = Url::from_str(s).context(RelayUrlParseError::from)?;
Ok(RelayUrl::from(inner))
}
}
Expand Down
30 changes: 11 additions & 19 deletions iroh-base/src/ticket.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use std::{collections::BTreeSet, net::SocketAddr};

use nested_enum_utils::common_fields;
use serde::{Deserialize, Serialize};
use snafu::{Backtrace, Snafu};

use crate::{key::NodeId, relay_url::RelayUrl};

Expand Down Expand Up @@ -50,7 +49,7 @@ pub trait Ticket: Sized {
fn deserialize(str: &str) -> Result<Self, ParseError> {
let expected = Self::KIND;
let Some(rest) = str.strip_prefix(expected) else {
return Err(KindSnafu { expected }.build());
return Err(ParseError::kind(expected));
};
let bytes = data_encoding::BASE32_NOPAD.decode(rest.to_ascii_uppercase().as_bytes())?;
let ticket = Self::from_bytes(&bytes)?;
Expand All @@ -59,30 +58,23 @@ pub trait Ticket: Sized {
}

/// An error deserializing an iroh ticket.
#[common_fields({
backtrace: Option<Backtrace>,
#[snafu(implicit)]
span_trace: n0_snafu::SpanTrace,
})]
#[derive(Debug, Snafu)]
#[n0_error::add_location]
#[derive(n0_error::Error)]
#[error(from_sources, std_sources)]
#[allow(missing_docs)]
#[snafu(visibility(pub(crate)))]
#[non_exhaustive]
pub enum ParseError {
/// Found a ticket with the wrong prefix, indicating the wrong kind.
#[snafu(display("wrong prefix, expected {expected}"))]
Kind {
/// The expected prefix.
expected: &'static str,
},
#[display("wrong prefix, expected {expected}")]
Kind { expected: &'static str },
/// This looks like a ticket, but postcard deserialization failed.
#[snafu(transparent)]
#[error(transparent)]
Postcard { source: postcard::Error },
/// This looks like a ticket, but base32 decoding failed.
#[snafu(transparent)]
#[error(transparent)]
Encoding { source: data_encoding::DecodeError },
/// Verification of the deserialized bytes failed.
#[snafu(display("verification failed: {message}"))]
#[display("verification failed: {message}")]
Verify { message: &'static str },
}

Expand All @@ -92,13 +84,13 @@ impl ParseError {
///
/// Indicate the expected prefix.
pub fn wrong_prefix(expected: &'static str) -> Self {
KindSnafu { expected }.build()
ParseError::kind(expected)
}

/// Return a `ParseError` variant that indicates verification of the
/// deserialized bytes failed.
pub fn verification_failed(message: &'static str) -> Self {
VerifySnafu { message }.build()
ParseError::verify(message)
}
}

Expand Down
3 changes: 1 addition & 2 deletions iroh-relay/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ webpki_types = { package = "rustls-pki-types", version = "1.12" }
data-encoding = "2.6.0"
lru = "0.16"
z32 = "1.0.3"
snafu = { version = "0.8.5", features = ["rust_1_81"] }
n0-snafu = "0.2.2"
n0-error = { path = "../../n0-error" }
nested_enum_utils = "0.2.0"

# server feature
Expand Down
Loading
Loading