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

ad.fee_estimation_fixes #18262

Merged
merged 1 commit into from
Jul 8, 2024
Merged

ad.fee_estimation_fixes #18262

merged 1 commit into from
Jul 8, 2024

Conversation

almogdepaz
Copy link
Contributor

@almogdepaz almogdepaz commented Jul 1, 2024

Purpose:

fixes conversion error in fee calculation
fix steady fee pressure test

Current Behavior:

when the tracker is able to find a passing bucket we divide by 1000 before returning the result currently we don't do that when we don't find a bucket and use the median

New Behavior:

divide median by 1000 on return

Testing Notes:

fixed the steady fee pressure test to make sure we get results

@almogdepaz almogdepaz self-assigned this Jul 1, 2024
@almogdepaz almogdepaz added bug Something isn't working Wallet Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog labels Jul 1, 2024
@almogdepaz almogdepaz requested review from aqk and emlowe July 1, 2024 13:25
@almogdepaz almogdepaz marked this pull request as ready for review July 1, 2024 16:01
@almogdepaz almogdepaz requested a review from a team as a code owner July 1, 2024 16:01
Copy link

coveralls-official bot commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9744990518

Details

  • 14 of 14 (100.0%) changed or added relevant lines in 2 files are covered.
  • 45 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.03%) to 90.965%

Files with Coverage Reduction New Missed Lines %
chia/server/node_discovery.py 1 79.79%
chia/daemon/client.py 1 73.94%
chia/_tests/core/util/test_file_keyring_synchronization.py 1 97.1%
chia/data_layer/data_layer.py 2 85.63%
chia/_tests/core/test_farmer_harvester_rpc.py 2 98.02%
chia/full_node/full_node.py 5 86.06%
chia/wallet/wallet_node.py 6 88.43%
chia/_tests/core/util/test_lockfile.py 27 77.73%
Totals Coverage Status
Change from base Build 9719381064: -0.03%
Covered Lines: 101654
Relevant Lines: 111700

💛 - Coveralls

Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good. I appreciate the improvement in the test by not starting at 0. I wonder if it would be relevant to also cover the case where we start from block 0. What's not obvious to me is why we ever divide by 1000, but perhaps that's a mystery carried over from the bitcoin implementation. Also, it seems generally risky to return a float and have -1.0 as a special case (since comparing exact floats is gnarly). an Optional[float] would probably make more sense.

@Starttoaster Starttoaster merged commit 3cecc27 into main Jul 8, 2024
372 of 373 checks passed
@Starttoaster Starttoaster deleted the fee_estimation_fix branch July 8, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog Wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants