search

Blockstream / greenlight

Build apps using self-custodial lightning nodes in the cloud
https://blockstream.github.io/greenlight/getting-started/
MIT License
109 stars 27 forks source link

Replace TlsConfig #383

Closed nepet closed 5 months ago

nepet commented 5 months ago

This PR replaces TlsConfig in the signer and the scheduler completely. Also removes the exaggerated credentials builder in favor of a flattened api.

@cdecker: Please tell me what you think about 744e282 - scheduler: Refactor scheduler to use typestate. This commit provides a more idiomatic (rust) way to handle the scheduler and the credentials. If we don't like it we can just strip it off before we merge, the commit below is in a working state as well.

After we decided if we want to go with this change I need to adapt the backend tests (they use glclient) before we can merge.

nepet commented 5 months ago

Rebased and added some inplace documentation. I may still need to fix glcli, but I am considering rewriting it in rust completely, what do you think? Is a python cli more accessible than a rust one?

cdecker commented 5 months ago

I like the idea of rewriting it in a language that is harder to drift apart, and it allows us to dogfood the rust bindings rather than the less strict python bindings 👍

nepet commented 5 months ago

@Randy808 Seems that we have some conflicting commits. I removed the Result<> from the TlsConfig as it was not used before. This way I can create defaults that get checked during connect. I like the idea of a validation during construction as well. What do you think, should we bubble up the result to the surface layer during construction of the Credentials (unfortunately, this way we won't be able to create defaults anymore) or should we panic on the creation of TlsConfig or creation of the Credentials which I guess is okay as we only run on client side but maybe developers would dislike it? Which way would you prefer?

Randy808 commented 5 months ago

@nepet In the TlsConfig's new you can change the match to return an Option instead of returning an error on failure. That way we can keep changes to a minimum since I also like the defaults.

It would also stay consistent with the pattern followed for load_file_or_default and what I did for identity (since I didn't want to panic there for the same reasons)

Edit: I also should've added a log in identity when the x509 cert couldn't be read so that would be a nice addition