multi: match fee rates and smart fees#505
Merged
Merged
Conversation
chappjc
force-pushed
the
match-fee-rates
branch
5 times, most recently
from
2421a14 to
cdcea26
Compare
This comment has been minimized.
This comment has been minimized.
JoeGruffins
reviewed
Jun 29, 2020
Member
JoeGruffins
left a comment
JoeGruffins
left a comment
There was a problem hiding this comment.
I could only find a few places to comment on the code. Everything makes sense and looks good to me. However, while testing this I continue to run into a panic in dcrd, and so will continue testing once the reasons for that are better understood.
Member
Author
|
decred/dcrd#2232 testnet issues. But I'm afraid to say I've not taken this for a spin yet, so there's likely many things I've neglected. |
JoeGruffins
reviewed
Jun 29, 2020
dnldd
reviewed
Jun 29, 2020
chappjc
commented
Jun 29, 2020
buck54321
requested changes
Jun 29, 2020
Member
buck54321
left a comment
buck54321
left a comment
There was a problem hiding this comment.
First pass. No major hangups. I still need to deep dive on the script size fixes and TestFundEdges stuff.
chappjc
commented
Jun 29, 2020
Comment on lines
+307
to
+308
| optimalRate := float64(optimalFeeRate) * 1e-5 | ||
| // fmt.Println((float64(optimalFeeRate)*1e-5)-0.00024) |
Member
Author
There was a problem hiding this comment.
BTW, the kind of thing that makes be super uncomfortable with floating point math: https://play.golang.org/p/Dhmxmp5OosM
The error is less than 1e-8, so it's going to work out to the same atoms/byte, but it still sucks.
Member
Author
|
@buck54321 Addressed your comments in 0f80a22, 73c00f7, and 00d632a. |
Member
Author
|
Tested on simnet now that it's rebased on master, and it worked as expected. However, fee estimation seems to not work on BTC simnet/regnet, so the server's configured fallback fee is sent to the client for BTC. dcrdex The server got the optimal fee rate of 10 atoms/byte and the fallback of 100 sat/byte: All negotiation steps including fee rate checks succeeded: dexc The client used the fee rates provided in the Since Note that the client's fallback fee is presently hardcoded. It will become a new github issue to have this configured and included in the db wallet scheme. |
buck54321
requested changes
Jul 1, 2020
Comment on lines
+46
to
+48
| // FallbackFeeRate is fee rate used for withdraws, refunds, and DEX fee | ||
| // payment transactions when optimal fee estimation is unavailable. | ||
| FallbackFeeRate uint64 |
Member
There was a problem hiding this comment.
I think this should be a WalletInfo.ConfigOpts setting, since it's particular to BTC and DCR.
I see your related comment at https://github.com/decred/dcrdex/pull/505/files#r447077464. Are we punting for now?
Member
Author
There was a problem hiding this comment.
Thought about ConfigOpts, but I got the impression that was for wallet software (dcrwallet, bitcoind, etc.) settings rather than settings for the Go wallet implementation. Will revisit this.
Member
Author
There was a problem hiding this comment.
Anyway, punt as per matrix chat. @itswisdomagain Maybe this could be a follow up for you. There's a sort of obsolete WalletInfo.DefaultFeeRate now and WalletConfig.FallbackFeeRate. The best way to clean this up depends on when, how, and where we want the fallback fee rate to be set by the user.
Member
There was a problem hiding this comment.
Right. Will think up and share some ideas and put up a PR once an issue for this is put up.
Member
Author
|
Tested on testnet now, also with the C0: https://tbtc.bitaps.com/b31b4f861b8e97728559e88ea66ad9c964c417546db5e5fcec1e47e72bcce1ec C1: https://testnet.dcrdata.org/tx/2e6a113d513d0e99b1d6e2ce02e465f6c9841548a8954a1156e904acb113c9b6 |
buck54321
approved these changes
Jul 2, 2020
Purpose
Smart fee rates from server provided to clients for their swap contract txns.
Details
Server asset fee estimation
- The server's asset backends provide an optimal fee rate (e.g. estimatesmartfee with dcr/btc/etc.).
- Fee rate is in atoms/byte, but we could consider atoms/KB for increased precision.
Fee rate communication
- Instead of the `"feeRate"` asset config parameter, it is now `"maxFeeRate"`, which indicates how much clients are required to reserve in their order-funding coins to cover the worst case fees at swap time.
- The server-originating `'match'` request payload (`msgjson.Match`) now includes `"feeratebase"` and `"feeratequote"`. The client only cares about one presently, the one that applies to their swap contract txns, but the other could be used for both auditing the counterparty contract txn and possibly their own redeem txn. The `msgjson.Match` serialization=>signature is updated to include these fields. The `dex/order.Match` type now also includes `FeeRateBase` and `FeeRateQuote` fields, but these are _not_ included in the serialization=>matchID.
- The `'config'` response from the server now includes a `swapSizeBase` field that corresponds to the size of the asset's swap transaction not including inputs, which is now required to compute the required order funds for arbitary numbers and types of inputs to the first swap in a chain.
Server-side (Swapper) contract fee rate handling
- The Swapper computes required fee rates on start of negotiation (after market matching and before sending `'match'` requests). If fee estimation fails or returns a value higher than the configured maxFeeRate, it uses maxFeeRate.
- The Swapper checks each contract txn's fee rate, ensuring it is at least the required fee rate for the relevant asset as set in the `matchTracker.Match.FeeRate{Base,Quote}`.
- The redemption txns' fee rates are not checked as this is the user's problem.
Client-side contract fee rate handling
- Use the provided fee rate from the `'match'` message.
- Since swap transactions batch several contracts, the highest fee rate across all of the batched swaps is used.
- Client assets estimate their own fee in `Redeem`, `Refund`, `PayFee`, and `Withdraw` internally (with a `FallbackFeeRate`). `Swap` uses the fee rate provided by the server.
- TODO: User setting for fallback fee rate.
Transaction sizing and order funding
- Several transaction sizing fixes, and a new `InitTxSizeBase` const that does not include inputs.
- Order funding had incorrectly computed the required funds for a chain of N swaps while double counting the already-included input of the first swap in the chain.
- Fix `RequiredFunds` (now `RequiredOrderFunds`) to account for this when adding inputs for the first txn in the chain, and perform all computations in integer instead of float.
- Client exchange wallets' `FundOrder` method uses `RequiredOrderFunds` with the `InitTxSizeBase`.
- Server assets provide an `InitTxSizeBase` method
- `Market` is updated to check required funds on new orders using `RequiredOrderFunds` and `InitTxSizeBase`
Server DB updates for contract fee rates
- Store the base and quote rates in the `matches` table.
- Finally use the placeholder `takerSell` column, using the value to load the correct rate for `UserMatch.FeeRateSwap`.
Update Swapper startup for the recently added match time field
- The recent addition of `matchTracker.MatchTime` in ab3dff2 requires reloading this time from `matchTracker.Match.Epoch.End()`
Squashed commit messages:
server/asset: Backend.FeeRate
Update btcd to master for EstimateSmartFee.
TODO: consider changing fee rates from atoms/byte to atoms/KB.
msgjson: Add feeratebase and feeratequote to Match
TODO: consider feerateswap and feerateredeem instead since these are
sent to the actor.
dex/order: add FeeRateBase and FeeRateQuote to Match.
dex/{,msgjson}: Rename FeeRate to MaxFeeRate
msgjson: Match need not be Stampable as ServerTime is set directly
Remove the unused (*Match).Stamp method.
msgjson: include FeeRate{Base,Quote} in serialization
swap: check fee rates in processInit and processRedeem
Currently get optimal fee rates on Negotiate and set them in each
dex/order.Match for the matchTrackers, and msgjson.Match for the
match' route request.
TODO: consider sending fee rates with audit ack and redeem ack requests.
Use dex.Asset.MaxFeeRate.
swap: Negotiate ensures assets of provided matches are supported
dex/calc: refactor RequiredFunds => RequiredOrderFunds
RequiredFunds added the input size to the reference SwapSize, but this
counted an input already. Use a "asset.SwapSizeBase" that is a swap txn size
without an input when computing the first swap in a chain of swaps.
The following N-1 swaps in a chain of N just use SwapSize as before.
The result is the same as before *if* the provided input size is a p2pkh
input as was assumed in the SwapSize (InitTxSize) computation.
The result is only different if the input is sized differently than a
p2pkh input, and we do support that.
Also refactor to use integer math only.
SwapSizeBase in messages and calculations.
client/asset: SwapSizeBase
dex/{,calc,msgjson}: SwapSizeBase fields
client/{core,webserver}: use SwapSizeBase
server: use SwapSizeBase in ConfigResponse and RequiredOrderFunds
client/asset: use fee estimation
Add FeeRate method to asset.Wallet inferface to estimate fee.
Add FeeRate field to the asset.Swaps struct.
Refactor asset.Wallet methods that send txns to either take a fee or use
an internal fee estimate.
- Swap uses the asset.Swaps.FeeRate field.
- Redeem takes a feeRate uint64 input, replacing the dex.Asset arg.
- Refund, PayFee, and Withdraw should estimate their own fee (e.g. with
FeeRate internally).
client/asset/{btc,dcr}: impl. fee est. and new asset.Wallet interface
Add EstimateSmartFee to rpcClient interface.
Add fallbackFeeRate field to ExchangeWallet, used if FeeRate fails to
provied an optimal fee estimate in Redeem, Refund, PayFee, and Withdraw.
Note that FeeRateWithFallback does not need to be part of the interface.
client/core: Add a note a TODO about FallbackFeeRate config and storage
dex/networks: fix some sizes, more docs
btc:
- swap redeem and refund sig script sizes were 1 byte too small;
they needed 2 bytes for to encode the push data size of 97.
- RedeemP2PKHInputSize was missing a varint data size for the sigscript
since TxInOverhead does not include that.
- (*SpendInfo).VBytes was also missing a varint data size byte(s). Use
wire.VarIntSerializeSize for this now, almost always returning 1.
- New InitTxSizeBase that excludes one input (RedeemP2PKHInputSize).
dcr:
- swap redeem and refund sig script sizes were 1 byte too small;
they needed 2 bytes for to encode the push data size of 97.
- Add DataPrefixSize and test it with SwapContractSize.
- Remove the sigscript size varint from TxInOverhead to match btc.
- Update (*SpendInfo).Size to compute the varint size based on the
sigScript as done now in btc's (*SpendInfo).VBytes.
- Use DataPrefixSize in (*SpendInfo).Size.
There are some TODOs with both networks:
- In btc, the InputInfo function should check and restrict the
redeem script format since p2sh with 1-of-1 p2ms is presently
indistinguishable from p2sh with non-multisig.
- ExtractContractHash is functionally ExtractScriptHash.
In dcr it now wraps ExtractScriptHash.
In btc, ExtractScriptHash should be simplified to extract it from the
redeem script []byte instead of extracting addresses and then getting
the script hash from a AddressScriptHash.
server/asset: add InitTxSizeBase to Backend interface
InitTxSizeBase is InitTxSize not including an input.
Some input size bug fixes.
Move EstimateSmartFee to the node interfaces instead of using rpcclient.
btc:
- Implement InitTxSizeBase.
- Add EstimateSmartFee to btcNode interface.
dcr:
- Implement InitTxSizeBase.
- Add EstimateSmartFee to btcNode interface.
- Use (*SpendInfo).Size in (*Backend).{utxo,output}
server/market: use InitTxSizeBase and RequiredOrderFunds
In order router, use InitTxSizeBase and the revised RequiredOrderFunds
to compute the required funds for incoming orders.
client/core: use FeeRate{Quote,Base} from Match msg
In (*trackedTrade).negotiate, use the fee rate from the match message.
Store the fee rate for the swap txn in
matchTracker.MetaMatch.Match.FeeRateSwap.
In (*trackedTrade).swapMatches where swaps are batches, get the highest
fee rate of all the matchTrackers sent to swap.
Use the new client/asset.Wallet interface (Swap, Refund, FeeRate, etc.).
Use asset.Wallet.FeeRate() to get the fee rate for the redeem txn.
NOTE that the redeem fee rate could be provided by the server, but the
user is already incentivized to get the redeem mined as it pays to them.
Hypothetically, the redeem fee rate could come from the 'audit' and
'redemption' requests from the server. Alternatively, the slightly
stale fee rate provided in the 'match' request, which is presently just
used for the contract txns, could also apply to the redemption txn.
dex/order: Add FeeRateSwap field to UserMatch
swap: update state for MatchTime in matchTracker
NOTE: MatchTime is not stored in the main DB, but it is used to define
the locktime for the swap contract and redeem txns.
db: matches table: store baserate and quoterate
Add BaseRate, QuoteRate, and TakerSell to db.MatchData.
Use the existing takerSell column, loading into MatchData.
Consider creating a Sell field in dex/order.UserMatch.
client/asset/btc: fix CompatibilityCheck ignoring P2WSHScript/WSHAddr
market: remove stray racy write in TestMarket_Cancelable
dex/networks/btc: correctly work with 1-of-1 p2ms-p2sh in InputInfo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolving #502
Purpose
Smart fee rates from server provided to clients for their swap contract txns.
Details
Server asset fee estimation
asset.Backendinterface methodFeeRate() (uint64, error)is added.Fee rate communication
"feeRate"asset config parameter, it is now"maxFeeRate", which indicates how much clients are required to reserve in their order-funding coins to cover the worst case fees at swap time.'match'request payload (msgjson.Match) now includes"feeratebase"and"feeratequote". The client only cares about one presently, the one that applies to their swap contract txns, but the other could be used for both auditing the counterparty contract txn and possibly their own redeem txn. Themsgjson.Matchserialization=>signature is updated to include these fields. Thedex/order.Matchtype now also includesFeeRateBaseandFeeRateQuotefields, but these are not included in the serialization=>matchID.Server-side (Swapper) contract fee rate handling
'match'requests). If fee estimation fails or returns a value higher than the configured maxFeeRate, it uses maxFeeRate.matchTracker.Match.FeeRate{Base,Quote}.Client-side contract fee rate handling
'match'message.Redeem,Refund,PayFee, andWithdrawinternally (with aFallbackFeeRate).Swapuses the fee rate provided by the server.Transaction sizing and order funding
InitTxSizeBaseconst that does not include inputs.RequiredFunds(nowRequiredOrderFunds) to account for this when adding inputs for the first txn in the chain, and perform all computations in integer instead of float.FundOrdermethod usesRequiredOrderFundswith theInitTxSizeBase.InitTxSizeBasemethod.Marketis updated to check required funds on new orders usingRequiredOrderFundsandInitTxSizeBase.Server DB updates for contract fee rates
matchestable.takerSellcolumn, using the value to load the correct rate forUserMatch.FeeRateSwap.Update Swapper startup for the recently added match time field
matchTracker.MatchTimein core: fix locktime handling #438 requires reloading this time frommatchTracker.Match.Epoch.End().