core: Check can trade with new wallet. #1055

JoeGruffins · 2021-04-19T06:41:30Z

If a user has active trades, they must be settled by a wallet that can
complete those trades. Disallow changing to a new wallet that cannot do
this.

closes #968

client/core/core_test.go

client/core/core.go

buck54321 · 2021-04-26T15:42:16Z

client/core/core.go

+					addr := trade.Trade().Address
+					owns, err := wallet.OwnsAddress(addr)


This is only the right address for the "from asset" (quote asset + buy order, or base asset + sell order). For the other asset, you could in theory check the refund address, but Core doesn't store that and it's not part of the *asset.Contract right now. Hmm. Not sure what to do.

I have to take blame for this situation, since I declined to store the refund address in the OrderMetadata and even removed access to the refund address when asset.Contract was changed from an interface to a concrete type.

I think it's now checking refund addresses should it find any. Do I have my from/to asset terminology backwards?

If a user has active trades, they must be settled by a wallet that can complete those trades. Disallow changing to a new wallet that cannot do this.

JoeGruffins · 2021-05-03T06:43:55Z

Had this strange test error once but don't think it's related to this pr, has this been seen before?

--- FAIL: TestReorg (1.01s)
    btc_test.go:1253: chain A block still mainchain for test 3:3 after reorg
2021-05-03 15:26:54.379 [ERR] TEST: CheckAddress error for btc : decoded address is of unknown format
2021-05-03 15:26:54.381 [ERR] TEST: CheckAddress error for btc 28Zpft83eov56iESWuPpV8XFLJ1b8gMZy7: checksum mismatch
2021-05-03 15:26:54.381 [ERR] TEST: CheckAddress error for btc 3GD2fSQxhkXDAW66i6JBwCqhLFSvhMNrtO: decoded address is of unknown format
2021-05-03 15:26:54.381 [ERR] TEST: CheckAddress error for btc 3GD2fSQx: checksum mismatch
FAIL
FAIL    decred.org/dcrdex/server/asset/btc      2.970s

client/core/core.go

buck54321 · 2021-05-03T22:21:35Z

client/asset/btc/btc.go

@@ -1687,6 +1687,15 @@ func (btc *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*a
 	}, nil
 }

+// RefundAddress extracts and returns the refund address from a contract.
+func (btc *ExchangeWallet) RefundAddress(contract dex.Bytes) (string, error) {


For *BCHWallet you'll need to convert with RecodeCashAddress. See e.g. (*BCHWallet).Address().

client/core/core.go

buck54321 · 2021-05-03T22:50:54Z

client/core/core.go

+							script := match.MetaData.Proof.Script
+							if len(script) == 0 {
+								continue
+							}


What if the new wallet doesn't have the balance to cover the trades? What about locked outpoints?

This has the potential to get really messy, I think. I'm not sure that we can reject changes under any circumstances without creating the potential for wedged orders. There are some changes that simply cannot be made with active orders like changing the account, and some that absolutely must be allowed if we are to completely avoid wedged orders, like changing the wallet password.

Maybe once wallets are internalized, this problem goes away? Will that bee real soon? Can consider this a bandaid?

Even when we have built-ins, remote wallets will still be an option.

We can probably get away with a simpler filter for now and punt on a comprehensive solution. But maybe just reject any changes when the wallet is the fromWallet on an active match. Don't even check the refund address, just error.

What if the new wallet doesn't have the balance to cover the trades? What about locked outpoints?

The purpose is to not allow changing to a different wallet with active trades. I think there is a change of the "toAsset" falling through if not yet matched because we don't have a refund address yet. Will the trade error out later if not enough funds?

changing the account

I see, we really need to check that an address belongs to an account, and not a wallet, to ensure the account hasn't changed?

changing the wallet password.

There shouldn't be a scenario where this blocks that.

Should I work on something more complex, or is it alright to specify this as temporary? Still guessing not an issue once wallets are internal by default.

Actually, ownsAddress does check if it's this account that owns it.

We can maybe roll with the refund address check as you have it. Better to be over-persmissive, I guess. But note that the following situation is problematic.

Our order matches as taker. Counterparty has broadcast. We have received audit request, but are waiting for confirmation before broadcasting our swap. In this case, if the user tries to reconfigure the fromWallet, even changing the account or wallet altogether, they will encounter no resistance, but will definitely error at swap time.

I wonder if we can reject changes in the specific case of being a taker in MakerSwapCast?

Still confused with to/from, but we are talking about having no refund address yet here?

You have it right in the code.You only check the refund address if trade.wallets.fromAsset.ID == waID. The refund address always comes from the fromWallet.

If my thinking is right, then answering this question should help. In what situation would we have no refund address = len(match.MetaData.Proof.Script) == 0?

I think no refund address = len(match.MetaData.Proof.Script) == 0 itself it always correct? But it's true from order status epoch up until match status MakerSwapCast for the maker and TakerSwapCast for the taker?

Looks like maybeDifferentWallet resolves my concerns. Will retest and approve.

JoeGruffins · 2021-05-04T01:31:24Z

Sorry for that rebase, commit message was wrong.

chappjc · 2021-05-12T14:09:49Z

client/core/core.go

+						}
+						// Assume all refund addresses
+						// are owned by the new
+						// wallet if one is.
+						return nil
+					}
+					// If we did not find a refund address,
+					// we cannot be sure that this is the
+					// same wallet.
+					//
+					// TODO: Implement a way to check that
+					// these accounts are the same or not.


