Rename Htlc::to_bitcoin_amount to satoshi_amount

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408

[jirijakes]

Per #3063 (comment) .

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (e5b7402) to head (ae59eec).
⚠️ Report is 3606 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3099 +/- ##
==========================================
+ Coverage 89.86% 89.96% +0.09% 
==========================================
Files 119 119 Lines 97507 98177 +670 Branches 97507 98177 +670 ==========================================
+ Hits 87629 88324 +695 + Misses 7307 7291 -16 + Partials 2571 2562 -9 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tnull]

Generally looks good, although tbh. I find satoshi_amount also a bit confusing as the Amount type's point is exactly that is independent of a denomination.

Should the method maybe be just be named fn amount(&self) -> Amount or fn as_amount(&self) -> Amount?

(cc @TheBlueMatt as he made the original comment)

[jirijakes]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

[tnull]

@tnull , I called it amount at first (#3063 (comment)) but I agree with Matt that it might be confusing, given that amount of HTLC has diferent meaning.

The 'satoshi' part should invoke that there will be some rounding happening.

Ah, I had forgotten about that. Given that it really just performs integer division, should we be explicit here and call it floor_amount or amount_floor, similar to, e.g., u32::div_floor?

[TheBlueMatt]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

[TheBlueMatt]

I believe it's soon enough after the method was first introduced that renaming it should not have big negative impact, if any.

Its not in a release, so there's no impact :)

[tnull]

Naming is hard :). I'm a bit meh on floor_amount because it doesn't mention the unit being floor'd to, which I think substantially improves readability of callsites.

So floor_sats_amount or amount_floor_sats?

[TheBlueMatt]

That would be good with me, or amount_sats_floor or any other permutation :)

[G8XSU]

amount_sats :)

[TheBlueMatt]

Closing as supersceded by #4408