Skip to content
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

Rename feeThresholdSat to maxFeeFlatSat #2079

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Rename feeThresholdSat to maxFeeFlatSat #2079

merged 2 commits into from
Nov 29, 2021

Conversation

thomash-acinq
Copy link
Member

The new name should be much easier to understand and matches the name used in eclair-core

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2021

Codecov Report

Merging #2079 (02ea55b) into master (59b4035) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2079   +/-   ##
=======================================
  Coverage   87.57%   87.57%           
=======================================
  Files         166      166           
  Lines       12717    12717           
  Branches      536      536           
=======================================
  Hits        11137    11137           
  Misses       1580     1580           
Impacted Files Coverage Δ
...scala/fr/acinq/eclair/crypto/WeakEntropyPool.scala 89.47% <0.00%> (-5.27%) ⬇️
...ala/fr/acinq/eclair/balance/ChannelsListener.scala 93.10% <0.00%> (-3.45%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 87.86% <0.00%> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 97.56% <0.00%> (+1.62%) ⬆️

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, this name was really confusing.

We are losing the unit with your proposal though, it's not obvious for most users whether we'll expect satoshis or millisatoshis here. It's not very elegant, but we've chosen for now to include the units in the API parameter names, I think it's a good idea to keep that, so what about one of these:

  • maxFeeSat / maxFeePct
  • maxFeeSatoshis / maxFeePct
  • maxFeeFlatSat / maxFeePct
  • maxFeeFlatSatoshis / maxFeePct

@thomash-acinq thomash-acinq changed the title Rename feeThresholdSat to maxFeeFlat Rename feeThresholdSat to maxFeeFlatSat Nov 29, 2021
@thomash-acinq
Copy link
Member Author

You're right, I just copied the name that we have in the code but there we have types to encode the unit.

@thomash-acinq thomash-acinq merged commit 9792c72 into master Nov 29, 2021
@thomash-acinq thomash-acinq deleted the debug-fiatjaf branch November 29, 2021 16:51
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 this pull request may close these issues.

3 participants