client: order options #1170

buck54321 · 2021-08-26T18:50:34Z

Related to #1164, this work allows wallet-defined, user-selected, per-order options that affect how swaps and redemptions are processed by the wallet.

This PR is barely roughed out, but is presented for discussion in contrast with #1164. The primary benefits of this approach over that of #1164, imo, are

The system is generalized and will provide flexibility for different assets.
The use of the new Options is completely optional. Developers of new assets can ignore them, or add them in piecemeal as demand requires.
core and db will not need to be updated with new options, since they don't actually use the options in any meaningful way.

~~Ignore the front-end stuff for now, and btc (and clones) is the only asset done so far.~~

Ready for review

martonp · 2021-09-02T17:49:29Z

I definitely agree with using an options map like you have here. I also think there should be a new request type that returns which order options each asset supports.

I'll start by updating #1164 to use this instead of how I have it there and adding this new request. Then add in these other options (feebump/splittx) in subsequent PRs. Does that sound good?

chappjc · 2021-09-14T16:30:14Z

The way forward that I see for this PR and #1164 is just for this PR to be pared down to just the backend pieces and merged as the basis for other options as well as the frontend work to use it all.

I'm not sure I see a clean way to rework 1164 on this basis short of just starting over with this one, but it's already here.

Let's clean this one up and see how it forms a basis for continuing on the order options feature.
Then let's iterate.

I also think there should be a new request type that returns which order options each asset supports.

We'll have to think this through some more as applicable options will vary depending on the wallet settings and other things we'd likely get from an order estimate.

@martonp Please work this out with @buck54321, but it seems simplest to get this on in. I think we are starting on a feature that is going to involve several iterations with plenty of room for improvement.

In any case, these are not for 0.3, so not top priority.

martonp · 2021-09-15T14:51:05Z

I've been working on some changes since this PR assumes that the options will not need to be used in core, but MinFeeRate actually does need to be used in core. I'll have something up later today.

buck54321 · 2021-09-15T15:53:50Z

I've been working on some changes since this PR assumes that the options will not need to be used in core, but MinFeeRate actually does need to be used in core. I'll have something up later today.

Maybe you could explain more, but we shouldn't be reasoning about these options in core.

martonp · 2021-09-15T17:53:14Z

Here is an example from PR #1164:

dcrdex/client/core/trade.go

Line 1380 in dcd429f

if t.metaData.MinSwapFeeRate != nil &&

This code in core needs to know if there's a minimum fee rate for a swap in order to pick the fee rate to use.

martonp · 2021-09-15T18:06:21Z

Why shouldn't we be reasoning about order options in core?

buck54321 · 2021-09-16T02:06:54Z

Why shouldn't we be reasoning about order options in core?

We obviously worry about some order options in core, but they are generally things that are communicated with the server. These additional fees are not communicated with the server, and their availability, interpretation, valid ranges, units, verbiage etc. are going to have a large degree of asset-dependence.
We don't need to do the fee rate logic in core. The example you linked is functionally identically to what I do here because the MinSwapFeeRate / FeeBump is an order-level variable, so you really only need to apply the highestFeeRate comparison once after the matches loop,

var highestFeeRate uint64
for i, match := range matches {
    ...
}
if t.metaData.MinSwapFeeRate != nil && *t.metaData.MinSwapFeeRate > highestFeeRate {
    highestFeeRate = *t.metaData.MinSwapFeeRate
}

and that operation can trivially be moved to the asset backend's Swap method.

