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

Refac workspace to follow rust guidelines. #1448

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

edouardparis
Copy link
Member

@edouardparis edouardparis commented Nov 7, 2024

This PR clarifies the rust workspace with its members:

  • liana: The lianad and liana-cli crate
  • liana-gui: The gui crate to build the graphical interface
  • liana-ui: A crate to quickly iterate on a design system

It allows to have coordination across crates easier, consistent build and shared dependency versions.
As a result the build processes in the guix container and the macos container are way faster as the dependencies are built only once.

The overall MSRV is changed for rust 1.71.1, which is the liana-gui MSRV.
Only one rust version is used for all the crates.

This PR is also a change of vision of the project. The liana daemon is not used as it is. It is too much cumbersome. It should be at the service of the future applications and for now at the service of the GUI.
The security aspects of the project should be aware of what the end user is using and in reality it is the liana-gui binary.

@edouardparis edouardparis force-pushed the refac-workspace branch 14 times, most recently from 00cc580 to d84332a Compare November 14, 2024 12:36
@edouardparis edouardparis marked this pull request as ready for review November 14, 2024 14:59
@edouardparis edouardparis requested review from pythcoiner, darosior and jp1ac4 and removed request for darosior November 14, 2024 15:01
Copy link
Collaborator

@jp1ac4 jp1ac4 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!

Do you think the Python tests should be in liana/?

doc/BUILD.md Outdated Show resolved Hide resolved
doc/BUILD.md Outdated Show resolved Hide resolved
doc/BUILD.md Outdated Show resolved Hide resolved
@pythcoiner
Copy link
Collaborator

pythcoiner commented Nov 15, 2024

reviewed bc774e9 commit with this command to sort out move-only changes:
git diff --diff-filter=M b46efc6 bc774e95

diff --git a/Cargo.toml b/Cargo.toml
index c348d97..fdb819a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,75 +1,7 @@
-[package]
-name = "liana"
-version = "8.0.0"
-authors = ["Antoine Poinsot <[email protected]>"]
-edition = "2018"
-repository = "https://github.com/wizardsardine/liana"
-license-file = "LICENCE"
-keywords = ["bitcoin", "wallet", "miniscript", "inheritance", "recovery"]
-description = "Liana wallet daemon"
-exclude = [".github/", ".cirrus.yml", "tests/",  "test_data/", "contrib/", "pyproject.toml"]
-
-[[bin]]
-name = "lianad"
-path = "src/bin/daemon.rs"
-required-features = ["daemon"]
-
-[[bin]]
-name = "liana-cli"
-path = "src/bin/cli.rs"
-required-features = ["daemon"]
-
-[features]
-default = ["daemon"]
-daemon = ["libc"]
-nonblocking_shutdown = []
-
-[dependencies]
-# For managing transactions (it re-exports the bitcoin crate)
-miniscript = { version = "11.0", features = ["serde", "compiler", "base64"] }
-
-# Coin selection algorithms for spend transaction creation.
-bdk_coin_select = "0.3"
-
-# For Electrum backend. This is the latest version with the same bitcoin version as
-# the miniscript dependency.
-bdk_electrum = { version = "0.14" }
-
-# Don't reinvent the wheel
-dirs = "5.0"
-
-# We use TOML for the config, and JSON for RPC
-serde = { version = "1.0", features = ["derive"] }
-toml = "0.5"
-serde_json = { version = "1.0", features = ["raw_value"] }
-
-# Logging stuff
-log = "0.4"
-fern = "0.6"
-
-# In order to have a backtrace on panic, because the
-# stdlib does not have a programmatic interface yet
-# to work with our custom panic hook.
-backtrace = "0.3"
-
-# Pinned to this version because they keep breaking their MSRV in point releases...
-# FIXME: this is unfortunate, we don't receive the updates (sometimes critical) from SQLite.
-rusqlite = { version = "0.30", features = ["bundled", "unlock_notify"] }
-
-# To talk to bitcoind
-jsonrpc = { version = "0.17", features = ["minreq_http"], default-features = false }
-
-# Used for daemonization
-libc = { version = "0.2", optional = true }
-
-# Used for generating mnemonics
-getrandom = "0.2"
-
-# Used for the hot signer
-bip39 = "2.0"
-
-# Additional entropy for generating mnemonics
-[target.'cfg(target_arch = "x86")'.dependencies]
-rdrand = "0.8"
-[target.'cfg(target_arch = "x86_64")'.dependencies]
-rdrand = "0.8"
+[workspace]
+resolver = "2"
+members = [
+  "liana",
+  "liana-gui",
+  "liana-ui",
+]

@pythcoiner
Copy link
Collaborator

Do you think the Python tests should be in liana/?

i think so, i'd keep the /tests folder to integrations tests implemented in rust if we want to do so in the future.

@@ -17,7 +17,7 @@ path = "src/main.rs"
async-trait = "0.1"
async-hwi = { version = "0.0.24" }
liana = { git = "https://github.com/wizardsardine/liana", branch = "master", default-features = false, features = ["nonblocking_shutdown"] }
Copy link
Collaborator

@pythcoiner pythcoiner Nov 15, 2024

Choose a reason for hiding this comment

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

maybe we should point to the local repo?

