-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix inbound_liquidity_msats
#1062
Fix inbound_liquidity_msats
#1062
Conversation
515f632
to
25b7b4e
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.
Looks good, added just a NIT on comments.
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.
I just have one nit, looking good!
13913a3
to
ee25097
Compare
@@ -187,6 +187,7 @@ dictionary NodeState { | |||
u64 max_single_payment_amount_msat; | |||
u64 max_chan_reserve_msats; | |||
sequence<string> connected_peers; | |||
u64 max_receivable_single_payment_amount_msat; |
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.
Now that we add this field the former one (inbound_liquidity_msats) changes its semantic.
My concern is that developers will upgrade and their logic will break without them knowing about it.
Can we rename the inbound_liquidity_msats
so developers will get a compilation error on upgrade which will force them to change the code?
We can also leave it with the old semantic (and mark it as deprecated) and add another field (total_inbound_liquidity_msat) with the unified liquidity.
Anything that will not let this change go under the radar.
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.
That's a very good point @roeierez. I'll rename it to total_inbound_liquidity_msat
.
8e43501
to
544c928
Compare
544c928
to
c51d212
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!
Addresses #1018