-
Notifications
You must be signed in to change notification settings - Fork 92
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
client: refactor btc FeeEstimator and added DOGE external #1967
Conversation
// Default to the BTC RPC estimator (see LTC). Consumers can use | ||
// NoLocalFeeRate or a similar dummy function to power feeRate() requests | ||
// with only an external fee rate source available. Otherwise, all method | ||
// calls must provide a rate or accept the configured fallback. | ||
if w.localFeeRate == nil { | ||
w.localFeeRate = rpcFeeRate | ||
} | ||
|
||
return w, nil | ||
} | ||
|
||
// NoLocalFeeRate is a dummy function for BTCCloneCFG.FeeEstimator for a wallet | ||
// instance that cannot support a local fee rate estimate but has an external | ||
// fee rate source. | ||
func NoLocalFeeRate() (uint64, error) { | ||
return 0, errors.New("no local fee rate estimate possible") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used, but an idea to illustrate what these comments in the constructor are describing.
// override feeRate to avoid unnecessary conversions and btcjson types. | ||
func (btc *ExchangeWalletElectrum) feeRate(_ RawRequester, confTarget uint64) (uint64, error) { | ||
satPerKB, err := btc.ew.wallet.FeeRate(btc.ew.ctx, int64(confTarget)) | ||
func (btc *ExchangeWalletElectrum) walletFeeRate(ctx context.Context, confTarget uint64) (uint64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes were really needed in electrum.go, but I wanted to remove the unused RawRequester junk from the ExchangeWalletElectrum
method, doing the wrapping inside the ElectrumWallet
constructor.
// NOTE: btc.(*baseWallet).feeRate calls the local and external fee estimators | ||
// in sequence, applying the limits configured in baseWallet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the gist of the design in this PR. A wallet choose to set these functions or not. If the local estimator is not set, the BTC RPC default is used. If the external estimator is not set, no external estimate is obtained.
I'm splitballing with the NoLocalFeeRate
dummy idea for the local estimator if an instance needs to be designed that only does external estimates but there is no possibility for local estimates. We can consider this for SPV, but we may not want to implement asset.FeeRater
for SPV wallets since we've established pretty well to use the fee rates from server that we have cached or fetched on-demand in Core
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this makes sense. Left a suggestion but LGTM
func (btc *baseWallet) feeRate(confTarget uint64) (uint64, error) { | ||
// Local estimate first. TODO: Allow only external (nil local). | ||
feeRate, err := btc.localFeeRate(btc.ctx, btc.node, confTarget) // e.g. rpcFeeRate | ||
if err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we also check feeRate != 0
? This way if it returns 0 we can also check an external fee rate if the user wants to
Unrelated to these changes, but confused about an error I'm getting trying this out on testnet:
Current likely-taker order limit is negative? This is trying to sell one doge. |
abb1769
to
a2455df
Compare
Expecting tests to fail. See last commit message. |
5d0c02a
to
3e05442
Compare
Also guard against FeeRate calls while not connected. Co-authored-by: Brian Stafford <[email protected]> Co-authored-by: Jonathan Chappelow <[email protected]>
3e05442
to
95cb28b
Compare
Re-tested now that we're rebased with |
This refactors the BTC
baseWallet
fee rate handling to abstract the internal/external estimate calls in afeeRate
method. TheestimateSmartFee
RPC-specific wallet method is now removed and no longer part of thebtc.Wallet
interface.This replaces #1888. The commit is still authored by @vctt94, but also "co-authored" by myself and @buck54321.
Co-authored-by: Brian Stafford [email protected]
Co-authored-by: Jonathan Chappelow [email protected]