Skip to content

Break builder pattern to expose all setters via bindings #88

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

Merged
merged 5 commits into from
May 26, 2023

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented May 9, 2023

Closes #65

Unfortunately there is no good way of exposing the builder pattern via Uniffi bindings currently. We therefore resort to internal mutability and using setters on the Builder object.

@tnull tnull changed the title 2023 05 expose builder in bindings Break builder pattern to expose all setters via bindings May 9, 2023
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from 96a70ae to e0cc6f8 Compare May 9, 2023 13:35
@tnull tnull added this to the 0.1 milestone May 11, 2023
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from acfcd7c to 0f1e7ed Compare May 12, 2023 10:02
@tnull
Copy link
Collaborator Author

tnull commented May 12, 2023

Rebased on main.

@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch 2 times, most recently from 0dbd449 to 070314f Compare May 18, 2023 08:57
@tnull
Copy link
Collaborator Author

tnull commented May 18, 2023

Rebased on main after #70 has landed. We now also expose the RGS setter methods via bindings.

Moreover, since we already have EntropySourceConfig and GossipSourceConfig, I now moved the Esplora server URL to a corresponding enum ChainDataSourceConfig, which will us to add further variants (e.g., Electrum) in the future without breaking the API.

Additionally, we document the set defaults and switch our default config to Network::Bitcoin while we're here.

@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from 070314f to 3b3a775 Compare May 18, 2023 09:08
@tnull tnull mentioned this pull request May 18, 2023
47 tasks
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from 3b3a775 to a9da44a Compare May 18, 2023 09:27
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from a9da44a to 49baa90 Compare May 23, 2023 17:26
@tnull
Copy link
Collaborator Author

tnull commented May 23, 2023

Rebased after #85 landed.

@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch 3 times, most recently from cdefbe6 to b5db6d0 Compare May 23, 2023 17:53
@tnull tnull mentioned this pull request May 25, 2023
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from b5db6d0 to 7a56598 Compare May 25, 2023 10:15
@tnull
Copy link
Collaborator Author

tnull commented May 25, 2023

Rebased on main after #101 landed.

@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from 7a56598 to bb02755 Compare May 25, 2023 12:07
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from bb02755 to b2afd61 Compare May 26, 2023 08:31
@tnull
Copy link
Collaborator Author

tnull commented May 26, 2023

I now included another commit that moves all UniFFI-specific types and trait impls to a dedicated module, which is now feature-gated behind the uniffi feature. There are three main motivations to do this:

  1. decluttering the codebase, in particular as types.rs started to get unwieldy
  2. UniFFI pulls in additional dependencies which Rust users don't have to put up with if they don't use it anyways and
  3. It may allow us to have a saner MSRV for the Rust part in the future, as UniFFI currently requires something like 1.62.0.

However, I'll look into actually lowering the MSRV separately, as it may require downgrading UniFFI, and IIRC there was good reason why I had upgraded to 0.23.0 in the first place...

tnull added 2 commits May 26, 2023 12:42
Unfortunately there is no good way of exposing the builder pattern via
Uniffi bindings currenlty. We therefore resort to internal mutability
and using setters on the `Builder` object.
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from b2afd61 to f6c4117 Compare May 26, 2023 10:43
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Looks good minus a couple easy comments.

@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from f6c4117 to 1b4b47a Compare May 26, 2023 20:38
tnull added 3 commits May 26, 2023 22:42
After we already have `EntropySourceConfig` and `GossipSourceConfig`,
we here move the esplora server URL to a corresponding `enum
ChainDataSourceConfig`, which will us to add further variants (e.g.,
`Electrum`) in the future without breaking the API.

Additionally, we document the set default values.
As we're getting ready for release
While we need quite a bit of dependencies and custom impls for UniFFI support, there is
no reason to pull them in for Rust-only builds. Here we move the
UniFFI-specific types and trait impls to a dedicated module and hide it
behind a new `uniffi` feature.
@tnull tnull force-pushed the 2023-05-expose-builder-in-bindings branch from 1b4b47a to de94a54 Compare May 26, 2023 20:42
@tnull
Copy link
Collaborator Author

tnull commented May 26, 2023

Updated the commits to address feedback and included:

