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

client: Base fraction unlocking on total lots. #1642

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Jun 2, 2022

closes #1544

The function wasn't a problem on the default simnet, but when used with smaller lot sizes isDust does not scale well. You can try out the way in master here: https://go.dev/play/p/QNqo2EDUiUX

I think using the ratio of one lot to max lots and multiplying that times the max reserved is more straightforward.

Also adding a log for client to see the net and locktimes on start up.

@JoeGruffins JoeGruffins marked this pull request as ready for review June 2, 2022 08:41
@chappjc
Copy link
Member

chappjc commented Jun 2, 2022

Isn't there a way to just pass a final bool to unlock all remaining? Surely either the caller or the trackedTrade can determine that based on some counters. I guess that's the same as (1,1), but it still seems like counting somewhere would determine when its the last one.
Just the fact that we're trying to detect left over reserves is super confusing and a little unsettling tbh.
If that makes no sense, no worries, I still don't have a firm grasp on the reserve system we have. I wish I had a better grasp of why dust in the reserves is even a thing.

@chappjc chappjc requested a review from martonp June 2, 2022 15:08
@chappjc
Copy link
Member

chappjc commented Jun 2, 2022

Another problem that relates to refund reserves is that it's never being saved to the DB, only RedemptionReserves

dcrdex/client/core/core.go

Lines 4534 to 4536 in a1d4690

Options: form.Options,
RedemptionReserves: redemptionReserves,
ChangeCoin: changeID,

But there's a RefundReserves too:

dcrdex/client/db/types.go

Lines 280 to 287 in a1d4690

RedemptionReserves uint64
// RedemptionRefunds is the amount of funds reserved by the wallet to pay
// the transaction fees for all the possible refunds in this order.
// The amount that should be locked at any point can be determined by
// checking the status of the order and the status of all matches related
// to this order, and determining how many more possible refunds there
// could be.
RefundReserves uint64

@chappjc
Copy link
Member

chappjc commented Jun 2, 2022

Definitely fix these things, but I'm investigating a reworked reserves system. If it pans out I'll share. I don't want to spend more than a day on it.

@JoeGruffins
Copy link
Member Author

just rebased

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

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

The change makes sense. I guess this was happening when a significant portion of the reserved funds were being used for the tx fees?

@JoeGruffins
Copy link
Member Author

I guess this was happening when a significant portion of the reserved funds were being used for the tx fees?

Yes, toggling the reserved amount can also make isDust true or false. Trying to think of a way to indicate this is the last trade so we can not worry about dust... or something.

@chappjc
Copy link
Member

chappjc commented Jun 6, 2022

Trying to think of a way to indicate this is the last trade so we can not worry about dust... or something.

I'll put something up in the morning. Almost ready.
We can merge this now though.

@chappjc chappjc merged commit 65d0d00 into decred:master Jun 6, 2022
@chappjc
Copy link
Member

chappjc commented Jun 6, 2022

@JoeGruffins Lots of test code to update, also completely untested with ETH, but this is the way I'm going: a96a980 Please peruse to see the concept though. The goal is to make it usable by the consumer (Core) and make rounding errors and the dust issue go away. It does handle a remainder if the reserves given by the wallet are not evenly divisible by the given N, but that is not sensible and should always be zero.

In doing this, I realized the LCM stuff was unnecessary because it was looping through matches for a given trackedTrade, for which the result of trade.isMarketBuy would be the same for all matches for that trade. But that's not important anymore if this concept works.

@JoeGruffins
Copy link
Member Author

type reservesCounter struct {
	done uint32 // mutable atomic

	num  uint32 // max lot / swap count
	mult uint64 // amount reserved per swap
	rem  uint64 // ideally zero, but handled if not
}

This is also what I had imagined we need to do, so pr it up...

@chappjc chappjc added this to the 0.5 milestone Aug 26, 2022
@chappjc chappjc added the ETH label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client/eth: attempting to unlock more than is reserved
3 participants