search

vulpemventures / ocean

:ocean: Elements/Liquid wallet daemon
MIT License
5 stars 7 forks source link

[WalletService] CreateWallet and RestoreWallet seems to be duplicate #7

Closed louisinger closed 2 years ago

louisinger commented 2 years ago

Not sure to understand the difference between RestoreWallet and CreateWallet. IMO, we should restore in all cases even if it's the first time the user creates the wallet (via CreateWallet) because he could for instance reuse an old seed. In that case, we MUST detect transactions and addresses already used.

tiero commented 2 years ago

Agreed, maybe InitWallet is better?

louisinger commented 2 years ago

Or nothing ? I mean, wallet config is really something we need to expose via a service ? In my mind, the user who owns the wallet will run the ocean instance. At startup time, the ocean "server" could request the seed for instance.

tiero commented 2 years ago

GDK has a "login" method, we in the tdex daemon (and LND for example) we have InitWallet

We can request a seed automatically, but how it works when you want to restore? Messing up manually with the file storage? I think an RPC is needed

sekulicd commented 2 years ago

From my POV it is ok to have both methods RestoreWallet and CreateWallet. With this we let one method do one thing and not having "one do it all", i think this way is cleaner for dev side but also for the wallet user when reading the doc.

louisinger commented 2 years ago

From my POV it is ok to have both methods RestoreWallet and CreateWallet. With this we let one method do one thing and not having "one do it all", i think this way is cleaner for dev side but also for the wallet user when reading the doc.

@sekulicd my main problem is the GDK implementation. "restoring" concept does not exist in Green (for them create == restore). Thus, we can't tell to GDK to "do not restore". In conclusion, it's ok to have two RPCs but in some cases (including GDK wallets) create and restore will do the same thing. Which could also disturb the user.

tiero commented 2 years ago

Is it CreateWallet redundant with GenSeed + RestoreWallet ?

louisinger commented 2 years ago

Is it CreateWallet redundant with GenSeed + RestoreWallet ?

using GDK ? yes

altafan commented 2 years ago

I don't think the interface should be influenced by any of the implementations. This is about user experience, not about how gdk or any other kind of wallet does things.

IMO, it's better here to have a clear separation and have 2 rpcs CreateWallet and RestoreWallet for creating a wallet from scratch or restoring one from a mnemonic.

@louisinger the gdk implementation, in this case, could just do the same things for both RPCs given the spec of the gdk apis, but it's totally fine. There maybe some extra steps we could do during the restore though, like checking if the utxo set has been completely restored. Just an example, it may be completely irrelevant, but in general, there are things we can at least enforce in the "restore" flow compared to the "create" one, where we don't need to take care of anything because the wallet is brand new.

altafan commented 2 years ago

This is not strictly related to the topic, but I want at least to point out that with this design of the RPCs it's not possible at all to restore wallets with accounts using custom template scripts, regardless having a single InitWallet or 2 distinct rpc.

We may need to iterate over the protos once more to make it possible for such "custom" wallets to support the restore flow.