Add new subservers package
#516
Conversation
There was a problem hiding this comment.
Great work on this, @ellemouton and @positiveblue! Nothing major from my end, except for the missing additional itests around startup (and the new RPCs that were added).
positiveblue
force-pushed
the
subservers
branch
from
97be6f3 to
db9d2d4
Compare
|
itest need to be added |
positiveblue
force-pushed
the
subservers
branch
from
db9d2d4 to
bab21fb
Compare
|
@guggero: review reminder |
|
Needs a rebase though. |
positiveblue
force-pushed
the
subservers
branch
from
bab21fb to
2539ccb
Compare
There was a problem hiding this comment.
Looking good!
a few nits & questions. The main thing that I think should be changed here is that we should keep the behaviour of the Litd startup process the same. In otherwords, if one of the subsystems fail to start, Lit should fail to start. We can change this behaviour once we add the 'status server' 👍
firewall/rule_enforcer.go
Outdated
| mid "github.com/lightninglabs/lightning-terminal/rpcmiddleware" | ||
| "github.com/lightninglabs/lightning-terminal/rules" | ||
| "github.com/lightninglabs/lightning-terminal/session" | ||
| "github.com/lightninglabs/lightning-terminal/subservers" |
There was a problem hiding this comment.
what is the main reason for moving the permissions logic? cause now the subserver package manages LiT's permissions which is kinda strange to me. Perhaps the permissions manager can remain where it is and then subservers can register their permissions with it?
There was a problem hiding this comment.
Good point, the main idea was for the permission manager to be able to get and returns SubServerName(s) typed strings. But it may be worth keeping the permissions where they were + use simple strings
There was a problem hiding this comment.
ah ok I see - so moving things to avoid the circular dependency. But yeah, perhaps in this case a little bit of copying is better than the large code shift.
interested to hear what @guggero 's opinion is on this 🙏
There was a problem hiding this comment.
Yeah, I'd probably also go with having a separate list of the subserver names and keeping the permissions where they are.
positiveblue
force-pushed
the
subservers
branch
2 times, most recently
from
e5044d9 to
10edc66
Compare
terminal.go
Outdated
| @@ -430,30 +456,49 @@ func (g *LightningTerminal) Run() error { | |||
| case <-readyChan: | |||
|
|
|||
| case err := <-g.errQueue.ChanOut(): | |||
| // TODO(positiveblue): set error status for lnd. | |||
There was a problem hiding this comment.
should we maybe not scatter all the TODOs for the status stuff? think it only makes sense if there is actually a status server
positiveblue
force-pushed
the
subservers
branch
2 times, most recently
from
a19c679 to
ee29c85
Compare
Add a `started` variable to the rpcProxy that is used to indicate if the proxy is ready to handle requests. This is because currently the webserver is dependent on the rpcProxy to start and we want to be able to start the webserver without being dependent on the rpcProxy so that it can be used to handle status requests in a future commit. So with this commit, we can now saftely start the webserver earlier on and then if requests come through for the rpcProxy, an error will be displayed to the user.
Remove the responsibility of creating an LND connection from the rpcProxy and instead let the main LightningTerminal struct handle it. All the lnd-connection specific functions are also moved into their own file.
A pure refactor commit. It just splits various parts of the main Run function out into helper functions in order to more clearly show the steps taken to set up LiT.
Since all the relevant dependancies have been accouted for, we know move the starting of the main webserver to earlier on in the Run function. This is in preparation for later when the webserver will be "always-on" and will be used to server status information about the rest of the LiT subservers.
Note: substitutes part of the work in #442, server status will be added in a different PR