feat: Add specific error codes for deserialization failures #121
Conversation
Signed-off-by: AvhiMaz <[email protected]>
WalkthroughThree new error variants were added to the DlpError enum to provide granular error context during delegation record deserialization. Corresponding deserialization functions were updated to return these specific errors instead of generic ProgramError::InvalidAccountData, and to_bytes validation was tightened to enforce exact data length requirements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/error.rs(1 hunks)src/state/utils/to_bytes.rs(1 hunks)src/state/utils/try_from_bytes.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T18:27:42.542Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 111
File: src/diff/types.rs:87-102
Timestamp: 2025-10-28T18:27:42.542Z
Learning: In Solana runtime, `account_info.data()` is guaranteed to have 8-byte alignment.
Applied to files:
src/state/utils/to_bytes.rs
🧬 Code graph analysis (1)
src/state/utils/to_bytes.rs (2)
src/state/commit_record.rs (1)
size_of(37-37)src/state/delegation_record.rs (1)
size_of(40-40)
🔇 Additional comments (1)
src/state/utils/to_bytes.rs (1)
9-11: Exact-length check removes panic risk but tightens the API contractComputing
expected_len = 8 + size_of::<Self>()and requiringdata.len() == expected_lenis a good safety improvement: it preventscopy_from_slicefrom panicking on mismatched buffer sizes and surfaces a clearDlpError::InvalidDataLengthinstead.This does slightly tighten behavior versus accepting arbitrary slice lengths, so it’s worth double-checking that all call sites pass exactly-sized buffers (e.g., full account data for these records) and don’t rely on any extra trailing space.
| #[error("Invalid data length for deserialization")] | ||
| InvalidDataLength = 18, | ||
| #[error("Invalid discriminator for delegation record")] | ||
| InvalidDiscriminator = 19, | ||
| #[error("Invalid delegation record deserialization")] | ||
| InvalidDelegationRecordData = 20, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
New DlpError variants are well-scoped and backwards compatible
Adding InvalidDataLength = 18, InvalidDiscriminator = 19, and InvalidDelegationRecordData = 20 with explicit discriminants preserves prior codes and cleanly encodes the three deserialization failure modes. The existing From<DlpError> for ProgramError impls automatically expose them to callers.
If these errors ever need to apply beyond delegation records, consider a slightly more generic name/message for InvalidDelegationRecordData, but that’s purely cosmetic.
🤖 Prompt for AI Agents
In src/error.rs around lines 44 to 49: the new DlpError variants
(InvalidDataLength = 18, InvalidDiscriminator = 19, InvalidDelegationRecordData
= 20) are correctly scoped and use explicit discriminants to preserve prior
codes; no functional changes required—leave these enum entries as-is, but if you
later want broader applicability consider renaming InvalidDelegationRecordData
to a more generic label and update its error message accordingly.
| if data.len() < 8 { | ||
| return Err(::solana_program::program_error::ProgramError::InvalidAccountData); | ||
| return Err($crate::error::DlpError::InvalidDataLength.into()); | ||
| } | ||
| if Self::discriminator().to_bytes().ne(&data[..8]) { | ||
| return Err(::solana_program::program_error::ProgramError::InvalidAccountData); | ||
| return Err($crate::error::DlpError::InvalidDiscriminator.into()); | ||
| } | ||
| bytemuck::try_from_bytes::<Self>(&data[8..]).or(Err( | ||
| ::solana_program::program_error::ProgramError::InvalidAccountData, | ||
| $crate::error::DlpError::InvalidDelegationRecordData.into(), | ||
| )) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Zero‑copy deserialization error mapping looks correct and consistent
Using DlpError::InvalidDataLength for data.len() < 8, InvalidDiscriminator for header mismatch, and InvalidDelegationRecordData for bytemuck::try_from_bytes{,_mut} failures cleanly separates the three failure modes and keeps the ProgramError surface unchanged via From<DlpError>. The immutable and mutable variants stay in sync.
If you want to tighten things up, you could switch from .or(Err(...)) to .map_err(|_| ... ) for slightly clearer intent, but behavior is already correct.
Also applies to: 21-29
🤖 Prompt for AI Agents
In src/state/utils/try_from_bytes.rs around lines 8 to 16 (and the corresponding
mutable variant at lines 21 to 29), replace the final
.or(Err($crate::error::DlpError::InvalidDelegationRecordData.into())) with
.map_err(|_| $crate::error::DlpError::InvalidDelegationRecordData.into()) so the
error mapping expresses intent more clearly while preserving identical behavior
and the same ProgramError conversion.
| if data.len() < 8 { | ||
| return Err(::solana_program::program_error::ProgramError::InvalidAccountData); | ||
| return Err($crate::error::DlpError::InvalidDataLength.into()); | ||
| } | ||
| if Self::discriminator().to_bytes().ne(&data[..8]) { | ||
| return Err(::solana_program::program_error::ProgramError::InvalidAccountData); | ||
| return Err($crate::error::DlpError::InvalidDiscriminator.into()); | ||
| } | ||
| Self::try_from_slice(&data[8..]).or(Err( | ||
| ::solana_program::program_error::ProgramError::InvalidAccountData, | ||
| $crate::error::DlpError::InvalidDelegationRecordData.into(), | ||
| )) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Borsh deserialization variant aligns well with the new error codes
The Borsh macro mirrors the zero‑copy behavior: InvalidDataLength for missing discriminator bytes, InvalidDiscriminator for header mismatch, and InvalidDelegationRecordData for any try_from_slice failure. This provides the same granularity for Borsh-backed types without changing the public signature.
If you later decide you need to distinguish Borsh length/format issues from other “bad payload” cases, you could introduce a more generic variant name (e.g., InvalidRecordData) or a struct-specific message, but that’s not required for this PR.
🤖 Prompt for AI Agents
In src/state/utils/try_from_bytes.rs around lines 42 to 50, ensure the Borsh
deserialization mirrors the zero‑copy error mapping: check for data length < 8
and return InvalidDataLength; verify the first 8 bytes match
Self::discriminator() and return InvalidDiscriminator on mismatch; then call
Self::try_from_slice(&data[8..]) and map any Err to InvalidDelegationRecordData;
implement these checks in this order and convert errors to the specified
$crate::error variants before returning.
1 participant
Problem
Deserialization failures all returned the same generic error, making debugging hard.
Solution
Added specific error types for:
Updated macros to use these errors and added a safety check to prevent panics during serialization.
Deploy Notes
Fixes #112
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.