diff --git a/src/lib.rs b/src/lib.rs
index b9b75ca..3a0526f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -375,22 +375,18 @@ impl Builder {

                // Initialize the on-chain wallet and chain access
-               let seed_bytes = if let Some(entropy_source_config) =
-                       &*self.entropy_source_config.read().unwrap()
-               {
-                       // Use the configured entropy source, if the user set one.
-                       match entropy_source_config {
-                               EntropySourceConfig::SeedBytes(bytes) => bytes.clone(),
-                               EntropySourceConfig::SeedFile(seed_path) => {
-                                       io::utils::read_or_generate_seed_file(seed_path)
-                               }
-                               EntropySourceConfig::Bip39Mnemonic { mnemonic, passphrase } => match passphrase {
-                                       Some(passphrase) => mnemonic.to_seed(passphrase),
-                                       None => mnemonic.to_seed(""),
-                               },
+               let seed_bytes = match &*self.entropy_source_config.read().unwrap() {
+                       Some(EntropySourceConfig::SeedBytes(bytes)) => bytes.clone(),
+                       Some(EntropySourceConfig::SeedFile(seed_path)) => {
+                               io::utils::read_or_generate_seed_file(seed_path)
+                       }
+                       Some(EntropySourceConfig::Bip39Mnemonic { mnemonic, passphrase }) => match passphrase {
+                               Some(passphrase) => mnemonic.to_seed(passphrase),
+                               None => mnemonic.to_seed(""),
+                       },
+                       None => {
+                               // Default to read or generate from the default location generate a seed file.
+                               let seed_path = format!("{}/keys_seed", config.storage_dir_path);
+                               io::utils::read_or_generate_seed_file(&seed_path)
                        }
-               } else {
-                       // Default to read or generate from the default location generate a seed file.
-                       let seed_path = format!("{}/keys_seed", config.storage_dir_path);
-                       io::utils::read_or_generate_seed_file(&seed_path)
                };

@@ -417,27 +413,22 @@ impl Builder {
                .expect("Failed to set up on-chain wallet");

-               let (blockchain, tx_sync) = if let Some(chain_data_source_config) =
-                       &*self.chain_data_source_config.read().unwrap()
-               {
-                       match chain_data_source_config {
-                               ChainDataSourceConfig::Esplora(server_url) => {
-                                       let tx_sync =
-                                               Arc::new(EsploraSyncClient::new(server_url.clone(), Arc::clone(&logger)));
-                                       let blockchain = EsploraBlockchain::from_client(
-                                               tx_sync.client().clone(),
-                                               BDK_CLIENT_STOP_GAP,
-                                       )
-                                       .with_concurrency(BDK_CLIENT_CONCURRENCY);
-                                       (blockchain, tx_sync)
-                               }
+               let (blockchain, tx_sync) = match &*self.chain_data_source_config.read().unwrap() {
+                       Some(ChainDataSourceConfig::Esplora(server_url)) => {
+                               let tx_sync =
+                                       Arc::new(EsploraSyncClient::new(server_url.clone(), Arc::clone(&logger)));
+                               let blockchain =
+                                       EsploraBlockchain::from_client(tx_sync.client().clone(), BDK_CLIENT_STOP_GAP)
+                                               .with_concurrency(BDK_CLIENT_CONCURRENCY);
+                               (blockchain, tx_sync)
+                       }
+                       None => {
+                               // Default to Esplora client.
+                               let server_url = DEFAULT_ESPLORA_SERVER_URL.to_string();
+                               let tx_sync = Arc::new(EsploraSyncClient::new(server_url, Arc::clone(&logger)));
+                               let blockchain =
+                                       EsploraBlockchain::from_client(tx_sync.client().clone(), BDK_CLIENT_STOP_GAP)
+                                               .with_concurrency(BDK_CLIENT_CONCURRENCY);
+                               (blockchain, tx_sync)
                        }
-               } else {
-                       // Default to Esplora client with blockstream endpoint.
-                       let server_url = DEFAULT_ESPLORA_SERVER_URL.to_string();
-                       let tx_sync = Arc::new(EsploraSyncClient::new(server_url, Arc::clone(&logger)));
-                       let blockchain =
-                               EsploraBlockchain::from_client(tx_sync.client().clone(), BDK_CLIENT_STOP_GAP)
-                                       .with_concurrency(BDK_CLIENT_CONCURRENCY);
-                       (blockchain, tx_sync)
                };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use enums for Network types instead of strings
2 participants