lightninglabs / lightning-node-connect

MIT License
78 stars 22 forks source link

wasm: pass keys in as parameters to ConnectServer #42

Closed ellemouton closed 2 years ago

ellemouton commented 2 years ago

In this commit, the way in which the local and remote keys are specified is changed. Instead of passing them in as command line arguments to the binary, they are instead passed in as parameters to the ConnectServer function.

This change allows the wasm binary to be pre-loaded and then only providing the keys afterwards. Rather than needing to provide the keys at load time.

ellemouton commented 2 years ago

cc @jamaljsr

kaloudis commented 2 years ago

Related lnc-web PR https://github.com/lightninglabs/lnc-web/pull/20

jamaljsr commented 2 years ago

Hey @ellemouton, absolutely impressive turnaround time on this update. This is a big deal for lnc-web, so thank you for the update.

I just have one request/question that I'd like to run by you and @kaloudis. Do y'all this it would be better to have two separate connect functions? One to connectWithPhrase for the first handshake and one to connectWithKeys for the subsequent handshakes. This isn't a huge deal but I think it improves the design of the API. It feels a bit strange passing in empty strings for the phrase or keys depending on which data the caller has. Any thoughts on this?

ellemouton commented 2 years ago

Do y'all this it would be better to have two separate connect functions? One to connectWithPhrase for the first handshake and one to connectWithKeys for the subsequent handshakes.

I can definitely change the API to have these 2 separate functions rather but all that does is move the decision making point of which handshake we are going to do from the go binary to the thing calling it. The current API is designed such that the caller does not need to worry about which handshake we are going to do, as long as they provide a remote key if one is stored, then all will work. Like in the example client, i initialise remoteKey as an empty string and then if one is stored in local storage, the variable gets populated. But the same variable gets passed to the ConnectServer function either way.

But maybe there is some context I am missing? Let me know, I can definitely change things around if needed. But yeah, ideally dont want the user to really need to worry about which handshake we are doing

ellemouton commented 2 years ago

rebased 👍

jamaljsr commented 2 years ago

I can definitely change the API to have these 2 separate functions rather but all that does is move the decision making point of which handshake we are going to do from the go binary to the thing calling it. The current API is designed such that the caller does not need to worry about which handshake we are going to do, as long as they provide a remote key if one is stored, then all will work. Like in the example client, i initialise remoteKey as an empty string and then if one is stored in local storage, the variable gets populated. But the same variable gets passed to the ConnectServer function either way.

But maybe there is some context I am missing? Let me know, I can definitely change things around if needed. But yeah, ideally dont want the user to really need to worry about which handshake we are doing

Ah ok, thanks for the rationale. Now I understand why it is designed this way.

I guess maybe this is more of a discussion for the API of lnc-web because that library has the capability of managing the keys for the app. It seems to me that the app will always have to determine whether we are using the first handshake or the second because it needs to prompt the user to enter a pairing phrase if the keys are not available. So the code path has to diverge depending on the current situation. To use an analogy, it feels like the difference between Register versus Login. This is indeed an app-level concern, so it's fair to keep LNC ConnectServer as is.

ellemouton commented 2 years ago

ah! ok yeah makes sense. Cool but let me know, am happy to add the option if it will make lives easier :)