Skip to content
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

Upgrade Rust bdk to alpha 3 #441

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

thunderbiscuit
Copy link
Member

This PR updates the version of bdk and associated libraries to the alpha 3 version.

The majority of the changes come from the introduction of new error enums. At the moment they are all erased into this one Alpha3Error::Generic error type. Further commits will pull the required errors one at a time and implement them until the generic variant is not required anymore.

@thunderbiscuit thunderbiscuit force-pushed the alpha3 branch 2 times, most recently from 115f383 to a6af56f Compare January 10, 2024 19:17
@thunderbiscuit thunderbiscuit requested a review from reez January 10, 2024 20:07
@thunderbiscuit thunderbiscuit self-assigned this Jan 10, 2024
Copy link
Collaborator

@reez reez left a comment

Choose a reason for hiding this comment

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

This looks great other than 1 requested change-

I'd like to not include the dependency thiserror since I think we can do without it, and then use something like this in error.rs instead:

#[derive(Debug)]
pub enum Alpha3Error {
    Generic,
}

impl fmt::Display for Alpha3Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            Alpha3Error::Generic => write!(f, "Error in FFI"),
        }
    }
}

impl std::error::Error for Alpha3Error {}

That matches for Alpha3Error what I've got for CalculateFeeError (which matching my code isn't really the important thing, but not including the dependency thiserror is more what I'm going for). With those changes thiserror can be removed and things compile on my end and cargo test --lib still passes locally. But let me know if you have any thoughts related to that or if I'm missing anything.

Other than that this looks great!

@thunderbiscuit
Copy link
Member Author

Oh yeah great idea!

Copy link
Collaborator

@reez reez left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

ACK 54beb23

@thunderbiscuit thunderbiscuit merged commit 54beb23 into bitcoindevkit:master Jan 11, 2024
17 checks passed
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.

2 participants