The rate handling here is not quite right anyway (sorry I haven't given any feedback on the PR yet). This would allow a server to set a fee rate > MaxFeeRate, but that's a rogue server.
Handling order options in the backend allows us to leverage the advantages from this PR's description.

The system is generalized and will provide flexibility for different assets.

The use of the new Options is completely optional. Developers of new assets can ignore them, or add them in piecemeal as demand requires.

core and db will not need to be updated with new options, since they don't actually use the options in any meaningful way.

chappjc · 2021-09-16T02:33:12Z

Yeah, I agree with that assessment. While this particular change is not at the top of our priorities, I think this PR should get out of draft soon so we can more clearly evaluate next steps and what if anything should come out of #1164. Whichever way we go, I think this entire initiative is important as it gives flexibility to the user, and in this PR's framework it keeps Core logic only concerned with logic that only it can/must handle while allowing backends to decide about the relevance and details of particular options on their own.

martonp · 2021-09-16T17:02:18Z

You're right, it seems there doesn't need to be logic in core that deals with MinFeeRate. I guess the changes I was working on are not necessary in this case, but I still think they might have some merit.

The issue I see with the current solution is that each of the options are defined separately in each asset's code, but when making the UI or the rpc client, there needs to be a unified set of options that are exposed. The ini tags on the option structs must also be consistent across each of the assets, and there's no way to currently enforce that through code.

My idea was to have the full set of options be defined separately, and then each asset would just specify which of these options it supports. This technique will preserve all of the benefits that you described, but it would also provide consistency, and it would also give us the option to use these options in core if this is ever needed. It would still never be necessary to update core when someone is adding a new order option though.

Here is a rough draft, let me know what you guys think:
#1216

buck54321 · 2021-11-15T12:06:07Z

JoeGruffins

Looks super cool!

I ran into a problem swapping, just using random values. Unfortunately I have not been able to reproduce, but here was the error I saw. Was with a fresh simnet dex and client.
[ERR] CORE: 127.0.0.1:17273 tick: {swapMatches order 36e26c2efb664116d4a4ce7aa1852a879b58356c8223dcb3d6b46cd223a565df - {error sending swap transaction: not enough funds to cover minimum fee rate. 0.0000428 < 0.0000251}}

I'll try a few more times though.

edit:
It seems to happen pretty consistently when I trade with myself with one lot and turning the three options on. Let me know if you are unable to reproduce.

JoeGruffins · 2021-11-22T05:15:27Z

client/asset/btc/btc.go

+	if feeBump > 1.01 {
+		bumpedMaxRate = uint64(math.Round(float64(bumpedMaxRate) * feeBump))
+		bumpedNetRate = uint64(math.Round(float64(bumpedNetRate) * feeBump))
+	}


feeBump > 1.0 would indicate a fee bump?

Sort of setting an informal minimum bump of 1% here, but there's not real reason it has to be that way.

Greater than 1% looks like. But I don't see a downside to blindly multiplying by feeBump as long as it is >1.

More fundamentally, the units and meaning of FeeBump all the way back to swapOptions and redeemOptions is not perfectly clear. It could conceivable be a percent added e.g. 0.01 instead of a multiplier like 1.01, or it could even be an integer that is an absolute sat/vB to add, or an integer that represents steps of say 0.1%.
Just depends on the definition of calcBumpedRate I suppose.

client/asset/btc/btc.go

JoeGruffins · 2021-11-22T05:51:03Z

client/asset/btc/btc.go

+		if fee > totalIn {
+			return nil, nil, 0, fmt.Errorf("redeem tx not worth the fees")
+		}
+		btc.log.Warnf("Ignoring fee bump (%v) resulting in fees > redemption", customCfg.FeeBump)


I think this only prints the pointer value as well, so not very useful. Maybe you want the float value.

client/asset/btc/btc.go

client/asset/dcr/dcr.go

JoeGruffins · 2021-11-22T06:28:30Z

client/asset/estimation.go

+	// is used, the fee rate used should be at least the suggested fee, else
+	// zero-conf coins might be rejected.


Ok, I see the reasoning for the TODO now.

client/asset/options.go

client/asset/btc/btc.go

chappjc · 2021-12-15T19:49:48Z

client/asset/btc/btc.go

+	userBump := *bump
+	if userBump > 2.0 {
+		return baseRate, fmt.Errorf("fee bump %f is higher than the 2.0 limit", userBump)


Hmm, normally this is quite sensible. But when fees are generally low, like 1 or 2 sat/vB, I'd really want to bump in absolute units of sat/vB. Applying a multiplier bump of 2.0 to a 1.0 sat/vB is fairly insignificant, and provides little buffer in the event rising fees. This is fine, just thinking for another scale option in the future.

Outside of DEX, I often pick a rate that is a only a few sat/vB higher than the current recommended, but still several times higher. A percentage is obviously the most robust for dealing with a wide range of fee rates over time, and good for higher rates, so this is sensible. I just wonder if we'll end up wanting an absolute bump options at some point, or maybe a percent limit that scales with current fee rate

I agree with this. An absolute bump would be preferred in some cases, but a multiplier has a wider range of applicability and also provides a natural upper limit to the bump size without hard-coded max values. Can re-visit as needed, of course.

client/asset/dcr/dcr.go

chappjc · 2021-12-15T20:19:58Z

client/asset/dcr/dcr.go

+		reason = fmt.Sprintf("+%d%% fees, -%s DCR overlock", int(math.Round(pctChange)), amount(overlock))
+	} else {
+		reason = fmt.Sprintf("+%.1f%% fees, -%s DCR overlock", pctChange, amount(overlock))


I find the negative amount overlock a bit confusing. We are trying to say that they are paying more in fees because of the added transaction, but doing that eliminates/avoids the over locking of %s DCR.
Is there are clearer way to express this within the constraints of this ui dialog?

client/webserver/site/webpack/dev.js

Add order-time wallet options. The options are returned as part of a pre-order estimate, and are persisted as part of order metadata so that they can be passed to FundOrder, Swap, and Redeem. BTC, clones, and DCR all get two swap options (fee bump, split) and 1 redemption option (fee bump). Define two different option types, boolean and xy-range. Each input type corresponds to a different UI input widget, with boolean being a simple checkbox while xy-range offers a slider and text inputs. The types are intentionally generic for reusability.

chappjc added this to the 0.4 milestone Aug 31, 2021

buck54321 mentioned this pull request Sep 3, 2021

eth/client: Implement MaxOrder #1184

Merged

martonp mentioned this pull request Sep 24, 2021

Draft: order options #1216

Closed

buck54321 force-pushed the order-estimates branch 4 times, most recently from 4ed0375 to 50551e2 Compare September 26, 2021 10:43

buck54321 mentioned this pull request Oct 26, 2021

app: improved registration sequence and form design #1234

Merged

buck54321 force-pushed the order-estimates branch from 50551e2 to 11ba297 Compare October 30, 2021 19:33

buck54321 force-pushed the order-estimates branch 2 times, most recently from 1aacd2e to 0176478 Compare November 15, 2021 12:05

buck54321 marked this pull request as ready for review November 15, 2021 12:06

chappjc removed this from the 0.4 milestone Nov 17, 2021

JoeGruffins reviewed Nov 22, 2021

View reviewed changes

chappjc mentioned this pull request Nov 22, 2021

rpc/client: Minimum fee rate for swap transactions #1164

Closed

chappjc added this to the 0.5 milestone Nov 28, 2021

buck54321 force-pushed the order-estimates branch from 0176478 to 64f8008 Compare December 6, 2021 08:53

chappjc reviewed Dec 15, 2021

View reviewed changes

buck54321 added 2 commits January 5, 2022 09:45

Use bumped rate for split remainder calcs

808de9d

buck54321 added 2 commits January 5, 2022 09:45

review followup

0487d57

review followup II

0f74bb4

buck54321 force-pushed the order-estimates branch from 8812be4 to 0f74bb4 Compare January 5, 2022 17:05

chappjc approved these changes Jan 7, 2022

View reviewed changes

chappjc merged commit 17a96d1 into decred:master Jan 7, 2022

chappjc mentioned this pull request Mar 15, 2022

client: allow paying higher swap fee rate than server specified rate #859

Closed

		// is used, the fee rate used should be at least the suggested fee, else
		// zero-conf coins might be rejected.

client: order options #1170

client: order options #1170

Uh oh!

Conversation

buck54321 commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martonp commented Sep 2, 2021

Uh oh!

chappjc commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martonp commented Sep 15, 2021

Uh oh!

buck54321 commented Sep 15, 2021

Uh oh!

martonp commented Sep 15, 2021

Uh oh!

martonp commented Sep 15, 2021

Uh oh!

buck54321 commented Sep 16, 2021

Uh oh!

chappjc commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martonp commented Sep 16, 2021

Uh oh!

buck54321 commented Nov 15, 2021

Uh oh!

JoeGruffins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeGruffins Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

buck54321 Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

chappjc Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoeGruffins Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoeGruffins Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chappjc Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chappjc Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

buck54321 commented Aug 26, 2021 •

edited

Loading

chappjc commented Sep 14, 2021 •

edited

Loading

chappjc commented Sep 16, 2021 •

edited

Loading

JoeGruffins left a comment •

edited

Loading

chappjc Dec 15, 2021 •

edited

Loading

chappjc Dec 15, 2021 •

edited

Loading