[feature]: add specific change address for send coin RPCs #10271#10810
[feature]: add specific change address for send coin RPCs #10271#10810murraystewart96 wants to merge 18 commits into
Conversation
…ller implementation
…king partial payment
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 ability to pin the change output of on-chain transactions to a specific address. By extending the RPC and internal wallet interfaces, callers can now explicitly define where change UTXOs should be sent. This feature includes robust validation to prevent invalid or dangerous address usage and ensures compatibility with existing wallet logic, such as the reserve requirement mechanism. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟠 High (2 files)
🟡 Medium (3 files)
⚪ Excluded from counting (3 files)
AnalysisThis PR is classified as CRITICAL because it touches multiple files in The changes appear to modify wallet interface signatures and their implementations across No severity bump thresholds were triggered (11 non-generated/non-test files, ~275 lines changed — both below the bump thresholds of 20 files / 500 lines). To override, add a |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to specify a custom change address for on-chain transactions via the SendCoins and SendOutputs RPCs. The changes include updates to the protobuf definitions, CLI commands, and internal wallet logic to propagate and respect the optional change_address field. Feedback from the review highlights several areas for improvement: the change_address field is missing from the SendMany message despite being mentioned in the PR description, and there is duplicated address validation logic in walletkit_server.go that should be consolidated into a shared utility. Additionally, several style guide violations were noted, including the use of block comments instead of double-slashes in protobuf files, improper wrapping of function return values in lnwallet/interface.go, and a typo in a log message comment.
I am having trouble creating individual review comments. Click here to see my feedback.
lnrpc/lightning.proto (1337)
The pull request description mentions that change_address is added to SendMany, but the field is missing from the SendManyRequest message in this file. Please ensure the implementation is complete and consistent with the description.
lnrpc/lightning.proto (1333-1336)
The project typically uses double-slash // for field comments to maintain consistency with the rest of the file. Additionally, per the style guide, comments should ideally follow the FieldName is ... pattern and explain the purpose or context.
// An optional address to send change to. If not set, a wallet-derived
// address is used.
References
- Always use the Golang code style described below in this document. readable code is the most important requirement. (link)
lnrpc/walletrpc/walletkit_server.go (828-838)
This address validation logic is duplicated from the decodeAndValidateAddr helper introduced in rpcserver.go. Furthermore, it omits the check to prevent sending to raw public keys, which is present in the SendCoins implementation. It is recommended to move the validation logic to a shared utility package (e.g., lnwallet or a dedicated address package) to ensure consistency and security across all RPC entry points.
lnwallet/interface.go (376-377)
The wrapping of the return values here violates the style guide. When a function declaration spans multiple lines, the closing parenthesis should not be on its own line if the return values follow, and the return values should ideally be on the same line as the closing parenthesis if they fit within the 80-character limit.
) (*txauthor.AuthoredTx, error)References
- Do not end a line with an open parenthesis if the function definition is not finished. If function arguments exceed the 80-character limit, maintain indentation on following lines. (link)
rpcserver.go (1452)
Typo: 'interal' should be 'internal'.
// Send all requires default internal wallet address to preserve
References
- Readable code is the most important requirement for any commit created. (link)
Change Description
#10271
Adds a change_address field to SendCoins, SendMany, and WalletKit.SendOutputs, allowing callers to pin the change output to a specific address rather than a wallet-derived one.
The field is mutually exclusive with send_all on SendCoins and the --sweepall flag on sendcoins.
Depends on btcwallet changes to support a custom change address at the transaction creation layer:
Issue: btcsuite/btcwallet#1234
PR: btcsuite/btcwallet#1235
Steps to Test
go test ./lnwallet/test/btcd/ -v -run "TestLightningWallet/btcwallet/btcd:create_simple_tx_change_addr"
NOTE: This will fail until the btcwallet PR has been merged and the new version has been brought into this repo
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.