Epic indent here! 😄 I think I might have made a package level function or dexConnection method taking an xcWallet instead of a closure, but this is fine.

JoeGruffins marked this pull request as ready for review April 19, 2021 06:44

JoeGruffins commented Apr 19, 2021

View reviewed changes

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

JoeGruffins commented Apr 19, 2021

View reviewed changes

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

buck54321 requested changes Apr 26, 2021

View reviewed changes

JoeGruffins added 2 commits May 3, 2021 11:31

core: Check can trade with new wallet.

bedca3e

If a user has active trades, they must be settled by a wallet that can complete those trades. Disallow changing to a new wallet that cannot do this.

Fix grammar.

fea48b0

JoeGruffins force-pushed the checkcantrade branch from 2589e89 to 76f72e5 Compare May 3, 2021 06:35

Also check refund addresses.

76f72e5

buck54321 requested changes May 3, 2021

View reviewed changes

JoeGruffins force-pushed the checkcantrade branch 2 times, most recently from 1620a9e to 6cd26d8 Compare May 4, 2021 01:30

Error if fromAsset and no refund addresses.

6cd26d8

buck54321 approved these changes May 4, 2021

View reviewed changes

JoeGruffins mentioned this pull request May 5, 2021

client/asset: handle gap limits #215

Closed

chappjc approved these changes May 12, 2021

View reviewed changes

chappjc reviewed May 12, 2021

View reviewed changes

chappjc merged commit 3fa59cf into decred:master May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Check can trade with new wallet. #1055

core: Check can trade with new wallet. #1055

JoeGruffins commented Apr 19, 2021

buck54321 Apr 26, 2021

JoeGruffins May 3, 2021

JoeGruffins commented May 3, 2021

buck54321 May 3, 2021

buck54321 May 3, 2021

JoeGruffins May 3, 2021

buck54321 May 3, 2021

JoeGruffins May 3, 2021 •

edited

Loading

JoeGruffins May 4, 2021

buck54321 May 4, 2021 •

edited

Loading

JoeGruffins May 4, 2021

buck54321 May 4, 2021

JoeGruffins May 4, 2021

buck54321 May 4, 2021

JoeGruffins commented May 4, 2021

chappjc May 12, 2021

		addr := trade.Trade().Address
		owns, err := wallet.OwnsAddress(addr)

core: Check can trade with new wallet. #1055

core: Check can trade with new wallet. #1055

Conversation

JoeGruffins commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins commented May 3, 2021

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

JoeGruffins May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 May 4, 2021 • edited Loading

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

JoeGruffins commented May 4, 2021

Choose a reason for hiding this comment

JoeGruffins May 3, 2021 •

edited

Loading

buck54321 May 4, 2021 •

edited

Loading