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

Add binding docstrings #512

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add binding docstrings #512

wants to merge 5 commits into from

Conversation

dangeross
Copy link
Collaborator

This PR is to support including docstrings defined in the Uniffi UDL file to be included in bindings that are generated from it.

  • Bump the React Native codegen to use Uniffi 0.25.0. In doing so we switch to use the NordSecurity uniffi 0.25.0 which supports docstrings (From 0.26.0 we can switch back to mozilla) 73765ba
  • Add docstring handling to the React Native codegen ff5ea42
  • Add comments to the UDL file for structs and methods c758753

There is a lot of changes to the boilerplate code on upgrading the React Native codegen to 0.25.0, we also have to bring in changes to the Swift and Kotlin codegen. You can find the meat & potatoes of the changes here:

Adding these changes also enables docstings to be used in C#, Go, Kotlin, Python & Swift generated bindings.

For example
Adding to the UDL file:

    /// Pays to a Bitcoin address via a chain swap.
    ///
    /// Depending on [Config]'s `payment_timeout_sec`, this function will return:
    /// * [PaymentState::Pending] payment - if the payment could be initiated but didn't yet
    ///   complete in this time
    /// * [PaymentState::Complete] payment - if the payment was successfully completed in this time
    ///
    /// # Arguments
    ///
    /// * `req` - the [PayOnchainRequest] containing:
    ///     * `address` - the Bitcoin address to pay to
    ///     * `prepare_response` - the [PreparePayOnchainResponse] from calling [BindingLiquidSdk::prepare_pay_onchain]
    ///
    /// # Errors
    ///
    /// * [PaymentError::PaymentTimeout] - if the payment could not be initiated in this time
    [Throws=PaymentError]
    SendPaymentResponse pay_onchain(PayOnchainRequest req);

Generates jsdocs:

/**
 * Pays to a Bitcoin address via a chain swap.
 *
 * Depending on {@link Config}'s `paymentTimeoutSec`, this function will return:
 * * {@link PaymentState.PENDING} payment - if the payment could be initiated but didn't yet
 *   complete in this time
 * * {@link PaymentState.COMPLETE} payment - if the payment was successfully completed in this time
 *
 * # Arguments
 *
 * * `req` - the {@link PayOnchainRequest} containing:
 *     * `address` - the Bitcoin address to pay to
 *     * `prepareResponse` - the {@link PreparePayOnchainResponse} from calling {@link preparePayOnchain}
 *
 * # Errors
 *
 * * {@link PaymentError.PAYMENT_TIMEOUT} - if the payment could not be initiated in this time
 */
export const payOnchain = async (req: PayOnchainRequest): Promise<SendPaymentResponse> => {
    const response = await BreezSDKLiquid.payOnchain(req)
    return response
}

@dangeross dangeross force-pushed the savage-rn-codegen-0.25 branch from 651e063 to b0883a9 Compare October 1, 2024 13:55
@@ -17,7 +17,8 @@ breez-sdk-liquid = { path = "../core" }
log = { workspace = true }
uniffi = { workspace = true, features = [ "bindgen-tests", "cli" ] }
# Bindgen used by KMP, version has to match the one supported by KMP
uniffi_bindgen = "0.25.2"
uniffi_bindgen = { workspace = true }
uniffi_bindgen_kmp = { version = "0.25.2", package = "uniffi_bindgen" }
Copy link
Contributor

Choose a reason for hiding this comment

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

With the new uniffi_bindgen_kmp, is the uniffi_bindgen_kotlin_multiplatform below still necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to split the use of uniffi_bindgen because of compatibility issues, so uniffi_bindgen_kmp is uniffi_bindgen 0.25.2 for use with uniffi_bindgen_kotlin_multiplatform.

I will play around a bit more to see if I can get them to use the same uniffi_bindgen.

Comment on lines -2 to -4
// BEGIN sdk-common mirror imports
// These are structs defined in sdk-common, which we want to make available in this project's UDL bindings

Copy link
Contributor

Choose a reason for hiding this comment

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

Could these lines be preserved? They would make life easier in the future when dealing with mirror structs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These comments where breaking uniffi when followed by a docstring e.g.

// BEGIN sdk-common mirror imports
// These are structs defined in sdk-common, which we want to make available in this project's UDL bindings

/// Wrapper for a BOLT11 LN invoice
dictionary LNInvoice {

I'm not sure why, I can remove the LNInvoice docstring if its important.

Is this an improvement if I comment the end of the block? 6caddd9

Copy link
Contributor

Choose a reason for hiding this comment

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

These comments where breaking uniffi when followed by a docstring e.g.

Would it work with a comment block like in 6caddd9 ?

Is this an improvement if I comment the end of the block? 6caddd9

Yes, thanks.

@@ -242,13 +388,21 @@ dictionary CurrencyInfo {
sequence<LocaleOverrides> locale_overrides;
};

// END sdk-common mirror imports
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

For example:

// END sdk-common mirror imports
// --------------------------------------------------------
// BEGIN sdk-common wrappers
// ...

@ok300
Copy link
Contributor

ok300 commented Oct 1, 2024

Related to breez/breez-sdk-greenlight#781

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