-
Notifications
You must be signed in to change notification settings - Fork 105
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
[0.6.2] btc: more sensible bond fee buffer #2392
Conversation
683cfc0
to
8fcfc67
Compare
38a5258
to
1619a37
Compare
When used for computing the bond fee buffer, the "fee rate limit" makes an extremely hight fee rate buffer, which makes BTC bonds impractical. This change: - switches to using a dynamic fee rate with the configured fallback rate for both ReserveBondFunds and BondsFeeBuffer, where the former is used as the requirement for the frontend to proceed with registration - uses the exported BondsFeeBuffer method when automatically creating the "extra" split output in certain order funding edge cases instead of a buffer based on the high limit. - tweaks the bond tx input count to be slightly less conservative, going from 12 -> 8 possible inputs - gives BondsFeeBuffer a fee rate parameter so that a rate can come from Core even if the wallet has no estimate source - give ReserveBondFunds a feeBuffer paramerter so that the buffer used during wallet funding (from BondsFeeBuffer) can be used, preventing possible bad UX if the fee rates go up and they have to send more. The motivation for these changes is that using the wallet's configure fee rate limit create extremely high fee bufferes, whereas we have defined the "fallback" fee rate to be a reasonable fallback where a current estimate is not available. Further, this is a buffer and precision is not critical, but turning users away from onboarding is a major issue. The right approach is to base reserves on a padded estimate based on current rates, not the swap fee rate limit. Finally, given this softening of the reserves amount, no adjustment to the enforced bond reserves in Reconfigure is done since we are now using current rates as a hit of future rates. Padding of this fee buffer already comes from: - planning for a lot of inputs - 4 parallel bond tracks - the exported BondsFeeBuffer method doubling the provided fee rate Even if fee rates skyrocket, and the reserves become insufficient for bond renewal, the user will still post bond if their balance allows, and they may deposit some nominal additional amount to cover any such deficit.
1619a37
to
8418664
Compare
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.
Looks good.
if (wallet.synced && wallet.balance.available > bondAsset.amount) { | ||
const bondsFeeBuffer = await this.getBondsFeeBuffer(assetID, page.regAssetForm) | ||
if (wallet.synced && wallet.balance.available > 2 * bondAsset.amount + bondsFeeBuffer) { |
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.
Nice! I had just noticed this while doing some other work.
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.
Yeah, I totally missed this condition when I was rushing through #2200
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.
Oh, the same thing exists on settings.ts. Fixing that in another PR. At least the first DEX uses the right info.
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.
The UI for bonds in settings is pretty rough. I was thinking about opening an issue. Would be good to see the bond sizes, fee buffer, durations, USD conversions, etc.
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.
100%
// value for up to 45 minutes from the last request for a given asset. A web | ||
// app could conceivably do the same, but we'll do this here between the | ||
// backend (Core) and UI so that a webapp does not need to employ local | ||
// storage/cookies and associated caching logic. |
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.
My thinking was initially to use front end logic too, but the webserver cache is probably better. We are leaving rpcserver hanging for now.
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.
Correct re: the rpcserver. However, changing responses from BondsFeeBuffer doesn't really matter for rpc since an rpc user would just record the value they obtained on their first call to BondsFeeBuffer and pass that on to PostBond, whereas on the frontend there's the possibility to make the UI fetch a new value and you've lost the original (and the walletWaitForm won't proceed)
#2382 for v0.6.2
Rebased on #2391 so SPV wallets are
FeeRater
s, which gives(*Core).feeSuggestionAny
access to the wallet's fee rate first.