client/core,eth: Lock funds for refund #1479

martonp · 2022-02-19T17:38:19Z

Previously, the funds needed for refunds were locked in FundOrder together with the funds needed for initiations, but they were unlocked when Swap was called with LockChange = false. This diff applies the same logic we use to reserve funds for redemptions to refunds.

Prior to this PR, redemptions were being reserved using the server's max fee rate, but the redemption transaction was being submitted with the current fee rate estimate. This PR updates this to use the server's max fee rate when submitting the transaction as well. There is no reason to use a lower rate when submitting the transaction, as ETH uses dynamic transaction fees, and the entire fee will not be used anyways. The server's max fee rate is also used for refunds.

client/asset: Rename AssetRedeemer interface to AssetLocker, and add functions for refunds with the same functionality as already exist for redemptions.
client/eth: Refactor fund locking to keep track of funds locked for initiation, refunds, and redemptions separately. Also, make ETH a FeeRater.
client/core: Keep track of refund reserves the same way as they are tracked for redemptions. In addition to unlocking when the deal is revoked or cancelled, funds reserved for refunds are also unlocked when either the counterparty or the current user does a redemption.

Closes #1466

client/asset/eth/eth.go

client/core/core.go

chappjc · 2022-02-22T20:48:13Z

Minor conflict by proximity to FeeRater changes in core_test.go. Not holding up my review though, that's coming shortly.

JoeGruffins

Working well.

chappjc

Some quick comments. I think we need to have a discussion in matrix about making the eth client a FeeRate with the formula we've settled on, and then perhaps capping the computed rate by the user's fee rate limit. related: #1485

client/db/types.go

client/asset/interface.go

client/asset/eth/eth.go

- client/asset: Rename AssetRedeemer interface to AssetLocker, and add identical functions used for refunds in addition to redemptions. - client/eth: Refactor fund locking to keep track of funds locked for initiation, refunds, and redemptions separately. - client/core: Keep track of refund reserves the same way as they are tracked for redemptions. In addition to unlocking when the deal is revoked or cancelled, funds reserved for refunds are also unlocked when either the counterparty or the current user does a redemption.

chappjc · 2022-02-28T20:49:25Z

client/core/core.go

@@ -4203,17 +4206,31 @@ func (c *Core) prepareTrackedTrade(dc *dexConnection, form *TradeForm, crypter e
 		if len(pubKeys) == 0 || len(sigs) == 0 {
 			return nil, 0, newError(signatureErr, "wrong number of pubkeys or signatures, %d & %d", len(pubKeys), len(sigs))
 		}
-		redemptionReserves, err = accountRedeemer.ReserveN(redemptionLots, wallets.toAsset.MaxFeeRate, wallets.toAsset.Version)
+		redemptionReserves, err = accountRedeemer.ReserveNRedemptions(redemptionRefundLots, wallets.toAsset.MaxFeeRate, wallets.toAsset.Version)


Works for me. Only additional thought about this matter is that the rate given to ReserveNRedemptions and ReserveNRefunds could be the larger of MaxFeeRate and the rate from FeeRate (if also a FeeRater). That would be pretty screwy if the server had a rate set that low, so whatever, let's not change anything now. This PR looks good to me.

If the swap cannot be initiated, then it doesn't matter if too little is reserved for refunds and redemptions.

The swap tx could still be mined with a gas fee cap less than what FeeRate would recommend. It would just be dicey for the refund (that happens in several hours). For the redeem, you're redeeming the other person's swap, so who knows what they used.

client/core/trade.go

chappjc · 2022-02-28T20:54:14Z

client/db/bolt/db.go

@@ -95,7 +96,8 @@ var (
 	fromVersionKey         = []byte("fromVersion")
 	toVersionKey           = []byte("toVersion")
 	optionsKey             = []byte("options")
-	reservesKey            = []byte("reservesKey")


For the record, I'm on board with breaking compatibility with the last dev version of the DB encoding. Not worth an upgrade esp. considering the state of ETH (not used in the wild).

…funds.

chappjc

LGTM. Kinda too bad that the unlock method has an error return though.

chappjc · 2022-03-01T00:05:28Z

client/asset/eth/eth.go

+	default:
+		return nil, fmt.Errorf("invalid fund reserve type: %v", t)


I'm tempted to say panic, remove the error and logging in unlock's caller.
With an enum of fundReserveTypes, it takes a special type of programmer error to pass an invalid value.

chappjc · 2022-03-01T15:07:38Z

Ready to merge unless @buck54321 is reviewing.

buck54321 · 2022-03-03T11:24:48Z

client/asset/eth/eth.go

+	initiationFunds := ord.Value + maxSwapFees
+	coins := asset.Coins{eth.createFundingCoin(initiationFunds)}
+
+	eth.lockedFundsMtx.Lock()
+	defer eth.lockedFundsMtx.Unlock()
+	err := eth.lockedFunds.lock(initiationFunds, initiationReserve, eth.balance)


Can we lock up the refund reserves here too, so the caller doesn't have to do it separately?

I was thinking about that too, but I did it this way because if the refund reserves were locked here, then FundOrder would need to return the amount locked for refunds separately. I didn't want this to have to be done in non-account based coins.

I see what you mean. I might propose a change later, but this is good for now.

buck54321 · 2022-03-03T11:32:17Z

client/asset/eth/eth.go

+}
+
+// lock locks funds for a use case.
+func (r *fundReserves) lock(amt uint64, t fundReserveType, getBalanceFn func() (*asset.Balance, error)) error {


A few things here that suggest to me that fundReserves is better as an ExchangeWallet struct field. 1) You're passing in ExchangeWallet methods, 2) you're returning errors just for logging, and 3) the mutex is still being handled by ExchangeWallet. How about converting these to ExchangeWallet methods and doing

