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

lib: add Amount, Network #1

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

rustaceanrob
Copy link
Contributor

Here I added Amount and Network. To convert to and from the UniFFI and internal types I added some macros, which may be moved into a marcos.rs or a more general utils.rs. I also added thiserror as we seemed to roughly agree it is appropriate as a dependency on the dev call.

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

I have a few questions/comments but this looks really good!

src/bitcoin.udl Show resolved Hide resolved
src/lib.rs Outdated
pub use bitcoin::amount::ParseAmountError as BitcoinParseAmountError;
pub use bitcoin::Network;

macro_rules! impl_from_core_type {
Copy link
Member

Choose a reason for hiding this comment

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

Great stuff! Took me a minute to parse it but I think this is an example of a macro that's simple and worth having. It also ensures we reduce the number of places where these simple workflows are written. I'm fine with those as is, but how do you feel about renaming the field like so:

macro_rules! impl_from_core_type {
    ($core_type:ident, $ffi_type:ident) => {
        impl From<$core_type> for $ffi_type {
            fn from(core_type: $core_type) -> Self {
                $ffi_type(core_type)
            }
        }
    };
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me

src/lib.rs Outdated
};
}

macro_rules! impl_from_outer_type {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above basically.

Copy link
Member

@thunderbiscuit thunderbiscuit Aug 15, 2024

Choose a reason for hiding this comment

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

I also think it's more intuitive to:

  1. Reverse the order of the arguments
  2. Name the macro impl_from_ffi_type()

So that the call site looks like this:

impl_from_ffi_type!(Amount, BitcoinAmount);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think these make sense to move to a "macros.rs" or "utils.rs"

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would have them in a macro.rs file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't do macro singular cause that's a keyword. Went with macros, going to force push in a sec

@thunderbiscuit
Copy link
Member

By the way I tested this locally with bitcoindevkit/bdk-ffi#546 and it works well. The import statements have to be adjusted for tests, and that's about it. Great stuff.

@rustaceanrob
Copy link
Contributor Author

Pushed those updates up

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK eb01b76

@thunderbiscuit thunderbiscuit merged commit eb01b76 into bitcoindevkit:master Aug 16, 2024
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