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.

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

swapserverrpc: add module for server protos #445

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Dec 10, 2021

Pull Request Checklist

  • Update release_notes.md if your PR contains major features, breaking changes or bugfixes

@carlaKC carlaKC force-pushed the module-swapserverrpc branch from 3f4ba51 to f3927bb Compare December 10, 2021 14:20
@carlaKC carlaKC marked this pull request as ready for review December 10, 2021 14:20
@carlaKC carlaKC force-pushed the module-swapserverrpc branch 4 times, most recently from 1e6ed74 to 17b8277 Compare December 13, 2021 07:48
@carlaKC carlaKC requested review from bhandras and guggero December 13, 2021 08:05
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 👍

go.mod Outdated
@@ -11,6 +11,7 @@ require (
github.com/jessevdk/go-flags v1.4.0
github.com/lightninglabs/aperture v0.1.6-beta
github.com/lightninglabs/lndclient v0.14.0-5
github.com/lightninglabs/loop/swapserverrpc v0.0.0-00010101000000-000000000000
Copy link
Member

Choose a reason for hiding this comment

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

This could use a future tag (e.g. v1.0.0) we add when the PR is merged right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if go mod somehow allows it, I'd point this to a tag that doesn't exist yet so we can just create and push it once the PR is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL you can do that!

go.mod Outdated
@@ -11,6 +11,7 @@ require (
github.com/jessevdk/go-flags v1.4.0
github.com/lightninglabs/aperture v0.1.6-beta
github.com/lightninglabs/lndclient v0.14.0-5
github.com/lightninglabs/loop/swapserverrpc v0.0.0-00010101000000-000000000000
Copy link
Member

Choose a reason for hiding this comment

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

Yes, if go mod somehow allows it, I'd point this to a tag that doesn't exist yet so we can just create and push it once the PR is merged.

Protobuf does not allow naming conflicts for files within the same
process, because all proto messages register themselves in a global
registry.

This is problematic because the server's itests import the client's
looprpc package to make rpc queries to the loopd client, thus importing
duplicate common.proto and server.proto from the client's looprc package
(since they're both in there as well).

This change moves the server's proto files into their own directory so
that they are not imported when we want to use the client's files. We
cannot change the package name for the server, because that would be
a breaking change (the package name is included in URIS). Fortunately,
we have the go_package option which allows us to place generated files
in a different location.
To make it possible to replace the swapserverrpc module in the server,
we declare it as its own submodule. This allows the server to make
changes to its local protos (and replace loop's copy with the local
version), to use changes that are not yet merged to the client repo.
@carlaKC carlaKC force-pushed the module-swapserverrpc branch from 17b8277 to a9e7748 Compare December 13, 2021 11:59
@carlaKC carlaKC merged commit 85dc0b2 into lightninglabs:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants