looprpc: preparatory commits for static open channel#1073
looprpc: preparatory commits for static open channel#1073hieblmi wants to merge 4 commits intolightninglabs:masterfrom
Conversation
Summary of ChangesHello @hieblmi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for enabling static open channel functionality within looprpc. It involves a comprehensive update of the Go development environment and project dependencies to align with the latest lnd version. The core change introduces a new RPC for initiating static channel openings, along with necessary data structures and state tracking mechanisms to manage deposits throughout the channel establishment process. These preparatory steps ensure looprpc can leverage lnrpc's capabilities for future static channel features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request prepares the looprpc package for using lnrpc by updating dependencies, Go versions, and build scripts. It also introduces the new StaticOpenChannel RPC and related protobuf messages. The changes are mostly correct and well-structured. My main feedback is about an inconsistency in the Go versions specified across different configuration and module files, which should be aligned to ensure build and linting stability.
| version: "2" | ||
| run: | ||
| go: "1.24" | ||
| go: "1.26" |
There was a problem hiding this comment.
There's an inconsistency in the Go versions specified across the repository. This file sets Go to 1.26, and so do several Dockerfiles and tools/go.mod. However, the go.mod files in the root, looprpc, and swapserverrpc directories specify go 1.25.5.
For consistency and to avoid potential build or linting issues, it's best to align all these to the same Go version. If the intention is to upgrade to Go 1.26, please update the other go.mod files accordingly (e.g., by running go mod tidy -go=1.26 in each module). Otherwise, this file and others should be aligned to 1.25.
fa0a1b9 to
d88cf08
Compare
| GRPC_GATEWAY_VERSION=$(go list -f '{{.Version}}' -m github.com/grpc-ecosystem/grpc-gateway/v2) | ||
|
|
||
| # Get lnd directory from parent go.mod to use the correct version | ||
| lnd_dir=$(cd .. && go list -f '{{.Dir}}' -m github.com/lightningnetwork/lnd) |
There was a problem hiding this comment.
Should we print out lnd_dir? Could help to reassure we're indeed using the right one.
| StaticOpenChannel opens a channel funded by selected static address | ||
| deposits. | ||
| */ | ||
| rpc StaticOpenChannel (StaticOpenChannelRequest) |
There was a problem hiding this comment.
I don't think the commit message is correct, we're extending the proto, not just using this type.
There was a problem hiding this comment.
That's a good clarification, I fixed the comment.
| string address = 2; | ||
| } | ||
|
|
||
| message OutPoint { |
There was a problem hiding this comment.
Maybe also mention in the commit message that the replace is safe as the lnrpc types are the same.
starius
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Left a couple of comments.
| syntax = "proto3"; | ||
|
|
||
| import "swapserverrpc/common.proto"; | ||
| import "lnrpc/lightning.proto"; |
There was a problem hiding this comment.
I propose to squash commits "looprpc: use lnrpc.OutPoint and lnrpc.OpenChannelRequest" and "loop: use lnrpc.OutPoint instead of looprpc.OutPoint". Otherwise commit "looprpc: use lnrpc.OutPoint and lnrpc.OpenChannelRequest" does not compile.
looprpc/perms.go
Outdated
| "/looprpc.SwapClient/StaticOpenChannel": {{ | ||
| Entity: "swap", | ||
| Action: "execute", |
There was a problem hiding this comment.
StaticOpenChannel and Withdrawal are not swaps. Do we need swap:execute permission for them? Can we make another permission to spend a deposit?
There was a problem hiding this comment.
good point. We could use deposit/spend. But unfortunately adding new caveats to existing calls breaks old clients since they need all listed permissions, so they'd have to re-bake their macaroons.
So I'll leave the current permissions untouched, but add a new deposit/spend caveat for the open channel rpc.
| defer mgr.Unlock() | ||
| return len(mgr.subscribers[NotificationTypeReservation]) > 0 | ||
| mockClient.Lock() | ||
| defer mockClient.Unlock() |
There was a problem hiding this comment.
Add an empty line before return
9810c52 to
faf9b1b
Compare
In successive commits we want to access lnrpc specifc methods and objects, so here we import github.com/lightningnetwork/lnd v0.20.1-beta into looprpc and update relevant versions.
…lRequest In this commit we import the lnd's lightning.proto into client.proto to then be able to use lnrpc.OutPoint and lnrpc.OpenChannelRequest instead of the looprpc.OutPoint type. This is safe because the lnrpc and looprpc versions of OutPoint are the same type.
Add macaroon permissions for the new StaticOpenChannel RPC method and change StaticAddressLoopIn action from read to execute to match other swap operations.
faf9b1b to
d4c9ab0
Compare
looprpc: prepare for lnrpc usage
In successive commits we want to access lnrpc specifc methods
and objects, so here we import github.com/lightningnetwork/lnd v0.20.1-beta
into looprpc and update relevant versions.