Don't return 1.0 feerate if none is found by `convert_fee_rate`

bitcoindevkit / rust-esplora-client

Bitcoin Esplora API client library. Supports plaintext, TLS and Onion servers. Blocking or async.

MIT License

28 stars 44 forks source link

Don't return 1.0 feerate if none is found by `convert_fee_rate` #90

Closed ValuedMammal closed 3 weeks ago

ValuedMammal commented 2 months ago

Change convert_fee_rate to return Option<f32> instead of Result.

PR #65 made this function effectively infallible by removing the parse int error while falling back to a bogus 1.0 feerate if a value isn't found in fee estimates. We could make it return an error if for example the given fee estimates map is empty without changing the function signature but in that case we would need a new Error variant making it a breaking change anyway, therefore just change the function to return Option which should only be None if given a target of 0 or an empty map assuming esplora always has a fee estimate for a target of 1 confirmation.

fixes #80

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9628513362

Details

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 87.49%

Totals
Change from base Build 9558989002:	-0.04%
Covered Lines:	1056
Relevant Lines:	1207

💛 - Coveralls

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9635372336

Details

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.05%) to 87.479%

Totals
Change from base Build 9558989002:	-0.05%
Covered Lines:	1055
Relevant Lines:	1206

💛 - Coveralls

notmandatory commented 3 weeks ago

@ValuedMammal this looks good but could you add a simple test as above but also that hits this new None case?

ValuedMammal commented 3 weeks ago

I added to the existing feerate_parsing test which tests the logic in convert_fee_rate. I'm not sure if you're suggesting to add another test? @notmandatory @oleonardolima

notmandatory commented 3 weeks ago

I added to the existing feerate_parsing test which tests the logic in convert_fee_rate. I'm not sure if you're suggesting to add another test? @notmandatory @oleonardolima

Oh sorry I didn't look closely enough at the change you made to that test, that covers it.

oleonardolima commented 3 weeks ago

I added to the existing feerate_parsing test which tests the logic in convert_fee_rate. I'm not sure if you're suggesting to add another test? @notmandatory @oleonardolima

Awesome, thanks! You covered the simple test I had in mind in ec5ee82

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 10513190278

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 87.531%

Totals
Change from base Build 9558989002:	0.0%
Covered Lines:	1060
Relevant Lines:	1211