[RFC] rename pay_parameters for no amount invoice #3397
Conversation
|
Makes sense to me. |
lightning/src/ln/bolt11_payment.rs
Outdated
| @@ -28,7 +28,7 @@ use crate::types::payment::PaymentHash; | |||
| /// | |||
| /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment | |||
| /// [`ChannelManager::send_preflight_probes`]: crate::ln::channelmanager::ChannelManager::send_preflight_probes | |||
| pub fn payment_parameters_from_zero_amount_invoice( | |||
| pub fn payment_parameters_from_any_amount_invoice( | |||
There was a problem hiding this comment.
nit: Could you then also change the docs above to say any-amount (also known as 'zero-amount')?
Btw, in LDK Node we used 'variable amount' terminology everywhere (which I honestly slightly prefer, but 'any' is fine, too).
There was a problem hiding this comment.
Good catch, now the new naming convention should be reflected.
Btw, in LDK Node we used 'variable amount' terminology everywhere (which I honestly slightly prefer, but 'any' is fine, too).
https://wasp-lang.dev/img/on-importance-of-naming-in-programming/naming-banish-thee-white.png
if you have a strong preference I will change, probably my brain is familiar with the cln convention, but happy to change if we have to (or if reflect better the reading of the code inside the project)
There was a problem hiding this comment.
if you have a strong preference I will change
Well I don't have a super strong preference, but if we change it here I'll probably end up aligning it in LDK Node in a follow-up to reduce potential confusion. 🤷♂️
Hence why I mentioned it in #2879.
There was a problem hiding this comment.
I read #2879 (comment) and sorry If I did not give the correct weight.
Looks like you did a lot of work to avoid confusion with this, so probably moving to a variable amount is a good start. Let me do that right away now
There was a problem hiding this comment.
Looks like you did a lot of work to avoid confusion with this, so probably moving to a variable amount is a good start. Let me do that right away now
Really not 'a lot of work', fine to go either way.
There was a problem hiding this comment.
No problem, any invoice is a cln convention and also if my giustification of use "any amount" was legit, I think we should preferer keep the same convention inside the repository of the same kind.
Should be fixed now in 69af73e
There was a problem hiding this comment.
ops I updated the commit but not the code :) so commit is c2c670b
vincenzopalazzo
force-pushed
the
macros/amount-less-refactoring
branch
from
6d641cf to
69af73e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3397 +/- ##
==========================================
+ Coverage 89.56% 89.64% +0.07%
==========================================
Files 127 128 +1
Lines 103547 104887 +1340
Branches 103547 104887 +1340
==========================================
+ Hits 92744 94023 +1279
- Misses 8094 8147 +53
- Partials 2709 2717 +8 ☔ View full report in Codecov by Sentry. |
vincenzopalazzo
force-pushed
the
macros/amount-less-refactoring
branch
from
69af73e to
a7cb0f5
Compare
vincenzopalazzo
force-pushed
the
macros/amount-less-refactoring
branch
2 times, most recently
from
fc1e87a to
65056ee
Compare
This commit renames the function `pay_parameters_for_zero_amount_invoice` to `pay_parameters_for_variable_amount_invoice`. The term "variable amount" is used to align with the naming convention in the LDK node, helping to avoid confusion between similar packages. Fixes: lightningdevkit#2879 Replaces: lightningdevkit#2979 Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
vincenzopalazzo
force-pushed
the
macros/amount-less-refactoring
branch
from
65056ee to
c2c670b
Compare
I am going through some issue that are without an owner and see if there is any handing fruit around them before opening other PR to create more work for the reviewers.
So this PR is simply fixing #2879 by proposing a new name "any amount" for the invoice with no amount. The reason for this is that from the spec perspective is correct to say that the invoice has no amount [1] but from a user perspective IMHO is more correct to say that theinvoice is for any amountand there is not a restriction on the value sent.Let me know if you do not like this version and if you want this PR that reiterates to the terminology used in #2979.
For more details, see the commit.
[1] https://github.com/lightning/bolts/blob/247e83d528a2a380e533e89f31918d7b0ce6a0c1/11-payment-encoding.md#human-readable-part
Fixes: #2879