search

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

Consider dropping `unwrap_or(1.0)` from `convert_fee_rate` #80

Closed tnull closed 15 hours ago

tnull commented 6 months ago

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

https://github.com/bitcoindevkit/rust-esplora-client/blob/7faab652bbf5897e0a752354deabedffd22d08aa/src/lib.rs#L90-L94

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 commented 6 months ago

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?

notmandatory commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

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

notmandatory commented 2 months ago

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.

oleonardolima commented 2 months ago

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?

Indeed it's weird, but it seems to work for testnet and it works for signet when using mempool.space API instead.

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

I also agree with returning an Option<f32>, using None instead of an Err, and make it a breaking change for the reasons as tnull mentioned.