Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add WatchtowerClient interface #235

wants to merge 3 commits into from
+242 −1

Conversation

Jontified
Copy link

Pull Request Checklist

  • PR is opened against correct version branch.
  • Version compatibility matrix in the README and minimal required version
    in lnd_services.go are updated.
  • Update macaroon_recipes.go if your PR adds a new method that is called
    differently 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

@Jontified Jontified marked this pull request as draft August 4, 2025 07:00
This was referenced Aug 19, 2025
Merged
Open
@Jontified Jontified force-pushed the master branch 2 times, most recently from f11e493 to 4a32fb0 Compare August 19, 2025 12:39
@Jontified Jontified marked this pull request as ready for review August 19, 2025 12:40
@Jontified
Copy link
Author

Jontified commented Aug 19, 2025 •
edited
Loading

This PR should be ready for review now. This lndmon PR uses the changes in this PR, check if out to get some context if needed.

@bitromortac
Copy link
Contributor

This PR should be ready for review now. This lndmon PR uses the changes in this PR, check if out to get some context if needed.

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.

@Jontified
Copy link
Author

This PR should be ready for review now. This lndmon PR uses the changes in this PR, check if out to get some context if needed.

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!

@Jontified Jontified force-pushed the master branch 2 times, most recently from 1d132d1 to cbfd3b6 Compare August 21, 2025 15:04
@bitromortac bitromortac self-requested a review August 21, 2025 16:20
bitromortac
bitromortac reviewed Aug 25, 2025
View reviewed changes
Copy link
Contributor

@bitromortac bitromortac left a 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?

@Jontified Jontified force-pushed the master branch 2 times, most recently from e9158a1 to ebe05a0 Compare August 26, 2025 12:49
@Jontified
Copy link
Author

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?

Rebased and fixed the various linter complaints relevant to this PR.

@bitromortac bitromortac self-requested a review August 26, 2025 13:07
bitromortac
bitromortac reviewed Aug 27, 2025
View reviewed changes
Copy link
Contributor

@bitromortac bitromortac left a 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.

bitromortac
bitromortac reviewed Aug 28, 2025
View reviewed changes
Copy link
Contributor

@bitromortac bitromortac left a 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
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Copy link
Contributor

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

@Jontified
lndclient: Use TrimSuffix instead of ReplaceAll
d559885 
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 <jonathan@mullvad.net>
@Jontified
lndclient: Add watchtower client interface
dc0908d 
Add the wtclient_client.go file which contains the lnd watchtower client
interface.

Signed-off-by: Jonathan <jonathan@mullvad.net>
@Jontified
lndclient: Add the watchtower client service
c3f10fc 
Add the watchtower client service to the macaroon recipe and to the main
lnd services.
@hieblmi hieblmi self-requested a review August 29, 2025 09:57
bitromortac
bitromortac approved these changes Aug 29, 2025
View reviewed changes
Copy link
Contributor

@bitromortac bitromortac left a comment •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bitromortac bitromortac bitromortac approved these changes

@hieblmi hieblmi Awaiting requested review from hieblmi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jontified @bitromortac