Skip to content
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

multi: create challenger, secret, and interceptor packages #91

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

multi: create challenger, secret, and interceptor packages #91

wants to merge 5 commits into from

Conversation

orbitalturtle
Copy link

This PR is just moving things around and exporting things. It attempts to make a few organizational changes:

  • Moves the challenger, secret, and client_interceptor code to new packages so that 1) they can be imported elsewhere. 2) to get rid of importing cycles when importing this code (particularly for the watchtower case described below)
  • Exports some of the testing mock tools, since they are potentially useful in the watchtower tests, and seems to make sense to keep things as DRY as possible!

Stepping back, the higher level goal here is to be able to use some of the minter and challenger code in the lnd watchtower for minting and verifying lsats. I had talked with @Roasbeef a while back about adding a method of paying a watchtower for storing state updates for other users, and we discussed using lsats for this purpose. This lays the groundwork for that. (Initial PR for the watchtower code incoming soon(tm))

If there are reasons to organize these changes in a different way, I'm more than happy to make adjustments, thanks!

@ellemouton
Copy link
Member

Thanks for the PR @orbitalturtle 🔥

@guggero - I dont have the perms to request myself as reviewer here it seems. Can you request review from me pls?

@bucko13
Copy link
Contributor

bucko13 commented Mar 8, 2023

Just wanted to chime in a plus one on the motivation for this. Being able to use this more as a library when applicable will be huge for lsat adoption imo.

ellemouton
ellemouton reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Looking good!

secrets/secrets.go Outdated Show resolved Hide resolved
aperture.go
@@ -18,9 +18,12 @@ import (
gateway "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
flags "github.com/jessevdk/go-flags"
"github.com/lightninglabs/aperture/auth"
"github.com/lightninglabs/aperture/challenger"
Copy link
Member

Choose a reason for hiding this comment

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

commit message comment:

  • NewLndChallenger isnt altered to take in an lndclient (which is the thing that would cause an import cycle), but rather is altered to take a challenger.InvoiceClient (which lnd client just happens to implement)
  • nitty nit: check out LND contribution guidelines for ideal commit title & message structure 🙏

Copy link
Member

Choose a reason for hiding this comment

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

also - for easier review, I think perhaps do 1 commit where you do the refactoring and then a separate commit for the NewLndChallenger change

Copy link
Author

Choose a reason for hiding this comment

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

I moved the NewLndChallenger changes into a new commit. The change was a bit awkward because NewLndChallenger initially needs AuthConfig, which is defined in the main package. But then in the next commit we remove AuthConfig entirely because it's not needed once we update NewLndChallenger to take in the client as a parameter. This was the cleanest way I could think to make the change - maybe it's still worth it for review readability. Lmk your thoughts!

log.go Outdated Show resolved Hide resolved
challenger/challenger.go Outdated Show resolved Hide resolved
secrets/test_util.go Show resolved Hide resolved
interceptor/log.go Outdated Show resolved Hide resolved
interceptor/client_interceptor.go Outdated Show resolved Hide resolved
interceptor/client_interceptor.go Outdated Show resolved Hide resolved
@guggero guggero removed their request for review April 18, 2023 08:36
@ellemouton
Copy link
Member

@orbitalturtle , planning to continue with this? I think it is pretty much good to go once the nits have been addressed :)

@orbitalturtle
Copy link
Author

@ellemouton Yikes, yes, sorry for being so slow 🦥 to get to this. I don't know how time moves so fast. I'll review and address your feedback this weekend. 🙏

@orbitalturtle orbitalturtle force-pushed the export-challenger branch 2 times, most recently from 7b649fb to f8dc8ba Compare May 25, 2023 00:16
@orbitalturtle
Copy link
Author

@ellemouton Finally got around to this. Sorry for the long wait. Promise if you have any more feedback I'll respond much faster! One comment/question above but otherwise was able to address the rest of the comments.

@Roasbeef
Copy link
Member

Approved the CI run.

@orbitalturtle
Copy link
Author

Thanks @Roasbeef - fixed that linter issue.

@ellemouton ellemouton self-requested a review May 25, 2023 05:42
ellemouton
ellemouton reviewed May 31, 2023
View reviewed changes
Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Looking good!

The one thing im not sure of is the mocks. seems strange that the auth package would supply the mock for the Mint or that the challenger would supply the mock for the InvoiceClient. Also, im not sure how useful these mocks would be elsewhere - from my experience if I am using a mock, i mostly need to tweak it so that it works a certain way and that will be hard to do if importing the mock from another repo. Might be a case of "a little bit of copying is better than a little bit of dependency". Interested to hear other people's thoughts on this too though!

onion_store.go Outdated Show resolved Hide resolved
aperture.go Outdated Show resolved Hide resolved
auth/test_utils.go Outdated Show resolved Hide resolved
interceptor/client_interceptor.go Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Author

@ellemouton Yeah definitely a fair point about the mocks. Here's what I'm envisioning. I'll definitely be making some changes to the mocks to test the aforementioned lnd watchtower payments changes. But I figured it would be a bit odd to copy the mocks and make updates to it in the lnd repo, since someone who might also be interested in these mock updates is unlikely to be looking in lnd/watchtower for them. If someone might want to use or copy these mocks, they're much more likely to look here in the aperture package for inspiration.

As you said, it's awkward that the auth package supplies the Mint mock and the or that the challenger supplies the mock for the InvoiceClient. I'd personally lean towards keeping these mocks exported, but moving them to a more appropriate package -- either a separate "mock" package or just to more appropriate existing packages. Then users can choose to either import or just copy the mocks for whatever projects their working on -- depending on what makes sense for their particular project.

@orbitalturtle
multi: move challenger and secrets files to new packages
1561990 
Moves challenger and secrets files to new packages so they can be
imported elsewhere.
@orbitalturtle
challenger: change NewLndChallenger client parameter
e3d746b 
- Alters NewLndChallenger so that it takes challenger.InvoiceClient in
as a parameter. (This is useful in the case of using NewLndChallenger
in the lnd watchtower project, to avoid having to import lndclient,
which leads to an import cycle.)
- Also deletes the AuthConfig parameter. Since we now pass the LND
client directly into the function, the AuthConfig parameter is no
longer needed.
@orbitalturtle
auth: export mock mint so we can use it elsewhere
e886fa2
@orbitalturtle
challenger: export mock invoice client
fa45009
@orbitalturtle
interceptor: move client interceptor to its own package
8152075
@lightninglabs-deploy
Copy link
Collaborator

@orbitalturtle, remember to re-request review from reviewers when ready

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

3 similar comments
@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

8 similar comments
@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@lightninglabs-deploy
Copy link
Collaborator

Closing due to inactivity

@ellemouton
Copy link
Member

!lightninglabs-deploy mute

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

@ellemouton ellemouton ellemouton left review comments

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

Successfully merging this pull request may close these issues.
5 participants
@orbitalturtle @ellemouton @bucko13 @Roasbeef @lightninglabs-deploy