Skip to content

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Jun 25, 2025

Partially resolves bitcoindevkit/bdk_wallet#30.

Description

This PR eliminates all unwrap() and expect() calls from bdk_electrum_client, replacing them with proper error handling. Given that all public methods already return Result, we now propagate error messages instead of panicking.

Changelog notice

  • Removed all unwrap()s and expect()s from bdk_electrum_client.rs.

Checklists

All Submissions:

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

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this! One comment.

@LagginTimes LagginTimes force-pushed the fix_electrum_panics branch 3 times, most recently from 4c3254b to 4277bfe Compare June 26, 2025 20:11
@LagginTimes LagginTimes force-pushed the fix_electrum_panics branch from 4277bfe to e3e4939 Compare July 4, 2025 19:54
@evanlinjin
Copy link
Member

I created a commit on top which cleans up some of the logic so that we return a more explicit error when we cannot find a point of agreement (a.k.a. wrong network): 7c1cc80. Feel free to cherry-pick this across if you find that it is appropriate.

I think it would make sense to include a sync-with-wrong-network test to make sure we hit this error.

@LagginTimes LagginTimes force-pushed the fix_electrum_panics branch from e3e4939 to 0fdbe9d Compare July 11, 2025 19:33
@LagginTimes LagginTimes force-pushed the fix_electrum_panics branch from 612afa2 to b24ae6d Compare July 13, 2025 18:47
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 b24ae6d

@evanlinjin evanlinjin merged commit e5f25a8 into bitcoindevkit:master Jul 18, 2025
19 checks passed
@oleonardolima oleonardolima mentioned this pull request Jul 31, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Never panic based on remote-provided inputs
3 participants