feat: promote 'UncategorizedError' in ledger to a top-level anyhow::Error #197
Conversation
…rror This makes the 'BlockValidation' a 3-variant enum, where the `Exception` case reflects exceptional scenarii that aren't expected to occur as part of the normal operations. Signed-off-by: KtorZ <[email protected]>
WalkthroughAlrighty, here's the lowdown: The changes overhaul the block validation error handling in the ledger rules. The Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant ValidateBlockStage
participant BlockValidation
participant Store
Worker->>ValidateBlockStage: roll_forward(point, raw_block)
ValidateBlockStage->>BlockValidation: execute(raw_block)
BlockValidation-->>ValidateBlockStage: Valid(()) / Invalid(details) / Err(error)
ValidateBlockStage-->>Worker: Ok(None) / Ok(Some(details)) / Err(error)
Worker->>Worker: Match on result
alt Valid
Worker->>Store: forward_state()
Worker->>Worker: BlockValidated
else Invalid
Worker->>Worker: BlockValidationFailed
else Err
Worker->>Worker: propagate error
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/amaru-ledger/src/rules/block.rs (1)
62-73: Nit: makebail&anyhowgeneric overimpl Into<anyhow::Error>Hand-rolling two helpers that ultimately wrap
anyhow::Erroris grand, but we can shave a yak here:-pub fn bail(msg: String) -> Self { - BlockValidation::Err(anyhow::Error::msg(msg)) +pub fn bail<M: Into<anyhow::Error>>(msg: M) -> Self { + BlockValidation::Err(msg.into()) }Keeps call-sites ergonomic (
bail("oops")) and avoids the mandatoryto_string()dance.crates/amaru-ledger/src/rules/block/ex_units.rs (1)
19-23: Unused genericElooks like cargo cultingThe function never returns
BlockValidation::Err(E), so the type parameter is effectively dead weight here.
Either:
- Remove the generic and fix the callers, or
- Add an early-exit path that can actually yield an
Err(E)(e.g., by parameterising the fold or the protocol lookup).Leaning option-1 unless you’ve a concrete plan for exotic error flavours down the track.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/amaru-ledger/Cargo.toml(1 hunks)crates/amaru-ledger/src/rules.rs(1 hunks)crates/amaru-ledger/src/rules/block.rs(5 hunks)crates/amaru-ledger/src/rules/block/body_size.rs(4 hunks)crates/amaru-ledger/src/rules/block/ex_units.rs(3 hunks)crates/amaru-ledger/src/rules/block/header_size.rs(2 hunks)crates/amaru/src/stages/ledger.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
crates/amaru-ledger/src/rules/block/body_size.rs (1)
crates/amaru-ledger/src/rules/block.rs (1)
anyhow(67-72)
crates/amaru-ledger/src/rules/block/header_size.rs (1)
crates/amaru-ledger/src/rules/block.rs (1)
anyhow(67-72)
crates/amaru-ledger/src/rules/block/ex_units.rs (1)
crates/amaru-kernel/src/lib.rs (2)
ex_units(745-745)ex_units(749-755)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: coverage
🔇 Additional comments (11)
crates/amaru-ledger/Cargo.toml (1)
21-21: Crikey! Adding anyhow for error handling - good stuff!Adding anyhow as a workspace dependency to streamline error handling. This supports the overall PR objective of promoting 'UncategorizedError' to a top-level anyhow::Error.
crates/amaru-ledger/src/rules.rs (1)
184-184: Updating test assertions to match the new generic BlockValidation - beauty!The test now properly checks for
BlockValidation::Valid(())pattern which matches the updated return type of the validate_block function. This aligns with the genericBlockValidation<A, E>structure that carries a payload in the Valid variant.crates/amaru-ledger/src/rules/block/body_size.rs (5)
21-21: Leveled up the function signature like a proper RPG character!Updated the function signature to be generic over error type
E, returningBlockValidation<(), E>instead of the previous non-generic type. This aligns with the PR's goal of promoting better error handling.
32-33: Clean return value with unit type - no worries!Updated to return
BlockValidation::Valid(())with the unit value to match the new generic type structure. Spot on!
51-52: Updated imports to match the new structure - fair dinkum!Direct import of BlockValidation keeps the code clean and consistent with other modules.
65-68: Test case matcher updated like a well-oiled machine!Updated test case matcher to handle the new BlockValidation structure. The pattern matching looks great with the proper destructuring of the InvalidBlockDetails variant.
69-71: Test function return type alignment - that's the ticket!Updated the test function to explicitly return
BlockValidation<(), anyhow::Error>, matching the function's new signature. This keeps your tests in sync with your implementation - as balanced as perfectly matched health and mana bars!crates/amaru-ledger/src/rules/block/header_size.rs (4)
22-22: Return type update with anyhow::Error - absolute ripper!Changed the return type from the non-generic
BlockValidationtoBlockValidation<(), anyhow::Error>, aligning with the improved error handling approach. This matches the PR's focus on using anyhow for better error context.
26-27: Proper error propagation with context - like a well-constructed quest line!These changes refine the error handling by:
- Using
map(BlockValidation::Valid)for successful conversion- Using
unwrap_or_elsewithBlockValidation::anyhow(e)and.context()to provide rich error information- Using the
?operator for error propagationThis gives exceptional error handling - better than the shields in Dune!
35-36: Updated return value to match generic structure - deadly!Now returning
BlockValidation::Valid(())with the unit type to match the generic type parameter. Consistent with the changes in other validation functions.
56-57: Updated test assertion to match the new Valid variant - you beauty!Test now correctly asserts against
BlockValidation::Valid(())with the unit payload, aligning with the updated enum structure. Keeping your tests in line like a proper formation in Final Fantasy Tactics!
| pub fn context<C>(self, context: C) -> Self | ||
| where | ||
| C: std::fmt::Display + Send + Sync + 'static, | ||
| { | ||
| match self { | ||
| BlockValidation::Err(err) => BlockValidation::Err(err.context(context)), | ||
| BlockValidation::Invalid { .. } | BlockValidation::Valid { .. } => self, | ||
| } | ||
| } |
There was a problem hiding this comment.
Bracket-vs-Brace mix-up breaks pattern matching, chief
You’re pattern-matching tuple variants with struct syntax ({ … }).
That’ll make the compiler spit the dummy and the build will stop dead in its tracks, long before we can yell “It’s over 9000!”.
- BlockValidation::Err(err) => BlockValidation::Err(err.context(context)),
- BlockValidation::Invalid { .. } | BlockValidation::Valid { .. } => self,
+ BlockValidation::Err(err) => BlockValidation::Err(err.context(context)),
+ BlockValidation::Invalid(_) | BlockValidation::Valid(_) => self,Same salad sneaks in a few lines below (see Termination impl).
Swap the braces for parentheses wherever you’re matching on Valid or Invalid.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn context<C>(self, context: C) -> Self | |
| where | |
| C: std::fmt::Display + Send + Sync + 'static, | |
| { | |
| match self { | |
| BlockValidation::Err(err) => BlockValidation::Err(err.context(context)), | |
| BlockValidation::Invalid { .. } | BlockValidation::Valid { .. } => self, | |
| } | |
| } | |
| pub fn context<C>(self, context: C) -> Self | |
| where | |
| C: std::fmt::Display + Send + Sync + 'static, | |
| { | |
| match self { | |
| BlockValidation::Err(err) => BlockValidation::Err(err.context(context)), | |
| BlockValidation::Invalid(_) | BlockValidation::Valid(_) => self, | |
| } | |
| } |
| fn report(self) -> ExitCode { | ||
| match self { | ||
| BlockValidation::Valid => ControlFlow::Continue(()), | ||
| BlockValidation::Invalid(e) => ControlFlow::Break(e), | ||
| Self::Valid { .. } => ExitCode::SUCCESS, | ||
| Self::Invalid { .. } | Self::Err { .. } => ExitCode::FAILURE, | ||
| } |
There was a problem hiding this comment.
Termination impl suffers the same brace-itis
Second verse, same as the first – tuple variants need “( … )”, not “{ … }”.
- Self::Valid { .. } => ExitCode::SUCCESS,
- Self::Invalid { .. } | Self::Err { .. } => ExitCode::FAILURE,
+ Self::Valid(_) => ExitCode::SUCCESS,
+ Self::Invalid(_) | Self::Err(_) => ExitCode::FAILURE,Without this, cargo check will rage-quit faster than you can say “mate, where’s my stubby”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn report(self) -> ExitCode { | |
| match self { | |
| BlockValidation::Valid => ControlFlow::Continue(()), | |
| BlockValidation::Invalid(e) => ControlFlow::Break(e), | |
| Self::Valid { .. } => ExitCode::SUCCESS, | |
| Self::Invalid { .. } | Self::Err { .. } => ExitCode::FAILURE, | |
| } | |
| fn report(self) -> ExitCode { | |
| match self { | |
| Self::Valid(_) => ExitCode::SUCCESS, | |
| Self::Invalid(_) | Self::Err(_) => ExitCode::FAILURE, | |
| } |
| None => BlockValidationResult::BlockValidated { | ||
| point: point.clone(), | ||
| span: restore_span(span), | ||
| }, | ||
| Some(_err) => BlockValidationResult::BlockValidationFailed { | ||
| point: point.clone(), | ||
| span: restore_span(span), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
We’re losing the juicy error details on the cutting-room floor
Some(_err) gets mapped to BlockValidationFailed, but the actual InvalidBlockDetails vanish into the void.
If downstream consumers ever want to surface what went wrong (UX, metrics, tracing), we’ll be left scratching our heads.
Consider piping the details through:
- Some(_err) => BlockValidationResult::BlockValidationFailed {
- point: point.clone(),
- span: restore_span(span),
- },
+ Some(details) => BlockValidationResult::BlockValidationFailed {
+ point: point.clone(),
+ span: restore_span(span),
+ // new field? or wrap `details` in the enum variant
+ details,
+ },Naturally this means tweaking BlockValidationResult and any callers, but future-you will thank present-you when the dashboard lights up.
Committable suggestion skipped: line range outside the PR's diff.
|
Thank you for making this change! I really do think this is much nicer to consume for the call site and clearly communicates the intention. Not sure why that test is failing in the coverage CI run though 😛 |
I have no idea either 😅 ... and it's slightly worrying that it fails on the coverage but not on the classic ones. |
| } | ||
| } | ||
|
|
||
| impl<A, E> Termination for BlockValidation<A, E> { |
There was a problem hiding this comment.
This is only useful for the tests? Termination semantic is A trait for implementing arbitrary return types in the main function. so feels a bit unnatural to have it implemented at the BlockValidation level?
There was a problem hiding this comment.
Indeed, this is used to satisfy some of the test code.
Although, I don't think the semantic is particularly weird here. On the contrary.
| .try_into() | ||
| .unwrap_or_else(|_| panic!("Failed to convert u32 to usize")); | ||
| .map(BlockValidation::Valid) | ||
| .unwrap_or_else(|e| BlockValidation::anyhow(e).context("failed to convert u32 to usize"))?; |
There was a problem hiding this comment.
With a proper error type we wouldn't need to use anyhow here?
There was a problem hiding this comment.
Yes, but those are exceptions; and I don't think we really care to turn them into user facing errors.
So it seems to be a perfect use case for anyhow?
| None => { | ||
| return BlockValidation::Invalid(InvalidBlockDetails::UncategorizedError(format!( | ||
| // TODO: Define a proper error for this. | ||
| return BlockValidation::bail(format!( |
There was a problem hiding this comment.
If we had a proper error here there wouldn't be a need for anyhow at all?
|
I've seen this failure happen sporadically on windows too. Not sure why this test would be flaky. |
Codecov ReportAttention: Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
This makes the 'BlockValidation' a 3-variant enum, where
Exceptioncase reflects exceptional scenarii that aren't expected to occur as part of the normal operations.Summary by CodeRabbit
New Features
Refactor
Chores