-
Notifications
You must be signed in to change notification settings - Fork 8
wallet: fix - Improve storage descriptor mismatch error handling #149
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
base: master
Are you sure you want to change the base?
wallet: fix - Improve storage descriptor mismatch error handling #149
Conversation
bennyhodl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consolidate the commits to 1 commit as well
| .load_wallet_async(&mut storage) | ||
| .await | ||
| .map_err(|e| WalletError::WalletPersistanceError(e.to_string()))?; | ||
| .map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be using the errors provided from BDK.
We can extract the LoadError returned from load_wallet_async()
https://docs.rs/bdk_wallet/latest/bdk_wallet/enum.LoadWithPersistError.html
| InvalidDerivationIndex, | ||
| #[error("Invalid secret key")] | ||
| InvalidSecretKey, | ||
| #[error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of multi-line comments. It becomes hard to match against in other environments. Such as typescript bindings.
I would prefer: The persisted descriptor does not match the provided descriptor. and the include expected and stored in the body.
| @@ -0,0 +1,194 @@ | |||
| mod test_util; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to keep the tests folder for full integration tests with a DDK instance.
These tests can be moved to the wallet module as unit tests.
| const MIN_FEERATE: u32 = 253; | ||
|
|
||
| /// Helper function to extract the checksum from a descriptor string. | ||
| fn extract_descriptor_checksum(descriptor: &str) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String matching seems a bit overkill and dangerous to check the error against.
We should be using the provided errors from BDK. If you would like to do extra validation or visibility please use the Descriptor<Pk> struct from the bdk crate.
From that check, we should make sure that we are using a DLC compatible descriptor spending type.
Summary
Fixes Issue #60
Added clear error messages when loading a wallet with the wrong seed. Replaces generic BDK errors with actionable messages showing which keychain failed (external/internal) and comparing expected vs stored descriptor checksums.
What Changed
WalletError::DescriptorMismatcherror with detailed descriptor comparisonExample Error
Key Points