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

Consider dropping unwrap_or(1.0) from convert_fee_rate #80

Open
tnull opened this issue Feb 10, 2024 · 5 comments
Open

Consider dropping unwrap_or(1.0) from convert_fee_rate #80

tnull opened this issue Feb 10, 2024 · 5 comments

Comments

@tnull
Copy link
Contributor

tnull commented Feb 10, 2024

Currently, when something goes wrong/the corresponding pair can't be found, convert_fee_rate will default to a bogus 1.0 fee rate:

pairs
.into_iter()
.find(|(k, _)| k <= &target)
.map(|(_, v)| v)
.unwrap_or(1.0)

This however messes with users' assumptions they depend on receiving a correct fee update (e.g. in Lightning). Rather than returning a (btw. undocumented) default value, convert_fee_rate should just error out if something goes wrong, as it's fallible anyways.

@tnull tnull changed the title Drop unwrap_or(1.0) from convert_fee_rate` Drop unwrap_or(1.0) from convert_fee_rate Feb 10, 2024
@tnull
Copy link
Contributor Author

tnull commented Feb 10, 2024

Mhh, one caveat seems to be that Esplora on Signet seems to return {} for fee-estimates. However, arguably falling back to 1 sat/vbyte should still be part of the user-side logic, i.e., a conscious decision?

@tnull tnull changed the title Drop unwrap_or(1.0) from convert_fee_rate Consider dropping unwrap_or(1.0) from convert_fee_rate Feb 10, 2024
@notmandatory
Copy link
Member

I agree this shouldn't default to 1 sat/vB, it should throw an error so the caller can decide if they want to retry or default to some value.

@tnull
Copy link
Contributor Author

tnull commented Mar 21, 2024

Yes, I also agree that this would be the preferable behavior, although want to note that changing it to return an error will let user's fee estimation on Signet fail where it used to work nicely before. So if the change is introduced it would be nice to feature it prominently in the release notes to make users aware of the fix. It might also be worth breaking the API for this to force users to double-check their code.

@ValuedMammal
Copy link

Considering that producing a value is contingent on find, perhaps returning Option<f32> would be appropriate.

@notmandatory
Copy link
Member

Returning None instead of an Err makes sense to me. The logic being that you asked for the fee rate for a given target and one didn't exist in the values returned by the esplora server. Then the caller can decide to use some default or not.

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

No branches or pull requests

3 participants