-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
chore(reth-storage-errors): no_std support #10011
chore(reth-storage-errors): no_std support #10011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
crates/storage/errors/Cargo.toml
Outdated
@@ -15,8 +15,9 @@ alloy-rlp.workspace = true | |||
reth-primitives.workspace = true | |||
reth-fs-util.workspace = true | |||
|
|||
thiserror-no-std = { workspace = true, default-features = false } | |||
thiserror-no-std = { workspace = true, default-features = false, optional = true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can actually remove this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
crates/storage/errors/src/db.rs
Outdated
pub enum DatabaseError { | ||
/// Failed to open the database. | ||
#[error("failed to open the database: {0}")] | ||
#[display(fmt="failed to open the database: {_0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt nit for every occurrence
#[display(fmt="failed to open the database: {_0}")] | |
#[display(fmt = "failed to open the database: {_0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…us error variants
b64c667
to
4e33140
Compare
@mattsse I think this is good to go if you could please take a look when you get a chance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last nit
/// Provider result type. | ||
pub type ProviderResult<Ok> = Result<Ok, ProviderError>; | ||
|
||
/// Bundled errors variants thrown by various providers. | ||
#[derive(Clone, Debug, thiserror_no_std::Error, PartialEq, Eq)] | ||
#[derive(Clone, Debug, Display, From, PartialEq, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to not use From here instead impl From
for the few variants manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the argument for this @mattsse ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same q
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't like the opt-out attributes, these from impls should be opt-in
crates/storage/errors/src/db.rs
Outdated
impl fmt::Display for DatabaseWriteError { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!( | ||
f, | ||
"write operation {} failed for key \"{}\" in table {}: {}", | ||
self.operation, | ||
reth_primitives::hex::encode(&self.key), | ||
self.table_name, | ||
self.info | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be possible to derive this too, check out example https://docs.rs/derive_more/latest/derive_more/derive.Display.html#custom-trait-bounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending @shekhirin and conflict
Towards #9478
Adds
no_std
support toreth-storage-errors
.Stacking PRs no longer necessary as implementing source for errors wrapped in error enum variants fixed the issues that were breaking tests.