Skip to content

fix(wallet): no debug on Display implementations#1881

Closed
luisschwab wants to merge 5 commits intobitcoindevkit:masterfrom
luisschwab:fix/no-debug-on-display-impls
Closed

fix(wallet): no debug on Display implementations#1881
luisschwab wants to merge 5 commits intobitcoindevkit:masterfrom
luisschwab:fix/no-debug-on-display-impls

Conversation

@luisschwab
Copy link
Copy Markdown
Member

@luisschwab luisschwab commented Mar 9, 2025

Description

Closes #1933.

Checklists

All Submissions:

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

@luisschwab luisschwab marked this pull request as draft March 9, 2025 23:34
@luisschwab luisschwab marked this pull request as ready for review March 9, 2025 23:51
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Mar 10, 2025
Comment thread crates/wallet/src/wallet/error.rs
@110CodingP
Copy link
Copy Markdown
Contributor

Although this PR is scoped to the wallet, I was wondering if we should also remove the Debug outputs from Display implementations in keychain_txout.rs (line 798), tx_graph.rs (line 228), spk_client.rs (line 24), and file_store/src/lib.rs (line 25)?

@luisschwab
Copy link
Copy Markdown
Member Author

Although this PR is scoped to the wallet, I was wondering if we should also remove the Debug outputs from Display implementations in keychain_txout.rs (line 798), tx_graph.rs (line 228), spk_client.rs (line 24), and file_store/src/lib.rs (line 25)?

Yes, but those should go on different PRs.

@luisschwab luisschwab force-pushed the fix/no-debug-on-display-impls branch from 328e42f to 946d454 Compare March 13, 2025 18:56
@luisschwab luisschwab marked this pull request as draft March 13, 2025 19:00
@luisschwab luisschwab force-pushed the fix/no-debug-on-display-impls branch from 0536428 to 2f6642f Compare March 21, 2025 19:12
@luisschwab luisschwab marked this pull request as ready for review March 21, 2025 19:17
Comment thread crates/wallet/src/types.rs Outdated
Comment thread crates/wallet/src/wallet/error.rs Outdated
Comment thread crates/wallet/src/wallet/mod.rs Outdated
Comment thread crates/wallet/src/wallet/persisted.rs
@luisschwab luisschwab force-pushed the fix/no-debug-on-display-impls branch from 2f6642f to a393147 Compare March 24, 2025 20:07
@luisschwab
Copy link
Copy Markdown
Member Author

@ValuedMammal I think this is RTM

@luisschwab
Copy link
Copy Markdown
Member Author

Closing this and moving to bitcoindevkit/bdk_wallet#12

@luisschwab luisschwab closed this Apr 4, 2025
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Apr 4, 2025
@luisschwab luisschwab removed this from BDK Wallet May 28, 2025
@luisschwab luisschwab deleted the fix/no-debug-on-display-impls branch September 29, 2025 00:21
evanlinjin added a commit that referenced this pull request Feb 7, 2026
823e4e9 fix(file_store)!: remove Debug usage and implement Display for StoreError. (Dmenec)
5a85f6e fix(chain)!: remove Debug usage and implement Display for CalculateFeeError (Dmenec)
d0889f3 docs(core): add detailed SyncRequest output example (Dmenec)
5d715c2 fix(chain)!: remove Debug usage and implement Display for InsertDescriptorError (Dmenec)

Pull request description:

  ### 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:
  * [x] I have signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `just check`, `just fmt` and `just test` before committing

ACKs for top commit:
  evanlinjin:
    ACK 823e4e9

Tree-SHA512: fc3d66b72dc3f7ed3dee3b017687ece73c93883d23cbb019c402e55957368a6487e02b400b092169c326a47a86403f7aa865fe7d7238ccc6eb77afef258ec1ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display implementations should not expose Debug output

3 participants