type ExchangeWallet struct { ... lockedFundsMtx sync.RWMutex lockedFunds struct { initiateReserves uint64 redemptionReserves uint64 refundReserves uint64 } ... }

That's a good point. Could even embed the mutex in that struct

buck54321 · 2022-03-03T12:11:29Z

client/core/trade.go

-	newReserved := applyFraction(num, denom, t.redemptionReserves)
-	if t.redemptionLocked+newReserved > t.redemptionReserves {
+
+	newReserved := t.reservesToLock(num, denom, t.redemptionReserves, t.redemptionLocked)
+
+	if err := redeemer.ReReserveRedemption(newReserved); err != nil {
+		t.dc.log.Errorf("error re-reserving redemption %d %s for order %s: %v",
+			newReserved, t.wallets.toAsset.UnitInfo.AtomicUnit, t.ID(), err)
+		return
+	}
+	t.redemptionLocked += newReserved
+}


This is a change in behavior, even if the outcome may be identical. Before, if the requested reservers were > redemptionReserves, an error was logged and that's it. Now, an error is logged, but we still call ReReserveRedemption with an argument of 0, and add 0 to the redemptionLocked. I don't expect that this would result in any issues, but calling ReReserveRedemption with 0 is questionable behavior.

buck54321 · 2022-03-03T12:32:12Z

client/asset/interface.go

-	// UnlockReserves is used to return funds when an order is canceled or
-	// otherwise completed unfilled.
-	UnlockReserves(uint64)
+	ReserveNRefunds(n, feeRate uint64, assetVer uint32) (uint64, error)


This method would be unneeded if we lock up the refund reserves in FundOrder.

buck54321

A few last things.

buck54321 · 2022-03-05T12:30:22Z

client/asset/eth/eth.go

+
+// fundReserveOfType returns a pointer to the funds reserved for a particular
+// use case.
+func (eth *ExchangeWallet) fundReserveOfType(t fundReserveType) *uint64 {


Let's move this down by the other related methods below the ExchangeWallet definition.

buck54321 · 2022-03-05T12:30:48Z

client/asset/eth/eth.go

 func (eth *ExchangeWallet) balance() (*asset.Balance, error) {
 	bal, err := eth.node.balance(eth.ctx)
 	if err != nil {
 		return nil, err
 	}

-	locked := eth.locked + eth.redemptionReserve
+	locked := eth.amountLocked()
+	fmt.Printf("in balance -- %+v - %v\n", bal, locked)


Looks like this can go

buck54321 · 2022-03-05T12:35:46Z

client/core/core.go

-		c.db, c.latencyQ, wallets, coins, c.notify, c.formatDetails, form.Options, redemptionReserves)
+		c.db, c.latencyQ, wallets, coins, c.notify, c.formatDetails, form.Options, redemptionReserves, refundReserves)
+
+	tracker.redemptionLocked = tracker.redemptionReserves


You've got another one of these three lines down.

buck54321 · 2022-03-05T12:50:41Z

client/core/trade.go

