Skip to content

Use a separate (non-trait) fee-estimation fn in LowerBoundedEstimator #1611

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

Conversation

TheBlueMatt
Copy link
Collaborator

This should make it somewhat more difficult to accidentally use a
straight fee estimator when we actually want a
LowerBoundedFeeEstimator by not having the types be exchangeable at
all.

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #1611 (af7e9b6) into main (2a3bf03) will decrease coverage by 0.09%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##             main    #1611      +/-   ##
==========================================
- Coverage   90.87%   90.77%   -0.10%     
==========================================
  Files          80       80              
  Lines       44569    44651      +82     
  Branches    44569    44651      +82     
==========================================
+ Hits        40500    40530      +30     
- Misses       4069     4121      +52     
Impacted Files Coverage Δ
lightning/src/chain/package.rs 93.04% <33.33%> (ø)
lightning/src/chain/chaininterface.rs 95.65% <100.00%> (-0.51%) ⬇️
lightning/src/chain/chainmonitor.rs 97.96% <100.00%> (ø)
lightning/src/chain/channelmonitor.rs 91.02% <100.00%> (+0.06%) ⬆️
lightning/src/ln/channel.rs 88.78% <100.00%> (+0.02%) ⬆️
lightning/src/ln/channelmanager.rs 85.11% <100.00%> (+0.02%) ⬆️
lightning/src/ln/functional_test_utils.rs 93.57% <0.00%> (-1.67%) ⬇️
lightning/src/routing/gossip.rs 91.32% <0.00%> (-0.62%) ⬇️
lightning/src/ln/payment_tests.rs 98.57% <0.00%> (-0.32%) ⬇️
lightning/src/ln/functional_tests.rs 96.79% <0.00%> (-0.32%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a3bf03...af7e9b6. Read the comment docs.

wpaulino
wpaulino previously approved these changes Jul 13, 2022
dunxen
dunxen previously approved these changes Jul 15, 2022
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Ah, yes! Thanks for the catch. This was required before changing the Channel method signatures.

LGTM

@TheBlueMatt TheBlueMatt added this to the 0.0.110 milestone Jul 21, 2022
@TheBlueMatt TheBlueMatt dismissed stale reviews from dunxen and wpaulino via 9d08b60 July 21, 2022 21:39
@TheBlueMatt
Copy link
Collaborator Author

Pushed an extra commit that is gonna be required for the bindings, but is rather simple.

@dunxen
Copy link
Contributor

dunxen commented Jul 22, 2022

The one fuzz just needs the reference to the estimator changed, otherwise looks good.

@TheBlueMatt TheBlueMatt force-pushed the 2022-07-lower-bounded-estimator-nit branch from 9d08b60 to 5f28d17 Compare July 22, 2022 15:33
@TheBlueMatt
Copy link
Collaborator Author

Oops, indeed, thanks.

dunxen
dunxen previously approved these changes Jul 22, 2022
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

ACK 5f28d17

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

ACK after outstanding feedback

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

ACK after squash

@tnull
Copy link
Contributor

tnull commented Jul 25, 2022

LGTM, feel free to squash.

This should make it somewhat more difficult to accidentally use a
straight fee estimator when we actually want a
LowerBoundedFeeEstimator by not having the types be exchangeable at
all.
This simplifies things for bindings (and, to some extent,
downstream users) by exploiting the fact that we can always "clone"
a reference to a struct by dereferencing and then creating a new
reference.
This change the method name on `LowerBoundedFeeEstimator` to
further differentiate it from the generic `FeeEstimator` trait.
@TheBlueMatt TheBlueMatt force-pushed the 2022-07-lower-bounded-estimator-nit branch from 3c5d7da to af7e9b6 Compare July 25, 2022 18:33
@TheBlueMatt
Copy link
Collaborator Author

Rebased without further changes.

@TheBlueMatt
Copy link
Collaborator Author

Go home Github, you're drunk (the missing required job did run, and passed...but is still listed as pending run...).

@TheBlueMatt TheBlueMatt merged commit d8cca98 into lightningdevkit:main Jul 25, 2022
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.

6 participants