eth: locked funds remain after match revoke

[chappjc]

The locked funds are off by a small amount after refunding or any failed swap. Here after a refund. It fixes itself on dexc restart.

Originally posted by @JoeGruffins in #1455 (comment)

[martonp]

I'll take a look.

[chappjc]

I noticed that the error return from unlockFunds is generally ignored except when called from ReturnCoins. So I suspect there are "attempting to unlock more than is currently locked" errors being ignored (and the locked amount not being decremented).
If you have something going on this, let's at least log errors from unlockFunds in client/asset/eth/eth.go when it's not appropriate to interrupt the flow. We might also consider setting eth.locked to 0 in this case rather than leaving it as long as we WRN.

[martonp]

That makes sense. Another issue I see is that on the last call to Swap for a trade, lockChange is set false, but this means that the funds that were locked in order to be used for a potential refund will no longer be locked.

[martonp]

I'm pretty sure that 0.126 you see there is just a gas fee, if you mine another block it will go away. I saw the same amount there even after a successful swap.

In the balance function, pending outgoing transactions are treated as locked:

dcrdex/client/asset/eth/eth.go

Line 382 in 8e22b7f

locked := eth.locked + eth.redemptionReserve + dexeth.WeiToGwei(bal.PendingOut)

Shouldn't this just be subtracted from available but not part of locked?

[chappjc]

Shouldn't this just be subtracted from available but not part of locked?

Hmm, that is odd. When swaps are sent, we account for those funds-in-limbo at the higher level in core.WalletBalance.ContractLocked, which frontend just adds to the wallet's locked for display. So I don't see why we'd need to count such funds twice (along with regular sends from the wallet like a withdraw) while they are unconfirmed. Those funds are either gone as in the case of a withdraw or accounted for on ContractLocked in the case of a swap. Similarly, tx fees for pending txns are added to outgoing as well, but those should be considered "gone" too. It's a good categorization for eth.Balance, but I don't see why PendingOut would be added to asset.Balance.Locked.

I think the intent may have been this:

diff --git a/client/asset/eth/eth.go b/client/asset/eth/eth.go
index 67c1c74c..37b30165 100644
--- a/client/asset/eth/eth.go
+++ b/client/asset/eth/eth.go
@@ -370,9 +370,9 @@ func (eth *ExchangeWallet) balance() (*asset.Balance, error) {
                return nil, err
        }
 
-       locked := eth.locked + eth.redemptionReserve + dexeth.WeiToGwei(bal.PendingOut)
+       locked := eth.locked + eth.redemptionReserve
        return &asset.Balance{
-               Available: dexeth.WeiToGwei(bal.Current) - locked,
+               Available: dexeth.WeiToGwei(bal.Current) - locked - dexeth.WeiToGwei(bal.PendingOut),
                Locked:    locked,
                Immature:  dexeth.WeiToGwei(bal.PendingIn),
        }, nil

This reminds me that the addFees closure in (*nodeClient).balance needs to be fixed w.r.t. the incorrect dynamic vs. legacy txn detection we discussed in #1447 (comment) with (*Backend).baseCoin