@@ -640,6 +702,7 @@ func (t *trackedTrade) negotiate(msgMatches []*msgjson.Match) error {
 	// reserves.
 	if remain := trade.Quantity - preCancelFilled; remain > 0 && (completedMarketSell || cancelMatch != nil) {


Not from this PR, but I think we need to return reserves when lo, ok := t.Order.(*order.LimitOrder); ok && lo.Force == order.StandingTiF here too.

You mean ImmediateTiF right?

Yes. Sorry.

JoeGruffins reviewed Feb 22, 2022

View reviewed changes

client/asset/eth/eth.go Outdated Show resolved Hide resolved

client/asset/eth/eth.go Show resolved Hide resolved

client/asset/eth/eth.go Outdated Show resolved Hide resolved

client/asset/eth/eth.go Outdated Show resolved Hide resolved

client/core/core.go Show resolved Hide resolved

martonp force-pushed the lockRefundFees branch from 8544515 to 546fed0 Compare February 22, 2022 15:29

JoeGruffins approved these changes Feb 23, 2022

View reviewed changes

chappjc reviewed Feb 24, 2022

View reviewed changes

client/db/types.go Outdated Show resolved Hide resolved

client/asset/interface.go Outdated Show resolved Hide resolved

client/asset/interface.go Outdated Show resolved Hide resolved

client/asset/eth/eth.go Outdated Show resolved Hide resolved

martonp added 2 commits February 25, 2022 17:55

Fixes based on review

1219455

martonp force-pushed the lockRefundFees branch 4 times, most recently from 5d55f85 to 7281814 Compare February 26, 2022 23:30

chappjc reviewed Feb 28, 2022

View reviewed changes

client/core/trade.go Outdated Show resolved Hide resolved

chappjc reviewed Feb 28, 2022

View reviewed changes

Make eth fee rater and use server max fee rate for redemptions and re…

6c8fcc1

…funds.

martonp force-pushed the lockRefundFees branch from 7281814 to 6c8fcc1 Compare February 28, 2022 23:12

chappjc approved these changes Mar 1, 2022

View reviewed changes

chappjc requested a review from buck54321 March 1, 2022 15:07

buck54321 reviewed Mar 3, 2022

View reviewed changes

Updates based on review

8549c5e

buck54321 requested changes Mar 5, 2022

View reviewed changes

Unlock reserves for ImmediateTiF and other minor updates

bdb6960

buck54321 approved these changes Mar 5, 2022

View reviewed changes

chappjc merged commit 20634d4 into decred:master Mar 5, 2022

chappjc added this to the 0.5 milestone Apr 21, 2022

chappjc added the ETH label Aug 26, 2022

martonp deleted the lockRefundFees branch December 20, 2022 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/core,eth: Lock funds for refund #1479

client/core,eth: Lock funds for refund #1479

martonp commented Feb 19, 2022 •

edited

Loading

chappjc commented Feb 22, 2022

JoeGruffins left a comment

chappjc left a comment

chappjc Feb 28, 2022

martonp Feb 28, 2022

chappjc Feb 28, 2022 •

edited

Loading

chappjc Feb 28, 2022 •

edited

Loading

chappjc left a comment

chappjc Mar 1, 2022

chappjc commented Mar 1, 2022

buck54321 Mar 3, 2022

martonp Mar 3, 2022

buck54321 Mar 4, 2022

buck54321 Mar 3, 2022

chappjc Mar 3, 2022

buck54321 Mar 3, 2022

buck54321 Mar 3, 2022

buck54321 left a comment

buck54321 Mar 5, 2022

buck54321 Mar 5, 2022

buck54321 Mar 5, 2022

buck54321 Mar 5, 2022

martonp Mar 5, 2022

buck54321 Mar 5, 2022

		default:
		return nil, fmt.Errorf("invalid fund reserve type: %v", t)

		@@ -640,6 +702,7 @@ func (t trackedTrade) negotiate(msgMatches []msgjson.Match) error {
		// reserves.
		if remain := trade.Quantity - preCancelFilled; remain > 0 && (completedMarketSell \|\| cancelMatch != nil) {

client/core,eth: Lock funds for refund #1479

client/core,eth: Lock funds for refund #1479

Conversation

martonp commented Feb 19, 2022 • edited Loading

chappjc commented Feb 22, 2022

JoeGruffins left a comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Feb 19, 2022 •

edited

Loading

chappjc Feb 28, 2022 •

edited

Loading

chappjc Feb 28, 2022 •

edited

Loading