Skip to content

Conversation

@ValuedMammal
Copy link
Contributor

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
Copy link

coveralls commented Jun 22, 2024

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 Coverage Status
Change from base Build 9558989002: -0.04%
Covered Lines: 1056
Relevant Lines: 1207

💛 - Coveralls

@ValuedMammal ValuedMammal force-pushed the fix/convert-fee-rate branch from 2acce7e to ed219e2 Compare June 23, 2024 17:55
@coveralls
Copy link

coveralls commented Jun 23, 2024

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 Coverage Status
Change from base Build 9558989002: -0.05%
Covered Lines: 1055
Relevant Lines: 1206

💛 - Coveralls

Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK ed219e2
I think you could add a simple test for it, but fine without it too.

#[test]
fn feerate_parsing() {
let esplora_fees = serde_json::from_str::<HashMap<u16, f64>>(
r#"{
"25": 1.015,
"5": 2.3280000000000003,
"12": 2.0109999999999997,
"15": 1.018,
"17": 1.018,
"11": 2.0109999999999997,
"3": 3.01,
"2": 4.9830000000000005,
"6": 2.2359999999999998,
"21": 1.018,
"13": 1.081,
"7": 2.2359999999999998,
"8": 2.2359999999999998,
"16": 1.018,
"20": 1.018,
"22": 1.017,
"23": 1.017,
"504": 1,
"9": 2.2359999999999998,
"14": 1.018,
"10": 2.0109999999999997,
"24": 1.017,
"1008": 1,
"1": 4.9830000000000005,
"4": 2.3280000000000003,
"19": 1.018,
"144": 1,
"18": 1.018
}
"#,
)
.unwrap();
assert_eq!(convert_fee_rate(6, esplora_fees.clone()).unwrap(), 2.236);
assert_eq!(
convert_fee_rate(26, esplora_fees).unwrap(),
1.015,
"should inherit from value for 25"
);
}

@notmandatory
Copy link
Member

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

@ValuedMammal
Copy link
Contributor Author

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

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK ec5ee82

@notmandatory
Copy link
Member

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.

@notmandatory notmandatory merged commit d008b9b into bitcoindevkit:master Aug 22, 2024
@oleonardolima
Copy link
Collaborator

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
Copy link

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.

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 Coverage Status
Change from base Build 9558989002: 0.0%
Covered Lines: 1060
Relevant Lines: 1211

💛 - Coveralls

@ValuedMammal ValuedMammal deleted the fix/convert-fee-rate branch August 27, 2024 04:31
chrono-raven6i90 added a commit to chrono-raven6i90/rust-esplora-client that referenced this pull request Oct 28, 2025
…if none is found by `convert_fee_rate`

ec5ee8233011269b2d0bbdf8a1647e27e61b9050 test: improve test `feerate_parsing` (valued mammal)
ed219e29455a9d38487f50d3862888b73cdb211c fix: don't return default 1.0 feerate if not found by `convert_fee_rate` (valued mammal)

Pull request description:

  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

ACKs for top commit:
  notmandatory:
    ACK ec5ee8233011269b2d0bbdf8a1647e27e61b9050

Tree-SHA512: 32fd2a8a57bcc1a6fb8569d779aca8a273c843d38afe6f092f0c70c7dad0ff7b37595284985ca4d3c5e253810b70857600043817fd9f928ee0c706108f8a9bcb
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.

Consider dropping unwrap_or(1.0) from convert_fee_rate

5 participants