[WIP] Add change address to SendCoinsRequest (#10271)#10769
[WIP] Add change address to SendCoinsRequest (#10271)#10769code-orange-dev wants to merge 4 commits intolightningnetwork:masterfrom
Conversation
Add change_address field to SendCoinsRequest. Relates-to: lightningnetwork#10271
PR Severity: LOW
Low (1 file):
AnalysisThis PR adds a single markdown changelog file (CHANGE-10271.md). Markdown documentation and release-note files fall into the LOW severity tier, as they require no code review of critical subsystems. No severity bump conditions apply: only 1 non-test/non-generated file is touched, and 31 lines changed (well below the 500-line threshold). To override, add a severity-override-{critical,high,medium,low} label. |
Summary of ChangesHello, 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 introduces the initial planning and documentation for adding a custom change address option to the SendCoinsRequest functionality. It serves as a roadmap for upcoming changes to the RPC definitions, CLI commands, and wallet logic. 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. 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. Footnotes
|
Add --change_addr flag to lncli sendcoins command, allowing users to specify a custom change address instead of using the wallet's internal change output. Changes: - Add change_addr flag to sendcoins CLI command - Add ChangeAddr field to SendCoinsRequest proto - Pass change address to SendCoins RPC call Fixes lightningnetwork#10271 🍊 Code Orange Dev School contribution
🟡 PR Severity: MEDIUM
🟡 Medium (2 files)
🟢 Low (1 file)
AnalysisThe overall severity is MEDIUM, driven by two files:
No severity bump applies: only 3 non-test/non-generated files changed (threshold: 20) and 42 lines changed (threshold: 500). No multiple distinct critical packages are touched. To override, add a |
Add basic unit test to verify SendCoinsRequest properly handles the ChangeAddr field. Part of lightningnetwork#10271
There was a problem hiding this comment.
Code Review
This pull request introduces an implementation guide for adding custom change address support to the SendCoins RPC. The reviewer provided several constructive suggestions to improve the guide, including extending support to the SendManyRequest RPC, correcting protobuf field numbering and documentation, and fixing inaccuracies regarding file paths and method names in the wallet logic.
|
|
||
| ## Changes Needed | ||
|
|
||
| ### 1. lnrpc/lightning.proto |
| ### 1. lnrpc/lightning.proto | ||
| Add to `SendCoinsRequest`: | ||
| ```protobuf | ||
| string change_address = 15; |
There was a problem hiding this comment.
The proposed protobuf field definition has two issues:
- Field Number Gap: The last field in
SendCoinsRequestis11. Using15creates an unnecessary gap. It is recommended to use the next available number,12, to maintain sequential ordering. - Missing Documentation: Per the instructions at the top of
lightning.proto(lines 8-11), all fields should have a documentation comment as they are parsed into the API documentation.
| string change_address = 15; | |
| // The optional address to send the change to. | |
| string change_address = 12; |
| }, | ||
| ``` | ||
|
|
||
| ### 3. lnd/lnwallet/btcwallet/btcwallet.go |
There was a problem hiding this comment.
| ``` | ||
|
|
||
| ### 3. lnd/lnwallet/btcwallet/btcwallet.go | ||
| Update `SendCoins` to use change address. |
There was a problem hiding this comment.
The BtcWallet struct in lnwallet/btcwallet/btcwallet.go does not have a SendCoins method. The logic for creating and sending on-chain transactions is handled by the SendOutputs method. The guide should be updated to reflect the correct method name to avoid confusion during implementation.
| Update `SendCoins` to use change address. | |
| Update `SendOutputs` to use change address. |
Add testSendCoinsWithChangeAddr to verify the change_addr feature works correctly in an integration test environment. The test: - Creates destination and change addresses - Sends coins with custom change address - Verifies change output goes to specified address - Confirms transaction is mined successfully Part of lightningnetwork#10271
|
@code-orange-dev, remember to re-request review from reviewers when ready |
Work in Progress
This PR adds
change_addresssupport toSendCoinsRequest.TODO
See CHANGE-10271.md for implementation guide.
Relates-to: #10271
🍊 Contributed via Code Orange Dev School