Feat installer #123

edouardparis · 2021-05-18T17:31:59Z

Add installer

Different use case:

--conf is present, the app tries to run and returns error if failure.
--datadir is present, the app tries to run and starts installer if config is
not fund in datadir or datadir does not exist.
no flags, the app tries to run with default datadir path, starts installer
if config is not fund in default datadir or default datadir does not exist.

close Build the setup wizard #106

danielabrozzoni

I scrolled briefly through it but I still haven't tested it. Can you add some documentation on how to start it, what it does etc?

src/revault.rs

src/app/config.rs

src/installer/mod.rs

src/installer/message.rs

src/installer/step/manager.rs

danielabrozzoni

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: PolicyCompilation(PolicyError(DuplicatePubKeys))', src/installer/step/stakeholder.rs:113:42
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[1]    21753 segmentation fault (core dumped)  ../revault-gui/target/debug/revault-gui --datadir /tmp/fakedatadir

:(

Overall, it looks really nice! But, either here or in a following PR, we should add some checks on the user inputs. Some issues I found:

you can add only one stakeholder, even tho the minimum is two
the manager threshold can be higher than the number of the managers, so you can create a super secure 8of1 (I saw a check for this in the code though 🤔 I think you allow the user to go on even if there are warnings, maybe somewhere else as well)
we accept whatever in the field cosigner keys, even aaaaaa
same for all the fields host and noise key (watchtowers, coordinator, cosigners...), and for the cookie in bitcoind, we should check that the input is valid
I think we should ask for the revaultd_path as well, otherwise the installer won't start correctly the gui :(
As I was saying in ui: Give a fixed width to the buttons #141, I think all the buttons should have fixed width - for example, it would be nice to have the "stakeholder", "stakeholder & manager", "manager" buttons with the same widths, or the "save" and "install"
As I was saying in Avoid putting the whole view in a Scrollable #125, some elements of the Ui should stay outside the scroll - like the "previous" or "save" buttons

danielabrozzoni · 2021-06-18T14:45:22Z

src/installer/step/manager.rs

+                    }
+                    message::Action::Decrement => {
+                        self.treshold_warning = false;
+                        if self.managers_treshold > 0 {


nit: maybe the threshold should start from 1?

danielabrozzoni · 2021-06-18T14:45:38Z

src/installer/step/manager.rs

+                    }
+                    message::Action::Decrement => {
+                        self.spending_delay_warning = false;
+                        if self.spending_delay > 0 {


same here, should be at least 1

src/installer/step/mod.rs

src/installer/view.rs

danielabrozzoni · 2021-06-18T15:24:58Z

src/installer/view.rs

+                .push(col_address)
+                .push(col_cookie)
+                .push(
+                    button::primary(&mut self.save_button, button::button_content(None, "Save"))


You can probably write "Install" in this button, no need to click install in the next view

I mean: you write "Install" here in this button, and you actually install clicking that button, and the next view is only a "everything went good"

danielabrozzoni · 2021-06-18T15:25:30Z

src/installer/view.rs

+        warning: Option<&String>,
+    ) -> Element<Message> {
+        let mut col = Column::new()
+            .push(text::bold(text::simple("You reached the end")).size(50))


I would put here "RevaultGUI is installed!" (or a better name, or a better phrasing) and the button for starting

danielabrozzoni · 2021-06-18T15:32:59Z

src/main.rs

+#[derive(Debug, Clone)]
+pub enum Message {
+    Install(installer::Message),
+    Run(app::Message),


cargo clippy is sad :(

warning: large size difference between variants --> src/main.rs:69:5 | 69 | Run(app::Message), | ^^^^^^^^^^^^^^^^^ this variant is 552 bytes | note: and the second-largest variant is 48 bytes: --> src/main.rs:68:5 | 68 | Install(installer::Message), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant help: consider boxing the large fields to reduce the total size of the enum | 69 | Run(Box<app::Message>), | ^^^^^^^^^^^^^^^^^

src/installer/step/mod.rs

danielabrozzoni

Fill private noise key -> Would be nice to have here a "your", Fill your private noise key. It would also be nice to have the validation while you write and not only when you click next, but I'm ok with merging even without this.
As we were saying in private chat, I think we should clarify how the secret is encoded? I think (but I'm not sure) at the moment the gui is treating it as utf-8 encoded (it uses as_bytes)
We should validate that the emergency address is on the same network as bitcoind, otherwise revaultd won't start :(
For doing this I think we should ask the emergency address after asking the bitcoind params

Again, I have no idea if the installation is correct as I can't start it, as the revaultd_path is not set in the revault_gui.toml

src/revaultd/config.rs

src/installer/mod.rs

src/installer/view.rs

edouardparis · 2021-07-02T14:35:50Z

install revaultd in your .cargo/bin with cd revaultd & cargo install --path .
GUI expect that revaultd is installed

danielabrozzoni · 2021-07-05T08:39:42Z

I would prefer avoiding installing revaultd on my machine actually 😅 We'll come up with something, I guess :)

ACK 4e54d3b - I didn't review the code in deep, but I tested extensively

darosior · 2021-07-05T08:52:26Z

I agree. I think that's the main argument for his idea of starting `revaultd`'s `main` in a thread. The REVAULTD_PATH would be great to have for us but not end users.

…

-------- Original Message --------

On Jul 5, 2021, 10:39, Daniela Brozzoni wrote: I would prefer avoiding installing revaultd on my machine actually 😅 We'll come up with something, I guess :) ACK [4e54d3b](4e54d3b) - I didn't review the code in deep, but I tested extensively — You are receiving this because your review was requested. Reply to this email directly, [view it on GitHub](#123 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F4MQGY4R5U7AXKBL53TWFVVVANCNFSM45C6ZISQ).

edouardparis · 2021-07-05T09:09:07Z

ACK 4e54d3b

edouardparis force-pushed the feat-installer branch 9 times, most recently from 0d54b35 to dd711b9 Compare May 26, 2021 14:25

edouardparis force-pushed the feat-installer branch 6 times, most recently from 3549330 to ef7d83d Compare June 1, 2021 09:12

edouardparis force-pushed the feat-installer branch from e5a5243 to 6c9db3d Compare June 7, 2021 13:15

edouardparis marked this pull request as ready for review June 10, 2021 08:57

edouardparis requested review from danielabrozzoni and darosior June 10, 2021 08:57

edouardparis force-pushed the feat-installer branch from 6c9db3d to 30f5707 Compare June 10, 2021 09:01

danielabrozzoni suggested changes Jun 11, 2021

View reviewed changes

edouardparis force-pushed the feat-installer branch from 30f5707 to 3a27560 Compare June 16, 2021 13:58

danielabrozzoni suggested changes Jun 18, 2021

View reviewed changes

danielabrozzoni reviewed Jun 18, 2021

View reviewed changes

danielabrozzoni mentioned this pull request Jun 21, 2021

Setup Wizard design #116

Closed

edouardparis force-pushed the feat-installer branch 3 times, most recently from a7c9f4a to 2b3fef9 Compare June 22, 2021 11:57

edouardparis requested a review from danielabrozzoni June 22, 2021 11:57

edouardparis force-pushed the feat-installer branch from 2b3fef9 to 48b0836 Compare June 22, 2021 12:57

edouardparis added 13 commits July 1, 2021 10:32

installer: use form::Value for our_xpub

52fd4e9

installer: add form::Value to DefineBitcoind

05729e5

fix installer: DefineStakeholdersXpubs as Stakeholder

3b294fb

fix installer: threshold

fe0fea0

fix installer: default threshold & spending delay

f919c12

fix installer: set cosigners fixed number

5685203

fix installer: descriptor tests

897a553

fix installer view: wording

2985e40

installer: validate noise key and socket addr

b5cd36b

installer: fill default value for bitcoind

33fdafb

fix installer: coordinatord step message

3f2f851

fix revaultd: default config file name

d18a629

installer: enforce cpfp xpubs number

424e6b9

edouardparis force-pushed the feat-installer branch from ef835e0 to 424e6b9 Compare July 1, 2021 08:32

installer: DefinePrivateNoiseKey

d26699e

edouardparis requested a review from danielabrozzoni July 1, 2021 16:10

danielabrozzoni reviewed Jul 2, 2021

View reviewed changes

src/revaultd/config.rs Show resolved Hide resolved

src/installer/mod.rs Outdated Show resolved Hide resolved

src/installer/view.rs Outdated Show resolved Hide resolved

src/installer/view.rs Outdated Show resolved Hide resolved

edouardparis added 6 commits July 2, 2021 15:19

fix installer: clippy

5e59f2c

fix revaultd: clippy error

539eb2d

fix installer: clippy error for spending delay

bbd0496

fix installer: address view

ff10b4d

fix installer: noise key warning

2a3364e

fix installer: validate emergency address

4e54d3b

edouardparis requested a review from danielabrozzoni July 3, 2021 17:42

danielabrozzoni approved these changes Jul 5, 2021

View reviewed changes

edouardparis merged commit 71fdc9a into revault:master Jul 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat installer #123

Feat installer #123

edouardparis commented May 18, 2021 •

edited

Loading

danielabrozzoni left a comment

danielabrozzoni left a comment

danielabrozzoni Jun 18, 2021

danielabrozzoni Jun 18, 2021

danielabrozzoni Jun 18, 2021

danielabrozzoni Jun 21, 2021

danielabrozzoni Jun 18, 2021

danielabrozzoni Jun 18, 2021

danielabrozzoni left a comment

edouardparis commented Jul 2, 2021

danielabrozzoni commented Jul 5, 2021

darosior commented Jul 5, 2021 via email

edouardparis commented Jul 5, 2021

Feat installer #123

Feat installer #123

Conversation

edouardparis commented May 18, 2021 • edited Loading

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni Jun 18, 2021

Choose a reason for hiding this comment

danielabrozzoni Jun 18, 2021

Choose a reason for hiding this comment

danielabrozzoni Jun 18, 2021

Choose a reason for hiding this comment

danielabrozzoni Jun 21, 2021

Choose a reason for hiding this comment

danielabrozzoni Jun 18, 2021

Choose a reason for hiding this comment

danielabrozzoni Jun 18, 2021

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

edouardparis commented Jul 2, 2021

danielabrozzoni commented Jul 5, 2021

darosior commented Jul 5, 2021 via email

edouardparis commented Jul 5, 2021

edouardparis commented May 18, 2021 •

edited

Loading