-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
lnrpc: add optional labels to on chain transactions #4213
lnrpc: add optional labels to on chain transactions #4213
Conversation
e76806c
to
28acfe8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding tests, we should also extend one of the existing integration tests to make sure the label is being shown properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool feature, i know this has been requested for some time!
1609297
to
4d180ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to start labeling sweeper transactions? would be nice to distinguish them from user submitted ones. could go further and label the various publishing locations throughout the codebase
Yeah definitely, actually have some discussion of this in the original issue. I was going to do it in a follow up, but it's actually quite a small change so added it here :)
I couldn't comment on the existing thread, but the labels could go as a follow-up as originally planned. We may want to brainstorm on the labels a bit more (mostly sweeps) and having that discussion be a blocker on this PR doesn't seem necessary.
Sweep labels are pretty challenging because we combine multiple outputs in a single sweep. I was originally thinking that we could label exactly what a sweep is for, but that would require having labels by tx input, which I think is beyond the scope of this field.
For the time being I could remove the sweep label and leave the others in (or the api label at the very least, since I think that's nice to include with the rpc changes) so that we can brainstorm? Def agree that we don't want to start writing labels now which we may want to change later, so happy to pull out any labels we aren't certain of into a follow up. |
I think you forgot to push @carlaKC 😛
Maybe let's just aim for the API and user send labels for now? For |
🤦♀️ many prs named label right now, pushed the wrong one.
No harm in taking more time to think about them, will go ahead with just the |
069e398
to
a32e0b6
Compare
a32e0b6
to
36f7cdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweep labels are pretty challenging because we combine multiple outputs in a single sweep. I was originally thinking that we could label exactly what a sweep is for, but that would require having labels by tx input, which I think is beyond the scope of this field.
I think it'd be fine to just say some thing like "batch sweep transaction at height=x". you're right, we really want tx-input labels but i agree that it is out of scope :)
PR looks in good shape, is the plan to do a follow up with the subsystem labels or do it all in one go? either way is fine by me
Should be addressed by #4157, not quite per tx input labels, but gives us a list of eech on chain resolution per channel which can then be matched with th e output form
@wpaulino pointed out that we might want to think more carefully about the labels we use for other sybsystems, so I'll add other labels in a follow up. Some labels like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending merge of the btcwallet PR.
Can be rebased now that the dependent PR has been merged. |
36f7cdd
to
7840d61
Compare
7840d61
to
1b12bdc
Compare
@carlaKC looks good, just needs a rebase! |
1b12bdc
to
1f7b7e7
Compare
1f7b7e7
to
82852d7
Compare
Rebased ⚾ Actions failures unrelated, but looking into a fix. |
82852d7
to
0137459
Compare
Add label parameter to PublishTransaction in WalletController interface. A labels package is added to store generic labels that are used for the different types of transactions that are published by lnd. To keep commit size down, the two endpoints that require a label parameter be passed down have a todo added, which will be removed in subsequent commits.
Add a label parameter to the WalletController SendOutputs endpoint and update rpcs that use it to allow optional provision of labels.
Add a label to our sweep all coins itest and check that it is correctly set.
0137459
to
d021eae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💫
This PR adds optional user provided labels to sends initiated via lnrpc/lncli, as described in #2597.
As is, other subsystems that use the same wallet endpoints simply send empty labels. A potential followup for this PR would be to set generic labels for lnd-published txns.
This PR depends on btcsuite/btcwallet#696, so it has a temporary commit which updates version to point to my fork to btcwalet which has the changes merged.
Tests TBD, just a POC to make sure the btcwallet PR has the necessary changes.