Skip to content

fix: no Debug on Display implementations #12

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

luisschwab
Copy link
Member

@luisschwab luisschwab commented Apr 4, 2025

Description

This PR removes the use of debugs on Display implemetations.

Changelog notice

  • Implement Display for KeychainKind.
  • Implement Display for LoadMismatch.
  • Add error message for CreateWithPersistError.
  • The CreateTxError::LockTime arm had requested and required inverted, now fixed.
  • Remove debugs.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@coveralls
Copy link

coveralls commented Apr 4, 2025

Pull Request Test Coverage Report for Build 14539669521

Details

  • 0 of 69 (0.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 85.942%

Changes Missing Coverage Covered Lines Changed/Added Lines %
wallet/src/wallet/error.rs 0 3 0.0%
wallet/src/types.rs 0 5 0.0%
wallet/src/wallet/persisted.rs 0 29 0.0%
wallet/src/wallet/mod.rs 0 32 0.0%
Totals Coverage Status
Change from base Build 14270666930: -0.6%
Covered Lines: 7269
Relevant Lines: 8458

💛 - Coveralls

@110CodingP
Copy link

nit: should we consider changing fmt::Display::fmt in the Display impls of LoadWithPersistError and FileStoreError to use write! for consistency in style?

@110CodingP
Copy link

ACK 6e521c9 (compiled the code and ran the tests)

@ValuedMammal ValuedMammal moved this to Todo in BDK Wallet Apr 8, 2025
@ValuedMammal ValuedMammal added this to the 1.3.0 milestone Apr 8, 2025
@ValuedMammal ValuedMammal added the bug Something isn't working label Apr 8, 2025
@ValuedMammal ValuedMammal moved this from Todo to Needs Review in BDK Wallet Apr 8, 2025
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK 6e521c9

@luisschwab
Copy link
Member Author

nit: should we consider changing fmt::Display::fmt in the Display impls of LoadWithPersistError and FileStoreError to use write! for consistency in style?

No, because we're using the Display implementation of the underlying types directly.

@luisschwab luisschwab force-pushed the fix/no-debug-on-display-impls branch from 6e521c9 to 0ae48b1 Compare April 16, 2025 19:23
@luisschwab luisschwab force-pushed the fix/no-debug-on-display-impls branch from 0ae48b1 to 0d8d0ec Compare April 18, 2025 18:14
@luisschwab
Copy link
Member Author

Rebased with suggestions from @ValuedMammal on changeset_info() implementation.

Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 0d8d0ec

@ValuedMammal ValuedMammal requested review from evanlinjin and removed request for oleonardolima April 22, 2025 20:05
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 0d8d0ec

Just a small nit.

Comment on lines +390 to +414
/// Helper function to display basic information about a [`ChangeSet`].
fn changeset_info(changeset: &ChangeSet) -> String {
let network = match &changeset.network {
Some(network) => network.to_string(),
None => "None".to_string(),
};

let mut checksum = String::new();
if let Some(desc) = &changeset.descriptor {
if let Ok(ck) = calc_checksum(&desc.to_string()) {
checksum += ck.as_str();
}
}
if let Some(desc) = &changeset.change_descriptor {
if let Ok(ck) = calc_checksum(&desc.to_string()) {
checksum += ck.as_str();
}
}
// Don't return an empty checksum
if checksum.is_empty() {
checksum = "None".to_string();
}

format!("Network: {}, Descriptor Checksum: {}", network, checksum)
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: change signature of this to take in f: &mut fmt::Formatter instead.

Also:

  • Separate out the descriptor checksums. `descriptor_checksum={}, change_descriptor_checksum={}.
  • All more info: tx_count, anchor_count, block_count, etc.

Copy link
Member Author

@luisschwab luisschwab May 8, 2025

Choose a reason for hiding this comment

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

What do you think @evanlinjin? cc @ValuedMammal

/// Helper function to display basic information about a [`ChangeSet`].
fn changeset_info(f: &mut fmt::Formatter<'_>, changeset: &ChangeSet) -> fmt::Result {
    let network = changeset.network
        .as_ref()
        .map_or("None".to_string(), |n| n.to_string());

    let descriptor_checksum = changeset.descriptor
        .as_ref()
        .and_then(|d| calc_checksum(&d.to_string()).ok())
        .unwrap_or_else(|| "None".to_string());

    let change_descriptor_checksum = changeset.change_descriptor
        .as_ref()
        .and_then(|d| calc_checksum(&d.to_string()).ok())
        .unwrap_or_else(|| "None".to_string());

    let tx_count = changeset.tx_graph.txs.len();

    let anchor_count = changeset.tx_graph.anchors.len();

    let block_count = if let Some(&count) = changeset.local_chain.blocks.keys().last() {
        count
    } else {
        0
    };

    writeln!(f, "")?;
    writeln!(f, "Network: {}", network)?;
    writeln!(f, "Descriptor Checksum: {}", descriptor_checksum)?;
    writeln!(f, "Change Descriptor Checksum: {}", change_descriptor_checksum)?;
    writeln!(f, "Transaction Count: {}", tx_count)?;
    writeln!(f, "Anchor Count: {}", anchor_count)?;
    writeln!(f, "Block Count: {}", block_count)?;
    Ok(())
}

Then it gets called by CreateWithPersistError like this:

impl<E: fmt::Display> fmt::Display for CreateWithPersistError<E> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            Self::Persist(err) => write!(f, "{}", err),
            Self::DataAlreadyExists(changeset) => {
                write!(f, "Cannot create wallet in a persister which already contains data: ")?;
                changeset_info(f, changeset)
            }
            Self::Descriptor(err) => {
                write!(f, "{err}")
            }
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

6 participants