feat(taker): import seed #3173

holzeis · 2022-10-12T09:22:29Z

allows to import a seed from a file.
only supports a 256 bytes seed file - umbrel seeds are actively rejected.
splits the identity from the wallet seed for taker and maker.
extended the wallet info event for a managed_wallet flag, indicating if the seed was externally provided or generated by ItchySats.
creates an backup file of the old seed file before overwriting it with the imported one.

resolves #2989

see also #3048

bonomat

Had a quick look and added some comments. Will need to have a nother look though :)

crates/daemon/src/seed.rs

bonomat · 2022-10-13T23:03:00Z

crates/daemon/src/wallet.rs

+        ) -> Result<(xtra::Address<Self>, watch::Receiver<Option<WalletInfo>>)> {
+            // generate empty export seed
+            let mut export_seed = [0u8; 256];
+            thread_rng().fill(&mut export_seed);


Given this test will run now regularly we should check if this seed indeed does not hold any Bitcoin 🤑

@bonomat what do you have in mind exactly?

bonomat · 2022-10-13T23:09:23Z

crates/daemon/src/wallet.rs

            })
        }
    }

+    impl Actor<ElectrumBlockchain, Tree> {
+        pub async fn spawn_test(


I'm not sure this is a good idea: this will run an online test against testnet using public infrastructure.
Besides that it can have random side effects if the server is down, it might slow down our test executions.

Do you think we can use our test wallet for this?

well.. we could do that, but I have the feeling we would very quickly end up with testing only test code. however, I can give it a try.

@bonomat have a look at 01f62ef. It uses the test wallet now, and does not sync with electrum for balance, but I guess checking on the restored address is sufficient.

taker-frontend/src/components/Wallet.tsx

luckysori

Nice work!

I am mostly unsure about:

Importing leading to overwriting the current wallet seed.
The way we determine the environment.

crates/maker/src/main.rs

crates/bdk-ext/src/lib.rs

crates/daemon-tests/tests/main/import_seed.rs

crates/daemon/src/lib.rs

luckysori · 2022-10-18T03:10:21Z

crates/daemon/src/wallet.rs

    use tokio_extras::Tasks;

-    impl Actor<(), bdk::database::MemoryDatabase> {
-        pub fn new_offline(
+    const IMPORT_SEED: [u8; 256] = [


🙃 Since it's only used in one test I would just inline it.

❓ Why can't it be a random seed with the right length?

@luckysori I am asserting on the address. To ensure that from the same import seed we will always derive the same address. Before, it was also used to check the balance (it owns 0.001 btc)

Do we need to assert on the address? Maybe it's enough to just check that it changes as that implies that a new wallet has been loaded.

what benefits would we get by that - i feel like the test would be less effective without that assertion.

I suppose it is valuable to check that importing a wallet is deterministic.

I would then suggest to move the expected seed into the specific test (I believe it is only used once). It's a bit noisy otherwise.

luckysori · 2022-10-18T03:11:38Z

crates/daemon/src/wallet.rs

@@ -512,6 +591,7 @@ mod tests {
                    time_to_lock,
                },
                blockchain_client: (),
+                db: None,


❓ Why is this field an Option? I would expect to always be able to provide the database.

@luckysori well, I don't like it either, but I didn't find a better way of doing it with the existing tests. Since the tests mainly use a MemoryDatabase, I do not have a Db available there. Do you have a suggestion?

Hmm, I didn't come up with anything after some limited experimenting. I might come back to this later myself.

crates/daemon/src/wallet.rs

crates/taker/src/routes.rs

holzeis · 2022-10-18T08:54:58Z

@luckysori

I am mostly unsure about:

1. Importing leading to overwriting the current wallet seed.

I've committed a change 84a510c which will automatically copy the existing seed file into a backup file with the timestamp in its name. e.g. taker_seed.1666082503.back

2. The way we determine the environment.

Alternatively to the environment variable, I could make use of the app seed argument, meaning if the seed is provided from extern we are not managing it in terms of importing or exporting. This will then be reflect with a simple boolean.

e.g. something like that.

let managed_seed = match opts.app_seed {
    Some(_) => false,
    None => true,
};

I will wait for your feedback before implementing that path though, as I am not sure if this is a much better approach than the current one.

luckysori · 2022-10-19T03:49:58Z

Alternatively to the environment variable, I could make use of the app seed argument, meaning if the seed is provided from extern we are not managing it in terms of importing or exporting. This will then be reflect with a simple boolean.

e.g. something like that.
let managed_seed = match opts.app_seed {
    Some(_) => false,
    None => true,
};
I will wait for your feedback before implementing that path though, as I am not sure if this is a much better approach than the current one.

I think it is better to rely on the presence of the app seed argument, because that is the reason why importing (and perhaps even exporting?) a seed shouldn't be allowed. On the other hand, the environment variable was introduced for metrics.

What I also care about is disabling the route based on this information, rather than exposing an event via SSE that the client has to interpret. It removes an event that doesn't really belong there imo and it also means that the API is unavailable even if you don't go through the UI.

holzeis · 2022-10-19T07:39:54Z

What I also care about is disabling the route based on this information, rather than exposing an event via SSE that the client has to interpret. It removes an event that doesn't really belong there imo and it also means that the API is unavailable even if you don't go through the UI.

That's actually a very good point. I will adapt that - however i still need to transfer the information about the disabled import / export feature. I will implement it using the app seed argument. Thanks for the feedback!

holzeis · 2022-10-19T08:55:10Z

@luckysori have a look at a98bdb5.

I've introduced a managed_wallet flag and added it to the WalletInfo event. Additionally the export / import routes will only be added in case the wallet is managed. I like it much better now, wdyt?

luckysori · 2022-10-20T05:22:16Z

I've introduced a managed_wallet flag and added it to the WalletInfo event. Additionally the export / import routes will only be added in case the wallet is managed. I like it much better now, wdyt?

Nice work! I like it much better too. It makes sense for it to go with the WalletInfo event!

luckysori

Thanks for addressing my comments!

holzeis · 2022-10-24T05:00:55Z

Thanks for addressing my comments!

Thanks for taking the time and give me comments 🙂 👍

holzeis · 2022-10-25T07:50:10Z

bors r+

bors · 2022-10-25T08:23:38Z

Build succeeded:

holzeis requested review from bonomat, da-kami, Restioson, klochowicz and luckysori October 12, 2022 09:22

holzeis force-pushed the feat-restore-seed-file branch 3 times, most recently from 5770509 to 2b7a9f6 Compare October 12, 2022 09:40

holzeis self-assigned this Oct 12, 2022

holzeis force-pushed the feat-restore-seed-file branch from 2b7a9f6 to 58c2786 Compare October 13, 2022 14:02

bonomat reviewed Oct 13, 2022

View reviewed changes

luckysori reviewed Oct 18, 2022

View reviewed changes

holzeis force-pushed the feat-restore-seed-file branch from 01f62ef to 51eb1c8 Compare October 18, 2022 07:56

luckysori approved these changes Oct 20, 2022

View reviewed changes

holzeis force-pushed the feat-restore-seed-file branch from 4f8555e to 4ee33ac Compare October 24, 2022 05:02

holzeis added 2 commits October 25, 2022 09:01

feat(taker): import seed

4e49800

feat(seed): split wallet and identity seed files

583a3df

holzeis force-pushed the feat-restore-seed-file branch from 4ee33ac to 583a3df Compare October 25, 2022 07:02

bors bot merged commit 53acc31 into master Oct 25, 2022

bors bot deleted the feat-restore-seed-file branch October 25, 2022 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(taker): import seed #3173

feat(taker): import seed #3173

holzeis commented Oct 12, 2022 •

edited

Loading

bonomat left a comment

bonomat Oct 13, 2022

holzeis Oct 19, 2022

bonomat Oct 13, 2022

holzeis Oct 14, 2022 •

edited

Loading

holzeis Oct 14, 2022

luckysori left a comment

luckysori Oct 18, 2022

luckysori Oct 18, 2022

holzeis Oct 18, 2022

luckysori Oct 19, 2022

holzeis Oct 19, 2022

luckysori Oct 19, 2022

luckysori Oct 18, 2022

holzeis Oct 18, 2022

luckysori Oct 19, 2022

holzeis commented Oct 18, 2022

luckysori commented Oct 19, 2022 •

edited

Loading

holzeis commented Oct 19, 2022

holzeis commented Oct 19, 2022

luckysori commented Oct 20, 2022

luckysori left a comment

holzeis commented Oct 24, 2022

holzeis commented Oct 25, 2022

bors bot commented Oct 25, 2022

feat(taker): import seed #3173

feat(taker): import seed #3173

Conversation

holzeis commented Oct 12, 2022 • edited Loading

bonomat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzeis commented Oct 18, 2022

luckysori commented Oct 19, 2022 • edited Loading

holzeis commented Oct 19, 2022

holzeis commented Oct 19, 2022

luckysori commented Oct 20, 2022

luckysori left a comment

Choose a reason for hiding this comment

holzeis commented Oct 24, 2022

holzeis commented Oct 25, 2022

bors bot commented Oct 25, 2022

holzeis commented Oct 12, 2022 •

edited

Loading

holzeis Oct 14, 2022 •

edited

Loading

luckysori commented Oct 19, 2022 •

edited

Loading