Skip to content

chore: implement Display for error types#879

Open
Micah-Shallom wants to merge 1 commit intogetfloresta:masterfrom
Micah-Shallom:chore/implement-display-for-error-types
Open

chore: implement Display for error types#879
Micah-Shallom wants to merge 1 commit intogetfloresta:masterfrom
Micah-Shallom:chore/implement-display-for-error-types

Conversation

@Micah-Shallom
Copy link
Copy Markdown

@Micah-Shallom Micah-Shallom commented Mar 10, 2026

Description

Closes #667.

Implements Display for all error types that were missing it, allowing them to be integrated into custom error types using thiserror and #[error(transparent)].

Error Types Updated

  • MemoryDatabaseErrorfloresta-watch-only
  • HeaderExtErrorfloresta-chain
  • FlatChainstoreErrorfloresta-chain
  • BlockchainBuilderErrorfloresta-chain
  • LeafErrorKindfloresta-chain
  • slip132::Errorfloresta-node

AI was used to help identify error types missing Display implementations across the codebase

@moisesPompilio moisesPompilio added enhancement New feature or request code quality Generally improves code readability and maintainability labels Mar 10, 2026
Copy link
Copy Markdown
Collaborator

@moisesPompilio moisesPompilio left a comment

Choose a reason for hiding this comment

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

The commit is missing a description; it only has the title

use crate::ChainStore;
use crate::DatabaseError;
use crate::DiskBlockHeader;
use core::fmt;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the position of this import is not here; did you get to run the format or lint on the code?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My bad.. I have amended this..Thanks

@moisesPompilio moisesPompilio added the BDK Floresta Issues related to the integration of Floresta in BDK label Mar 10, 2026
@Micah-Shallom Micah-Shallom force-pushed the chore/implement-display-for-error-types branch 2 times, most recently from f336be8 to 06ba469 Compare March 10, 2026 14:24
@moisesPompilio moisesPompilio self-requested a review March 10, 2026 15:03
Copy link
Copy Markdown
Collaborator

@jaoleal jaoleal left a comment

Choose a reason for hiding this comment

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

Heres some superficial review. Still need to check if the imports are correct

FlatChainstoreError::Io(e) => write!(f, "I/O error: {}", e),
FlatChainstoreError::BlockNotFound => write!(f, "Block not found"),
FlatChainstoreError::IndexIsFull => write!(f, "Index is full"),
FlatChainstoreError::DbTooNew(v) => write!(f, "Database too new: {}", v),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
FlatChainstoreError::DbTooNew(v) => write!(f, "Database too new: {}", v),
FlatChainstoreError::DbTooNew(v) => write!(f, "Database too new: {v}"),

impl fmt::Display for FlatChainstoreError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
FlatChainstoreError::Io(e) => write!(f, "I/O error: {}", e),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
FlatChainstoreError::Io(e) => write!(f, "I/O error: {}", e),
FlatChainstoreError::Io(e) => write!(f, "I/O error: {e}"),

FlatChainstoreError::IndexIsFull => write!(f, "Index is full"),
FlatChainstoreError::DbTooNew(v) => write!(f, "Database too new: {}", v),
FlatChainstoreError::Poisoned => write!(f, "Poisoned"),
FlatChainstoreError::InvalidMagic(m) => write!(f, "Invalid magic: {}", m),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
FlatChainstoreError::InvalidMagic(m) => write!(f, "Invalid magic: {}", m),
FlatChainstoreError::InvalidMagic(m) => write!(f, "Invalid magic: {m}"),

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
LeafErrorKind::EmptyStack => write!(f, "Empty stack"),
LeafErrorKind::InvalidInstruction(e) => write!(f, "Invalid instruction: {}", e),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LeafErrorKind::InvalidInstruction(e) => write!(f, "Invalid instruction: {}", e),
LeafErrorKind::InvalidInstruction(e) => write!(f, "Invalid instruction: {e}"),

impl fmt::Display for HeaderExtError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
HeaderExtError::Chain(e) => write!(f, "Chain error: {}", e),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
HeaderExtError::Chain(e) => write!(f, "Chain error: {}", e),
HeaderExtError::Chain(e) => write!(f, "Chain error: {e}"),

Error::InvalidChildNumberFormat => write!(f, "Invalid child number format"),
Error::InvalidDerivationPathFormat => write!(f, "Invalid derivation path format"),
Error::UnknownVersion(v) => write!(f, "Unknown version: {:?}", v),
Error::WrongExtendedKeyLength(l) => write!(f, "Wrong extended key length: {}", l),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Error::WrongExtendedKeyLength(l) => write!(f, "Wrong extended key length: {}", l),
Error::WrongExtendedKeyLength(l) => write!(f, "Wrong extended key length: {I}"),

