-
Notifications
You must be signed in to change notification settings - Fork 46
Add WatchtowerClient interface #235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
f11e493
to
4a32fb0
Compare
This PR should be ready for review now. This |
Thank you for this contribution 👍. I took an initial look and the approach looks good. Could you please change your commit structure such that it complies with the dev guidelines (see https://github.com/lightningnetwork/lnd/blob/master/docs/development_guidelines.md)? Specifically, squashing your commits to only contain the essential operations (1. adding the interface/implementation, 2. change of the server name parsing, 3. adding the service). I'd take another look afterwards. |
OK! Commit structure should be good now! |
1d132d1
to
cbfd3b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes! There are still some style nits left. I have also added some rules that we require to the linter. Could you rebase your PR onto #238 such that the linter can give hints?
e9158a1
to
ebe05a0
Compare
Rebased and fixed the various linter complaints relevant to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates, this looks close. I'll test the dependent PR in lndmon next once you have this feedback in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I still have some minor suggestions, but after that it looks good to go 🙏
wtclient_client.go
Outdated
timeout time.Duration | ||
} | ||
|
||
// A compile time check to ensure that wtClientClient implements the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: double space before wtClientClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final nit: wtClientClient
wasn't updated in the comment, you can address this after the other reviewer's feedback
Use strings.TrimSuffix instead of strings.ReplaceAll in the macaroon recipe code. This is necessary because the WtClient macaroon name contains the word "client" and would be malformed in the old code. New code just removes the last "client". Signed-off-by: Jonathan <[email protected]>
Add the wtclient_client.go file which contains the lnd watchtower client interface. Signed-off-by: Jonathan <[email protected]>
Add the watchtower client service to the macaroon recipe and to the main lnd services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉. Thank you for addressing the feedback so quickly 👍
Edit: there are still some linter errors left to be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort, thank you for your contribution!
The linter is currently failing due to lll
issues and I flagged some minor nits.
Once CI is green this LGTM!
Stats(ctx context.Context) (*wtclientrpc.StatsResponse, error) | ||
|
||
// TerminateSession terminates the given session and marks it to not be | ||
// used for backups anymore. Returns a human readable status string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: human-readable
|
||
// newClientClient creates a new watchtower client interface. | ||
func newWtClient(conn grpc.ClientConnInterface, | ||
wtClientMac serializedMacaroon, timeout time.Duration) *wtClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wtClientMac
fits in line before.
} | ||
|
||
// Stats returns the in-memory statistics of the client since startup. | ||
func (m *wtClient) Stats(ctx context.Context) (*wtclientrpc.StatsResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: break line
|
||
// Policy returns the active watchtower client policy configuration. | ||
func (m *wtClient) Policy(ctx context.Context, | ||
policyType wtclientrpc.PolicyType) (*wtclientrpc.PolicyResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: break line
|
||
// WatchtowerClientClient implements the watchtower client interface | ||
// https://lightning.engineering/api-docs/category/watchtowerclient-service/ | ||
// NOTE: For the macaroon code to work correctly this needs to be named |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: correctly,
Pull Request Checklist
in
lnd_services.go
are updated.macaroon_recipes.go
if your PR adds a new method that is calleddifferently than the RPC method it invokes.
This PR adds the WatchtowerClient interface. The reason for adding this is so that it can be used in
lndmon
in a way described here: lightninglabs/lndmon#118