-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add NetworkMismatch
error type
#485
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for looking into this!
src/builder.rs
Outdated
@@ -189,6 +191,7 @@ impl fmt::Display for BuildError { | |||
Self::WalletSetupFailed => write!(f, "Failed to setup onchain wallet."), | |||
Self::LoggerSetupFailed => write!(f, "Failed to setup the logger."), | |||
Self::InvalidNodeAlias => write!(f, "Given node alias is invalid."), | |||
Self::NetworkMismatch => write!(f, "Given network does match."), |
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.
nit: Could you specify what it doesn't match?
BuildError::WalletSetupFailed | ||
.map_err(|e| match e { | ||
bdk_wallet::LoadWithPersistError::InvalidChangeSet( | ||
bdk_wallet::LoadError::Mismatch(bdk_wallet::LoadMismatch::Network { .. }), |
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.
Seems we might also want to match on the Genesis
variant?
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.
hmmm, since the genesis hash is not a configuration that can be directly set by the user I think the check on the network mismatch will already catch it? Because to do the check on the genesis hash we would first need to get it with something like: bitcoin::blockdata::constants::genesis_block(config.network).block_hash();
which depends on the network provided by the user. So it doesn't seem possible to pass the network check but fail the Genesis
one. That's how I saw it but lmk if this is wrong and I'll change it.
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 mean, technically yes, but are we sure BDK won't ever emit a Genesis
variant if we set the wrong Network
?
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.
if we set the wrong network, yes I guess any of these two errors (Network or Genesis mismatch) could be thrown.
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.
Right, that's why I'd prefer to match on both and return the mismatch error if either variant is returned.
8991768
to
206a092
Compare
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.
Changes look good to me overall.
src/builder.rs
Outdated
@@ -189,6 +191,7 @@ impl fmt::Display for BuildError { | |||
Self::WalletSetupFailed => write!(f, "Failed to setup onchain wallet."), | |||
Self::LoggerSetupFailed => write!(f, "Failed to setup the logger."), | |||
Self::InvalidNodeAlias => write!(f, "Given node alias is invalid."), | |||
Self::NetworkMismatch => write!(f, "Given network does not match the node's network."), |
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.
nit : Given network does not match the node's previously configured network.
Return specific error if the node is restarted with a different network from the one it was first created with. This will be caught when trying to load the `BDK` wallet.
206a092
to
b1b2caf
Compare
for #117
Added a
NetworkMismatch
error type if the node is restarted with a different network. This is caught when trying to load bdk wallet.