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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rpc] Add funding allocation map to listpeers command #2232

Merged
merged 2 commits into from Jan 16, 2019

Conversation

Projects
None yet
2 participants
@conscott
Copy link
Collaborator

conscott commented Jan 8, 2019

A redo of #2207

The point is the know the initial state of funding allocated to the channel by the funder / fundee. At the moment there are only one-sided channel opens, but dual-funded channels will be available in the near future 馃

This change also implicitly let's you know who initiated the channel open, which was the original intent of #2207

@conscott conscott force-pushed the conscott:add_funder_to_listpeers branch 3 times, most recently from acbff2f to 3f9e859 Jan 8, 2019

@niftynei
Copy link
Collaborator

niftynei left a comment

Can you expand a bit on the intended use case of these numbers?

if (channel->funder == LOCAL) {
json_add_u64(response, pubkey_to_hexstr(tmpctx, &p->id), 0);
json_add_u64(response, pubkey_to_hexstr(tmpctx, &ld->id),
channel->funding_satoshi * 1000);

This comment has been minimized.

@niftynei

niftynei Jan 8, 2019

Collaborator

What's the rationale for multiplying this by 1000? funding_satoshi should be a satoshi value already.

Realized that you're doing this as msats.

@conscott

This comment has been minimized.

Copy link
Collaborator Author

conscott commented Jan 9, 2019

Sure @niftynei !

The original intent of #2207 was to allow you to see if the the channel open was inbound or outbound, basically to know who funded the channel. lnd has a similar field in listpeers.

Christian then mentioned that dual funded channels are coming soon, so why not just have a map of channels id's -> initial channel state funding (what I have done here). From this information we can implicitly know who opened the channel, but also in the future know if it was dual-funded and what the initial state was.

I think this useful because it can allow you to see more rich stats on the 'progress' of your channel usage since it has been opened, along with msatoshi_to_us_min and msatoshi_to_us_max

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Jan 10, 2019

Cool yeah I can see how that would be useful for metric tracking.

Could you add an entry to the CHANGELOG.md file and update the listpeers manpage to include a short blurb about this added fieldset?

@conscott conscott force-pushed the conscott:add_funder_to_listpeers branch from 3f9e859 to 31699f2 Jan 11, 2019

@conscott

This comment has been minimized.

Copy link
Collaborator Author

conscott commented Jan 11, 2019

Rebased on top of #2248 to get the CHANGELOG.md for next release (should wait to merge this until that one is through).

@niftynei : I added note to the CHANGELOG 31699f2, however for the listpeers manpage, we currently don't describe anything in the channel field, so I am thinking we could open a separate issue and make a note to have a PR to fully describe all these fields, instead of the one I just added.

What do you think?

@conscott conscott force-pushed the conscott:add_funder_to_listpeers branch from 31699f2 to 2442e95 Jan 12, 2019

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Jan 15, 2019

i'll take it lol

ACK 2442e95

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Jan 15, 2019

fwiw #1360 has a todo for the listpeers manpage.

@conscott conscott force-pushed the conscott:add_funder_to_listpeers branch from 2442e95 to aa7a051 Jan 16, 2019

@conscott

This comment has been minimized.

Copy link
Collaborator Author

conscott commented Jan 16, 2019

Rebased.

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Jan 16, 2019

ACK aa7a051

@niftynei niftynei merged commit 5fd1760 into ElementsProject:master Jan 16, 2019

2 checks passed

ackbot PR ack'd by niftynei
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment