Skip to content

Conversation

@Dmenec
Copy link

@Dmenec Dmenec commented Dec 11, 2025

Description

Fixes #1933, remove the usage of Debug on Display implemetations mentioned in #1881 (comment)

Changelog notice

  • Replaced Debug usage with Display messages for InsertDescriptorError, CalculateFeeError and StoreError.
  • InvalidMagicBytes now displays expected and actual bytes in hexadecimal instead of using Debug.
  • Added a new short_descriptor function to shorten descriptors for concise error output.
  • Added detailed SyncRequest Display for Esplora and Electrum examples.

Checklists

All Submissions:

  • I have signed all my commits
  • I followed the contribution guidelines
  • I ran just check, just fmt and just test before committing

Replace Debug-based formatting with user-friendly Display messages.

Add helper to shorten descriptors for concise error output.
Limited to 3 max shown items and added a suffix when there are additional entries.
@ValuedMammal ValuedMammal moved this to In Progress in BDK Chain Dec 12, 2025
@ValuedMammal ValuedMammal added this to the Chain 0.24.0 milestone Dec 12, 2025
@Dmenec Dmenec force-pushed the fix/no-debug-on-display-impls branch from 492114b to b73016e Compare December 14, 2025 14:57
@qatkk
Copy link

qatkk commented Dec 21, 2025

Concept ACK — this approach looks good, thanks for the changes. I ran the tests locally and they all passed.

While trying to recreate the initial issue’s scenario (#1933), I found that bdk_wallet master still references the older API names (list_canonical_txsfilter_chain_unspentsfilter_chain_txoutsfrom_genesis_hash), whereas this PR’s bdk_chain exposes the newer canonicalization API (canonical_iter) and LocalChain::from_genesis. As a result, building bdk_wallet against this PR failed. Is there any branch in bdk_wallet that already adopts the new APIs, so I can test against them?

I have a few small suggestions below. Most of these are nits, but since this PR is already addressing the area, I thought it’d be reasonable to mention them here:

  • It might be nice to add a CI to prevent using Debug for Display in future changes.

  • In the short_descriptor function, it could be useful to show the last 8 characters of the descriptor so the full checksum is visible.

  • A small note on InvalidMagicBytes: it might be helpful to also show any difference between the length of expected and received byte.

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

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Display implementations should not expose Debug output

3 participants