feat: finer grained error codes#745
Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
|
KICS version: v2.1.16
|
…retty quickly. Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
m2ux
left a comment
There was a problem hiding this comment.
PR Review Summary
PR: #745 — feat: finer grained error codes
Issue: PM-21798
Reviewer: Workflow-based review (retrospective)
Date: 2026-02-23
Executive Summary
Solid implementation that correctly resolves the u8 error code collision between MalformedError::UnknownError and SystemTransactionError::IllegalPayout (both formerly mapped to 139). The enum expansion, code reassignment, and brute-force regression test are well-designed. Two documentation gaps and a scope concern are noted below.
Overall Rating: Approve (with comments)
Code Review Findings
| Severity | Category | Finding | Location |
|---|---|---|---|
| Medium | Documentation | Range reservation comment // Reserved from [150-255) is now inaccurate — codes 166–210 are actively used |
types.rs:406 |
| Medium | Documentation | Range reservation comment // Reserved from [100-150) no longer holds — transaction error codes now span [100–139] + [166–210] |
types.rs:312 |
| Low | Documentation | Non-contiguous u8 ranges (e.g., InvalidError using 100–109 and 193–200) lack inline rationale explaining the backward-compatibility motivation |
types.rs, From<LedgerApiError> for u8 impl |
| Low | Documentation | Change file doesn't explicitly mention that all SystemTransactionError codes shifted from 139–148 to 201–210 — downstream consumers may miss this |
changes/added/finer-grained-error-codes.md |
Strengths
- Brute-force regression test (
error_codes_are_unique) is self-adapting via SCALE decoding — automatically covers new variants without manual maintenance - Defensive fallback with
log::warn!for unmapped variants provides forward compatibility - Backward compatibility preserved for existing codes 0–138
- Clean variant naming — intermediate enum variants mirror source enum names exactly
Test Review Findings
| Area | Assessment | Notes |
|---|---|---|
| Collision prevention | Covered | Core requirement validated by error_codes_are_unique |
| Backward compatibility | Gap (medium) | No assertion that specific existing codes (e.g., EffectsMismatch => 100) are unchanged |
| Per-variant correctness | Gap (low) | Specific code assignments not asserted (uniqueness is, values are not) |
| Fallback logging | Acceptable gap | Requires a future unknown variant to test — inherently difficult |
Recommendation: Consider adding a few spot-check assertions for sentinel values (e.g., assert_eq!(u8::from(InvalidError::EffectsMismatch.into()), 100)) to catch accidental enum reordering.
Validation Results
| Check | Result |
|---|---|
| CI — Build | Pass |
| CI — Tests (unit + E2E) | Pass |
| CI — Formatting & Linting | Pass |
| CI — Metadata Check | Pass |
| CI — Merge Queue | Passed and merged |
All CI checks passed. The PR was merged via merge queue.
Scope Observation
The Rust toolchain bump (1.90 → 1.93) and cargo-nextest pin (0.9.128) are bundled with this PR but are not required by any feature used in the error code changes. For future PRs, consider separating infrastructure changes (toolchain, CI config) into dedicated PRs to simplify bisection and review.
Action Items
Should Address (follow-up):
- Update the two range reservation comments in
types.rsto reflect actual code allocation - Add a note to the change file about
SystemTransactionErrorcode reassignment (139–148 → 201–210)
Nice to Have:
3. Add inline comment explaining why new InvalidError codes start at 193 instead of 110
4. Add backward-compatibility spot-check assertions to the test
Signed-off-by: Mike Clay <mike.clay@shielded.io>
Signed-off-by: Mike Clay <mike.clay@shielded.io>
Overview
To aid debugging / developer experience.
Also corrects rust-toolkit.toml
Breaking changes:
🗹 TODO before merging
📌 Submission Checklist
🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links