Skip to content

channel_reserve_satoshis variable names confusing #181

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

Closed
yuntai opened this issue Sep 14, 2018 · 4 comments · Fixed by #613
Closed

channel_reserve_satoshis variable names confusing #181

yuntai opened this issue Sep 14, 2018 · 4 comments · Fixed by #613
Assignees

Comments

@yuntai
Copy link
Contributor

yuntai commented Sep 14, 2018

In the spec,

channel_reserve_satoshis is the minimum amount that the other node is to keep as a direct payment.

And, as my understanding, this means it's the value that the node receiving Open/AcceptChannel should keep.

We do channel.thier_channel_reserve_satoshis = msg.channel_reserve_satoshis
So channel.their_channel_reserve_satoshis is used when checking conditions for our channel reserve and I found it pretty confusing.

Perhaps,
channel.our_channel_reserve_satoshis = msg.channel_reserve_satoshis
and
Channel::get_their_channel_reserve_satoshis is better?

@TheBlueMatt
Copy link
Collaborator

Hmm, I don't think flipping it on its head is any more clear, it was the value provided by them ie its not our own setting, but, indeed, we have to have it towards us. We could rename it something like min_channel_reserve_to_self to make that more clear?

@yuntai
Copy link
Contributor Author

yuntai commented Sep 25, 2018

Now I am not confused as you clearly stated, and agree that flipping wouldn't make any clear. I added some comments in #189 and think that would suffice to prevent confusion.

@yuntai yuntai closed this as completed Oct 5, 2018
@valentinewallace valentinewallace self-assigned this May 1, 2020
@valentinewallace
Copy link
Contributor

valentinewallace commented May 1, 2020

Reopening because I'd agree these names are confusing, as pointed out as well by @ariard here.

I think it's effectively "our setting," so would be fine to just flip it, but since people seem to disagree I'm open to something along the lines of remote_channel_reserve_by_us as suggested by Antoine in the linked comment, or min_channel_reserve_to_self

@jkczyz
Copy link
Contributor

jkczyz commented May 1, 2020

Reopening because I'd agree these names are confusing, as pointed out as well by @ariard here.

I think it's effectively "our setting," so would be fine to just flip it, but since people seem to disagree I'll go with something along the lines of remote_channel_reserve_by_us as suggested by Antoine in the linked comment, or min_channel_reserve_to_self

My two sats:

Who is doing the setting is irrelevant once the setting is in place and is thus extraneous information. In the context of a channel, all that matters is what side the reserve applies to. IMHO, "local" and "remote" seem descriptive enough in this case. However, I haven't put much thought into whether there are more suitable names for this concept.

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 a pull request may close this issue.

4 participants