type Result<T> = floresta_common::prelude::Result<T, MemoryDatabaseError>;

impl Display for MemoryDatabaseError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
fn fmt(&self, f: &mut Formatter<'_>) -> Result {

Comment on lines +44 to +45
impl fmt::Display for BlockchainBuilderError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure if those could be called withtout fmt::. If theres any conflict you can leave as it is, besides that, prefer importing

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

importing would work for Display and FOrmatter but wouldnt for Result due to shadowing of Rusts's default Result with the one we define.....so i just decided to use fmt:: for all of them for uniformity instead of mixing prefixed and unprefixed imports

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Import like this:

use core::fmt;
use core::fmt::Display;
use core::fmt::Formatter

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @luisschwab ..this has been done...thanks

Comment on lines +385 to +387
impl fmt::Display for FlatChainstoreError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the fix has been implemented @jaoleal

Comment on lines +226 to +227
impl fmt::Display for LeafErrorKind {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@jaoleal
Copy link
Copy Markdown
Collaborator

jaoleal commented Mar 10, 2026

See: #876

@Micah-Shallom
Copy link
Copy Markdown
Author

@jaoleal thanks for you reviews.. i looked at #876 you pointed me to... noticed @luisschwab also raised the same issue where there would be a conflict with the non-fmt Result preluded automatically by rust
Screenshot from 2026-03-11 03-28-55

I'll make Display and Formatter unprefixed and keep Result as fmt::Result to avoid the conflict. Let me know if you'd prefer I keep fmt:: on all three for uniformity instead.

@Micah-Shallom Micah-Shallom force-pushed the chore/implement-display-for-error-types branch from 06ba469 to 993dfd5 Compare March 11, 2026 02:45
@csgui csgui added reliability Related to runtime reliability, stability and production readiness and removed enhancement New feature or request code quality Generally improves code readability and maintainability labels Mar 11, 2026
@csgui csgui added this to the Q2/2026 milestone Mar 11, 2026
@csgui csgui removed the BDK Floresta Issues related to the integration of Floresta in BDK label Mar 12, 2026
@Micah-Shallom Micah-Shallom force-pushed the chore/implement-display-for-error-types branch from 993dfd5 to a4c83a6 Compare March 12, 2026 22:31
@Micah-Shallom Micah-Shallom force-pushed the chore/implement-display-for-error-types branch from a4c83a6 to dcab56a Compare March 22, 2026 20:30
@Micah-Shallom
Copy link
Copy Markdown
Author

hi @moisesPompilio , all review feedback has been addressed on this PR.....would appreciate a re-review..thank you.

BlockchainBuilderError::MissingChainstore => write!(f, "Missing chainstore"),
BlockchainBuilderError::MissingChainParams => write!(f, "Missing chain parameters"),
BlockchainBuilderError::IncompleteTip => write!(f, "Incomplete tip"),
BlockchainBuilderError::Database(e) => write!(f, "Database error: {:?}", e),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
BlockchainBuilderError::Database(e) => write!(f, "Database error: {:?}", e),
BlockchainBuilderError::Database(e) => write!(f, "Database error: {e:?}"),

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed

Error::InvalidChildNumber(n) => write!(f, "Invalid child number: {}", n),
Error::InvalidChildNumberFormat => write!(f, "Invalid child number format"),
Error::InvalidDerivationPathFormat => write!(f, "Invalid derivation path format"),
Error::UnknownVersion(v) => write!(f, "Unknown version: {:?}", v),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This still needs to be applied.

@moisesPompilio moisesPompilio requested a review from jaoleal March 23, 2026 22:52
Implement Display trait for error types that were missing it, enabling integration with thiserror and #[error(transparent)].

Error types updated: MemoryDatabaseError, HeaderExtError, FlatChainstoreError, BlockchainBuilderError, LeafErrorKind, slip132::Error.
@Micah-Shallom Micah-Shallom force-pushed the chore/implement-display-for-error-types branch from dcab56a to 1c97642 Compare March 24, 2026 02:44
@luisschwab luisschwab removed their request for review March 26, 2026 15:50
@luisschwab
Copy link
Copy Markdown
Member

@Micah-Shallom please don't request reviews like this. I removed it for a reason.

@luisschwab luisschwab removed their request for review March 26, 2026 15:57
@Micah-Shallom
Copy link
Copy Markdown
Author

Micah-Shallom commented Mar 26, 2026

apologies @luisschwab ....i didnt know you removed it and thought my network was bad and so i kept punching the request review button...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reliability Related to runtime reliability, stability and production readiness

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: implement Display for all error types

5 participants