Skip to content
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

Adds helper function lnrpc.RPCTransaction to create a single lnrpc.Transaction and refactors lnrpc.RPCTransactionDetails #5854

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

bjarnemagnussen
Copy link
Contributor

@bjarnemagnussen bjarnemagnussen commented Oct 12, 2021

Description

Currently inside SubscribeTransactions the streamed lnrpc.Transaction structure does not get its Label field populated, which seems to be an oversight.

This PR adds a helper function lnrpc.RPCTransaction to return a single lnrpc.Transaction and refactors the helper function lnrpc.RPCTransactionDetails. This is alos used inside SubscribeTransactions to make sure the Label field is correctly populated and have the creation of a RPC Transaction in one place.

Fixes #5853

@bjarnemagnussen bjarnemagnussen changed the title Add lnrpc.RPCTransaction to create a single lnrpc.Trnsaction and refactor lnrpc.RPCTransactionDetails Adds helper function lnrpc.RPCTransaction to create a single lnrpc.Transaction and refactors lnrpc.RPCTransactionDetails Oct 12, 2021
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!
LGTM, just two nits 🎉

lnrpc/rpc_utils.go Outdated Show resolved Hide resolved
lnrpc/rpc_utils.go Outdated Show resolved Hide resolved
@bjarnemagnussen bjarnemagnussen force-pushed the upstream/RPCTransaction branch from b894eab to 4ff2b07 Compare October 14, 2021 06:41
@bjarnemagnussen
Copy link
Contributor Author

Thanks @guggero for your quick review! I have addresses your points!

@carlaKC carlaKC self-requested a review November 29, 2021 11:29
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@guggero guggero added this to the v0.14.2 milestone Nov 30, 2021
@guggero
Copy link
Collaborator

guggero commented Nov 30, 2021

Can you please add this to the 0.14.2 release notes, so we can get this merged? Thanks.

@bjarnemagnussen
Copy link
Contributor Author

Thanks! I have rebased and added release notes to 0.14.2

@Roasbeef
Copy link
Member

Roasbeef commented Dec 9, 2021

@bjarnemagnussen can you rebase again? The UI shows there's a merge conflict, but then doesn't actually say what it is....

@bjarnemagnussen
Copy link
Contributor Author

@bjarnemagnussen can you rebase again?

Done! Let me know if is still causing some issues

@guggero guggero merged commit 66a669d into lightningnetwork:master Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC SubscribeTransactions does not have the Label field populated
4 participants