Suggested change
liana = { git = "https://github.com/wizardsardine/liana", branch = "master", default-features = false, features = ["nonblocking_shutdown"] }
liana = { path = "../liana", default-features = false, features = ["nonblocking_shutdown"] }

but doing so we fail building:

❯ cargo build --release                                                                                                                 
   Compiling liana v8.0.0 (/home/pyth/rust/liana/liana)
   Compiling liana-gui v8.0.0 (/home/pyth/rust/liana/liana-gui)
error[E0061]: this function takes 2 arguments but 1 argument was supplied
   --> liana-gui/src/daemon/embedded.rs:25:22
    |
25  |         let handle = DaemonHandle::start_default(config.clone()).map_err(DaemonError::Start)?;
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------- an argument of type `bool` is missing
    |
note: associated function defined here
   --> /home/pyth/rust/liana/liana/src/lib.rs:565:12
    |
565 |     pub fn start_default(
    |            ^^^^^^^^^^^^^
help: provide the argument
    |
25  |         let handle = DaemonHandle::start_default(config.clone(), /* bool */).map_err(DaemonError::Start)?;
    |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error[E0061]: this function takes 2 arguments but 1 argument was supplied
   --> liana-gui/src/installer/mod.rs:346:25
    |
346 |     let daemon_handle = liana::DaemonHandle::start_default(cfg);
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^----- an argument of type `bool` is missing
    |
note: associated function defined here
   --> /home/pyth/rust/liana/liana/src/lib.rs:565:12
    |
565 |     pub fn start_default(
    |            ^^^^^^^^^^^^^
help: provide the argument
    |
346 |     let daemon_handle = liana::DaemonHandle::start_default(cfg, /* bool */);
    |                                                           ~~~~~~~~~~~~~~~~~

For more information about this error, try `rustc --explain E0061`.
error: could not compile `liana-gui` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

and should apply this kind of (untested) patch but i don't really understand why it don't fail otherwise:

diff --git a/liana-gui/src/daemon/embedded.rs b/liana-gui/src/daemon/embedded.rs
index c4f77d7..17e1e3a 100644
--- a/liana-gui/src/daemon/embedded.rs
+++ b/liana-gui/src/daemon/embedded.rs
@@ -18,6 +18,10 @@ pub struct EmbeddedDaemon {
 
 impl EmbeddedDaemon {
     pub fn start(config: Config) -> Result<EmbeddedDaemon, DaemonError> {
+        #[cfg(not(all(unix, feature = "daemon")))]
+        let handle =
+            DaemonHandle::start_default(config.clone(), true).map_err(DaemonError::Start)?;
+        #[cfg(all(unix, feature = "daemon"))]
         let handle = DaemonHandle::start_default(config.clone()).map_err(DaemonError::Start)?;
         Ok(Self {
             handle: Mutex::new(Some(handle)),
@@ -31,6 +35,7 @@ impl EmbeddedDaemon {
     {
         match self.handle.lock().await.as_mut() {
             Some(DaemonHandle::Controller { control, .. }) => method(control),
+            Some(DaemonHandle::Server { .. }) => unimplemented!(),
             None => Err(DaemonError::DaemonStopped),
         }
     }
diff --git a/liana-gui/src/installer/mod.rs b/liana-gui/src/installer/mod.rs
index e0adac8..fefe7ce 100644
--- a/liana-gui/src/installer/mod.rs
+++ b/liana-gui/src/installer/mod.rs
@@ -334,7 +334,11 @@ impl Installer {
 
 pub fn daemon_check(cfg: liana::config::Config) -> Result<(), Error> {
     // Start Daemon to check correctness of installation
-    match liana::DaemonHandle::start_default(cfg) {
+    #[cfg(not(all(unix, feature = "daemon")))]
+    let daemon_handle = liana::DaemonHandle::start_default(cfg, true);
+    #[cfg(all(unix, feature = "daemon"))]
+    let daemon_handle = liana::DaemonHandle::start_default(cfg);
+    match daemon_handle {
         Ok(daemon) => daemon
             .stop()
             .map_err(|e| Error::Unexpected(format!("Failed to stop Liana daemon: {}", e))),

Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks to me it's related to changes in 191249a

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like default-features is not taken into account when compiling all the crate:
rust-lang/cargo#8366

Copy link
Member Author

@edouardparis edouardparis Nov 15, 2024

Choose a reason for hiding this comment

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

I think this change is sadly out of scope for now, we need to clean the liana crate and remove the daemonfeature that is not good in my opinion. (Process should not fork itself and user should manage it either with a systemd service or his own scripts.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

i agree, services should be managed by systemd

Copy link
Collaborator

@jp1ac4 jp1ac4 left a comment

Choose a reason for hiding this comment

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

ACK 647e4de.

Copy link
Collaborator

@jp1ac4 jp1ac4 left a comment

Choose a reason for hiding this comment

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

ACK 6175ca7.

@pythcoiner
Copy link
Collaborator

ACK 6175ca7

@edouardparis edouardparis merged commit 89a4187 into wizardsardine:master Nov 15, 2024
24 checks passed
@edouardparis edouardparis deleted the refac-workspace branch November 15, 2024 12:54
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.

3 participants