Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions CHANGE-10271.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# LND #10271 Implementation Guide

## Changes Needed

### 1. lnrpc/lightning.proto
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While the PR focuses on SendCoinsRequest, it would be beneficial to also add change_address support to SendManyRequest for completeness, as both RPCs involve creating on-chain transactions where a custom change address might be desired.

Add to `SendCoinsRequest`:
```protobuf
string change_address = 15;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The proposed protobuf field definition has two issues:

  1. Field Number Gap: The last field in SendCoinsRequest is 11. Using 15 creates an unnecessary gap. It is recommended to use the next available number, 12, to maintain sequential ordering.
  2. 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.
Suggested change
string change_address = 15;
// The optional address to send the change to.
string change_address = 12;

```

### 2. cmd/lncli/commands.go
Add flag:
```go
cli.StringFlag{
Name: "change_address",
Usage: "Optional address to send change to",
},
```

### 3. lnd/lnwallet/btcwallet/btcwallet.go
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The file path lnd/lnwallet/btcwallet/btcwallet.go contains an unnecessary lnd/ prefix. Based on the repository structure provided in the context, the correct path is lnwallet/btcwallet/btcwallet.go.

Suggested change
### 3. lnd/lnwallet/btcwallet/btcwallet.go
### 3. lnwallet/btcwallet/btcwallet.go

Update `SendCoins` to use change address.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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.

Suggested change
Update `SendCoins` to use change address.
Update `SendOutputs` to use change address.


## Generate Protobuf
```bash
make rpc
```

## Test
```bash
make check
```
69 changes: 69 additions & 0 deletions PR_TODO.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# PR #10769 Completion Checklist

## Completed ✅
- [x] Add `change_addr` field to `SendCoinsRequest` proto
- [x] Add `--change_addr` flag to `sendcoins` CLI command
- [x] Pass change address to SendCoins RPC call
- [x] Push branch to fork
- [x] Create upstream PR

## Remaining Work 📝

### 1. Regenerate Protobuf Code
```bash
make rpc
```
This will regenerate:
- `lnrpc/lightning.pb.go`
- `lnrpc/lightning_grpc.pb.go`

### 2. Add Unit Tests
Create `cmd/commands/commands_test.go` or update existing tests:
```go
func TestSendCoinsWithChangeAddr(t *testing.T) {
// Test that change_addr is properly parsed and passed
}
```

### 3. Add Integration Tests
Update `lntest/itest/lnd_on_chain_test.go`:
```go
func testSendCoinsWithChangeAddr(t *harnessTest) {
// Test sending coins with custom change address
}
```

### 4. Update Documentation
- Update `docs/cmd/lncli.md` if it exists
- Add example usage to PR description

### 5. Wallet Implementation
The wallet needs to actually use the change address. Update:
- `lnwallet/btcwallet/btcwallet.go` - `SendCoins` method
- `rpcserver.go` - Pass change address to wallet

## How to Get Write Access

1. **Become a regular contributor:**
- Submit quality PRs
- Review other PRs
- Participate in discussions

2. **Join the LND community:**
- Discord: https://discord.gg/lightning
- IRC: #lnd on Libera.Chat
- Mailing list: lightning-dev

3. **Build reputation:**
- Fix bugs
- Add features
- Write documentation

4. **Apply for maintainership:**
- After consistent contributions
- Ask existing maintainers

## Current Status
- PR is open and ready for review
- Basic implementation is complete
- Needs protobuf regeneration and tests
7 changes: 7 additions & 0 deletions cmd/commands/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,12 @@ var sendCoinsCommand = cli.Command{
"the amt flag",
},
txLabelFlag,
cli.StringFlag{
Name: "change_addr",
Usage: "(optional) an address to send the change to; if " +
"unset, the change will be returned to the wallet's " +
"change output",
},
},
Action: actionDecorator(sendCoins),
}
Expand Down Expand Up @@ -682,6 +688,7 @@ func sendCoins(ctx *cli.Context) error {
SpendUnconfirmed: minConfs == 0,
CoinSelectionStrategy: coinSelectionStrategy,
Outpoints: outpoints,
ChangeAddr: ctx.String("change_addr"),
}
txid, err := client.SendCoins(ctxc, req)
if err != nil {
Expand Down
Loading
Loading