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

multi: order fee estimation #958

Merged
merged 5 commits into from
Feb 25, 2021
Merged

multi: order fee estimation #958

merged 5 commits into from
Feb 25, 2021

Conversation

buck54321
Copy link
Member

An order fee estimate consists of 3 values for the swap and 2 for the redemption. The swap has a max possible fees (MaxFeeRate), and both high and low estimates corresponding to the best and worst case settlement sequences, but at the prevailing network fee rate.

Similarly, the redemption estimate has high and low estimates evaluated for the worst and best settlement sequences (1 tx per lot
vs 1 tx for all).

Comment on lines +359 to +361
type PreSwap struct {
Estimate *SwapEstimate `json:"estimate"`
}
Copy link
Member Author

@buck54321 buck54321 Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of a few seemingly unnecessary wrapper structs that will soon be used to accommodate the order-time options. I've made some temporary comments in this file to explain their purpose.

Since order-time options are entirely optional, setting up the structs now means that when the options become available, we don't need to break the API.

client/asset/btc/btc.go Outdated Show resolved Hide resolved
func (btc *ExchangeWallet) RedemptionFees() (uint64, error) {
// estimateSwap prepares an *asset.SwapEstimate.
func (btc *ExchangeWallet) estimateSwap(lots, lotSize, networkFeeRate uint64, utxos []*compositeUTXO,
nfo *dex.Asset, trySplit bool) (*asset.SwapEstimate, bool /*split used*/, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the second return is ever utilized. It will be in the future?

Copy link
Member Author

@buck54321 buck54321 Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be in the future?

Yes. Split transactions will be an order-time option for utxo-based blockchains, so we'll need to know which path estimateSwap took.

client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
// chained_swap_sizes = (lots - 1) * swap_size
// first_swap_size = swap_size_base + backing_bytes
// total_bytes = first_swap_size + chained_swap_sizes
// total_bytes = swap_size_base + backing_bytes + (lots - 1) * swap_size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the second one here be total_bytes +=?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are actually just two different formula for the same thing. I probably just shouldn't have moved this. It made more sense where it was, I think.

return &asset.PreRedeem{
Estimate: &asset.RedeemEstimate{
RealisticWorstCase: worst * feeRate,
RealisticBestCase: best * feeRate,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the best case useful for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All we can really say before matching is that the fees will (realistically) fall in a range between the best and worst case. Best case provides the lower limit. Worst cast provides the upper limit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to expand on this a little, the worst case is probably the one I would pay attention to the most, but for users who place large orders, they may want to collect statistics over time to better estimate where their risk falls between these two points.

From a UI perspective, this will allow us to offer some additional clarity on fees instead of just showing the worst-case value.

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/interface.go Outdated Show resolved Hide resolved
client/asset/interface.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just some minor nits and probably just a little confusion on my part about how PreRedeem is called from MaxBuy and MaxSell, also if PreRedeem is going to use lotsize at some point because it's only using lots now.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/asset/interface.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/orderbook/orderbook.go Outdated Show resolved Hide resolved
buck54321 and others added 5 commits February 25, 2021 14:29
An order fee estimate consists of 3 values for the swap and 2 for
the redemption. The swap has a max possible fees (MaxFeeRate),
and both high and low estimates corresponding to the best and worst
case settlement sequences, but at the prevailing network fee rate.

Similarly, the redemption estimate has high and low estimates
evaluated for the worst and best settlemetn sequences (1 tx per lot
vs 1 tx for all).
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

Successfully merging this pull request may close these issues.

None yet

3 participants