Skip to content

Conversation

mlerner
Copy link
Contributor

@mlerner mlerner commented Nov 17, 2017

This fulfills: #416

I've broken this down into three commits:

  1. Update the proto definitions to include destination addresses.
  2. Implementation - Take the outputs controlled by the wallet, decode the transaction, and get the addresses that correspond to that transaction outputs pkscript. These destination addresses are then passed to the rpcserver. Additionally, rpcserver.go was updated to include the destination addresses in the TransactionDetails response.
  3. Add test that checks whether the addresses stored in the wire TxOut are equal to the destination addresses.

@mlerner mlerner force-pushed the ml/416 branch 12 times, most recently from 8a5873c to b3cbe2f Compare November 23, 2017 08:05
Copy link
Member

@Roasbeef Roasbeef 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 PR!

The diffs look pretty good for the most part, I especially like the decomposition of the the commits themselves. None of my comments are particularly major, so it shouldn't take long for us to get this in.

lnrpc/rpc.proto Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Naming suggestion to keep things short: dest_addrs.

Copy link
Member

Choose a reason for hiding this comment

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

The version isn't necessarily 1 (new updates use version 2 to take advantage of CSV ,etc) . Instead, you can just initialize an empty transaction like so:

wireTx := &wire.MsgTx{}

Copy link
Member

Choose a reason for hiding this comment

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

Similar comment here w.r.t to just creating an empty transaction (though I realize now that my original code made this mistake as well).

Copy link
Member

Choose a reason for hiding this comment

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

This could use a comment explaining that the mapping is from: blockHash -> transactionHash -> transactionOutputs.

Copy link
Member

Choose a reason for hiding this comment

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

We typically try on a best-effort basis to wrap to 80 characters. This diff has a few offenders.

Copy link
Member

Choose a reason for hiding this comment

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

Rather than manually comparing, you can just do reflect.DeepEqual.

@mlerner mlerner force-pushed the ml/416 branch 4 times, most recently from 24730df to a2002da Compare December 1, 2017 04:32
@mlerner
Copy link
Contributor Author

mlerner commented Dec 1, 2017

@Roasbeef: updated with your feedback.

@Roasbeef
Copy link
Member

Roasbeef commented Dec 3, 2017

Can you rebase this, and regenerate the proto files? Thanks, they're currently conflicting as a PR went in recently which also modified the proto files.

@mlerner
Copy link
Contributor Author

mlerner commented Dec 4, 2017

@Roasbeef: updated.

@mlerner
Copy link
Contributor Author

mlerner commented Dec 6, 2017

Rebased again.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐳

@Roasbeef Roasbeef merged commit 231ed5b into lightningnetwork:master Dec 8, 2017
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.

2 participants