Use walleterror for error handling in try_get_internal_address #460
Conversation
reez
changed the title
reez
force-pushed
the
reez/err-address
branch
2 times, most recently
from
e503d3b to
c644409
Compare
|
Warning Rate Limit Exceeded@reez has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 38 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates aim to enhance error handling within the Bitcoin Dev Kit (BDK) FFI interface. Changes include renaming Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- bdk-ffi/src/bdk.udl (2 hunks)
- bdk-ffi/src/error.rs (5 hunks)
- bdk-ffi/src/lib.rs (1 hunks)
- bdk-ffi/src/wallet.rs (3 hunks)
Additional comments: 7
bdk-ffi/src/lib.rs (1)
- 35-35: The change from
WalletCreationErrortoWalletErroraligns with the PR's objectives to enhance error handling by making it more generic. Ensure that all references to the oldWalletCreationErrorhave been updated toWalletErroracross the entire codebase.Verification successful
The script's execution did not find any references to
WalletCreationError, indicating that the replacement withWalletErrorhas been successfully carried out across the Rust files in the codebase. This aligns with the PR's objectives to enhance error handling by making it more generic. However, it's important to ensure thatWalletErroris correctly implemented and used whereverWalletCreationErrorwas previously referenced.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to WalletCreationError. Expecting no results. rg --type rust 'WalletCreationError'Length of output: 36
bdk-ffi/src/bdk.udl (2)
- 19-19: Renaming
WalletCreationErrortoWalletErrorin the UDL file is consistent with the PR's objectives. This change enhances the clarity and specificity of error handling.- 101-101: Updating the
Throwsannotation fromWalletCreationErrortoWalletErrorin theWalletinterface aligns with the renaming and improves error specificity. Ensure all references and annotations have been correctly updated throughout the UDL file.Also applies to: 106-106
Verification successful
The absence of output from the
rgcommand searching forWalletCreationErrorsuggests that there are no remaining references toWalletCreationErrorin the UDL file or anywhere else in the codebase. This aligns with the intended update of changing references fromWalletCreationErrortoWalletErroras mentioned in the review comment. Therefore, it can be concluded that the update has been correctly implemented throughout the UDL file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that there are no remaining references to WalletCreationError in the UDL file. rg --type udl 'WalletCreationError'Length of output: 63
Script:
#!/bin/bash # Verify that there are no remaining references to WalletCreationError in the UDL file. rg 'WalletCreationError'Length of output: 24
bdk-ffi/src/error.rs (1)
- 32-32: Renaming
WalletCreationErrortoWalletErrorand updating its variants and implementations in theerror.rsfile aligns with the PR's objectives to enhance error handling. This change makes error handling more specific and meaningful.bdk-ffi/src/wallet.rs (3)
- 35-35: The
Wallet::newfunction now returns aResult<Self, WalletError>, aligning with the PR's objective to make error handling more specific to wallet operations. This change is appropriate and enhances the clarity of error handling in wallet creation.- 70-74: The
try_get_internal_addressmethod has been updated to returnResult<AddressInfo, WalletError>instead of usingAlpha3Error, and the error mapping now usesWalletError::Write. This change makes the error more specific and meaningful, aligning with the PR's objectives. However, ensure thatWalletError::Writeaccurately represents all possible error conditions that could occur in this context.- 70-74: The change from
Alpha3ErrortoWalletErrorin thetry_get_internal_addressmethod is a significant improvement in making the error handling more specific and meaningful. However, it's crucial to ensure that theWalletError::Writevariant is the most appropriate choice for all possible errors that could occur in this method. Consider if there are otherWalletErrorvariants that might more accurately describe certain error conditions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- bdk-ffi/src/bdk.udl (2 hunks)
- bdk-ffi/src/error.rs (5 hunks)
- bdk-ffi/src/lib.rs (1 hunks)
- bdk-ffi/src/wallet.rs (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- bdk-ffi/src/bdk.udl
- bdk-ffi/src/error.rs
- bdk-ffi/src/lib.rs
- bdk-ffi/src/wallet.rs
|
Those pesky errors. So now this remaps a write error from a PersistBackend::Write error to a NewOrLoadError::Write. I'm worried about the confusion that we're about to introduce as Rust errors are attempted to be mapped to the ffi layer, but can't be fully mapped either (see my PR on persistence) because some of the methods change or combine multiple Rust calls and must therefore combine the potentially returned errors. Sorry I'm just venting here. But errors are a big mess right now, and we haven't even begun to seriously get into them. |
I totally understand what youre saying, and those concerns about the complexities here error mapping from Rust to the FFI layer and also the potential for confusion this might introduce. Error handling across FFI boundaries definitely has some challenges, especially as we try to maintain clarity and meaningfulness in the errors presented at the FFI layer while trying to encapsulate the richness+specificity of Rust's error handling. Hopefully this PR is a way for us to inch forward towards how all of this is going to work out since that's the general direction we are heading towards, that's why I tried to keep it as small+tight as possible without doing much refactoring and kind of naming+leaving these things in plain sight that "stick out like a sore thumb" at the moment. |
thunderbiscuit
force-pushed
the
reez/err-address
branch
3 times, most recently
from
bac73b7 to
f2d4545
Compare
|
I'm playing with the @reez @notmandatory let me know what you think. If there is interest, I might actually go and attempt a refactor of our current set of errors using it as a separate PR. Here is the gist of what it comes down to: Before#[derive(Debug)]
pub enum WalletError {
// Errors coming from the FileError enum
Io {
e: String,
},
InvalidMagicBytes {
got: Vec<u8>,
expected: Vec<u8>,
},
// Errors coming from the NewOrLoadError enum
Descriptor,
Write,
Load,
NotInitialized,
LoadedGenesisDoesNotMatch,
LoadedNetworkDoesNotMatch {
expected: Network,
got: Option<Network>,
},
}
impl fmt::Display for WalletError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Io { e } => write!(f, "io error trying to read file: {}", e),
Self::InvalidMagicBytes { got, expected } => write!(
f,
"file has invalid magic bytes: expected={:?} got={:?}",
expected, got,
),
Self::Descriptor => write!(f, "error with descriptor"),
Self::Write => write!(f, "failed to write to persistence"),
Self::Load => write!(f, "failed to load from persistence"),
Self::NotInitialized => {
write!(f, "wallet is not initialized, persistence backend is empty")
}
Self::LoadedGenesisDoesNotMatch => {
write!(f, "loaded genesis hash does not match the expected one")
}
Self::LoadedNetworkDoesNotMatch { expected, got } => {
write!(f, "loaded network type is not {}, got {:?}", expected, got)
}
}
}
}
impl std::error::Error for WalletError {}thiserror#[derive(Debug, thiserror::Error)]
pub enum WalletError {
#[error("IO error, this may mean that the file is too short")]
Io,
#[error("file has invalid magic bytes: expected={expected:?} got={got:?}")]
InvalidMagicBytes { got: Vec<u8>, expected: Vec<u8> },
#[error("error with descriptor")]
Descriptor,
#[error("failed to write to persistence")]
Write,
#[error("failed to load from persistence")]
Load,
#[error("wallet is not initialized, persistence backend is empty")]
NotInitialized,
#[error("loaded genesis hash does not match the expected one")]
LoadedGenesisDoesNotMatch,
#[error("loaded network type is not {expected}, got {got:?}")]
LoadedNetworkDoesNotMatch { expected: Network, got: Option<Network> },
}The From traits still need to be implemented for the Rust errors however, so that part doesn't really get benefits from using thiserror. |
|
Good work @thunderbiscuit I think we should try ACK f2d4545 |
reez
force-pushed
the
reez/err-address
branch
2 times, most recently
from
f597fa4 to
a265112
Compare
|
@thunderbiscuit I rebased this on
Wanted to get your feedback on what your thoughts on how this should shape up before it is mergeable? I didn't rename |
|
After playing with it a bit, it felt to me like those were fairly distinct errors. I feel like this is initially hidden by the fact that they both use the persistence's
Which makes me want to reduce it to the following two distinct errors:
Open to discussing more of course. |
reez
force-pushed
the
reez/err-address
branch
3 times, most recently
from
1a07c41 to
47d0b10
Compare
Pushed some updates to this per your comments and our convo, I think its ready to go, but let me know if any other thoughts/updates, happy to change anything else if needed- |
I've just been looking thru what functions we currently use (or I use in a sample iOS project) that have Alpha3Error so I can turn those into more specific errors.
Description
Adds
PersistenceErrorand replaces the use ofAlpha3ErrorwithPersistenceErrorin the try_get_internal_address function.I added a unit test for
PersistenceErroras well.Notes to the reviewers
Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: