eth/client: Swap #1248

martonp · 2021-10-23T11:44:31Z

This diff implements the Swap function in the eth client wallet. In addition, the eth client wallet no longer does gas estimations using the eth client and uses hardcoded values instead. Gas estimations do not vary unless the eth client is updated, and these changed will be caught by the harness tests, so it is safe to just use the hardcoded values.

Part of #1154

chappjc · 2021-11-14T16:40:19Z

client/asset/eth/eth.go

+		GasTipCap: big.NewInt(GasTipCap),
+		Context:   eth.ctx}
+
+	tx, err := eth.node.initiate(&opts, eth.networkID, initiations)


Do we have a bigger problem with EIP-1559. If we lock value (the "change" amount) based on getInitGas and swaps.FeeRate (the GasFeeCap), then tx gets mined using some unknown lower gas cost, the actual amount available is something else.
I suppose this just means we'll have more locked than is necessary until the order is fully executed and the change is returned?

That's right. We could just use a legacy transaction and all the funds could be used up and this issue wouldn't exist, but I think we might as well let the user save a little bit.

OK, we'll use an eip 1559 txn, but let's doc the unknown actual cost issue somewhere in this function.

chappjc · 2021-11-14T16:43:12Z

client/asset/eth/eth.go

+		eth.lockFunds(asset.Coins{change})
+	}
+
+	feesUsed := swaps.FeeRate * initGas


We have initGas * swaps.FeeRate above when calculating totalUsedValue, so how about we assign it to a variable like maxPossibleFee to convey it's meaning better in both places.

JoeGruffins

Looks good. Could you make the commit messages longer? Maybe comment on the need to remove getInitGas and some other stuff this does.

JoeGruffins · 2021-11-25T06:35:02Z

client/asset/eth/eth.go

@@ -273,13 +272,18 @@ func NewWallet(assetCFG *asset.WalletConfig, logger dex.Logger, network dex.Netw
 				len(accounts))
 	}

+	networkID, err := getNetworkID(network)
+	if err != nil {
+		return nil, fmt.Errorf("NewWallet: unable to get netowrk ID: %w", err)


netowrk -> network

JoeGruffins · 2021-11-25T06:40:04Z

client/asset/eth/eth.go

+		return nil, errors.New("coin id must be amount coin id")
+	}
+
+	if !bytes.Equal(amountCoinID.Address.Bytes(), eth.acct.Address.Bytes()) {


I believe you can compare the common.Address type with ==

JoeGruffins · 2021-11-25T06:59:14Z

client/asset/eth/eth.go

@@ -506,14 +453,18 @@ func (*ExchangeWallet) PreRedeem(req *asset.PreRedeemForm) (*asset.PreRedeem, er

 // coin implements the asset.Coin interface for ETH
 type coin struct {
-	id srveth.AmountCoinID
+	id dex.Bytes


Maybe just my opinion but I think srveth.CoinID would be more appropriate, because it's easy to type check if this is meant to hold different types now.

I'll leave this part for now, the previous agreement was to just send the transaction hash, but I'm thinking it would be better to send the entire TxCoinID including the index. I commented over here:
#1154 (comment)

JoeGruffins · 2021-11-25T07:07:12Z

client/asset/eth/eth.go

+// SignedRefund returns an empty byte array. ETH does not support a pre-signed
+// redeem script becuase the nonce needed in the transaction can not be previously
+// determined. We will need to come up with a different way to support manual
+// refunds in ETH.


Could you add the TODO: tag to this somewhere? That makes it easier to find all the todo's later.

I just made in issue instead.

JoeGruffins · 2021-11-25T07:10:02Z

client/asset/eth/eth.go

+// Swap sends the swaps in a single transaction. The Receipts returned can
+// be used to refund a failed transaction. The fees used returned are the max


Maybe the Receipts comment is wrong for eth. They cannot be used to refund, because the the signature will need to include the next nonce for the account to be accepted by the network. Right? And those must be in ascending order incrementing by one for each mined transaction.

Yes that's right, the comment is wrong.

JoeGruffins · 2021-11-25T07:34:42Z

client/asset/eth/eth.go

+				RefundTimestamp: big.NewInt(0).SetUint64(contract.LockTime),
+				SecretHash:      secretHash,
+				Participant:     address,
+				Value:           big.NewInt(0).SetUint64(contract.Value),


I think this need to be converted to wei from gwei here correct? I believe all uint64 values the server deals with are gwei, and the actual contract wants the basic unit.

Yes this was wrong.

JoeGruffins · 2021-11-25T07:39:28Z

client/asset/eth/eth.go

+		Value:     big.NewInt(0).SetUint64(totalContractValue),
+		GasFeeCap: big.NewInt(0).SetUint64(swaps.FeeRate),


Don't these also need to be converted from gwei to wei? I could be wrong, just let me know.

Yes they do.

JoeGruffins · 2021-11-25T07:49:40Z

client/asset/eth/eth_test.go

@@ -108,16 +119,34 @@ func (n *testNode) syncProgress(ctx context.Context) (*ethereum.SyncProgress, er
 func (n *testNode) pendingTransactions(ctx context.Context) ([]*types.Transaction, error) {
 	return nil, nil
 }
-func (n *testNode) initiate(opts *bind.TransactOpts, netID int64, initiations []swap.ETHSwapInitiation) (*types.Transaction, error) {
-	return nil, nil
+func (n *testNode) initiate(opts *bind.TransactOpts, netID int64, initiations []dexeth.ETHSwapInitiation) (*types.Transaction, error) {


Maybe worth noting that this is not concurrent safe because it sets a few values, although the real initiate probably is. It's probably fine that way.

Yeah in the real one only the locked funds are updated, and those are protected by a mutex. The tests aren't expected to be concurrent safe are they?

That depends if they are used concurrently. :p

To ensure the product code does not have data races, which have security implications in addition to the potential to cause incorrect behavior and even to panic the app (e.g. concurrent map writes), we run tests with the race detector. If there is no concurrent use of the method or the mutated fields, it doesn't need a sync mechanism, but otherwise it would.

As an aside, having the race detector on with tests helps us be acutely aware of the access patterns of the product code using these stubs. In many cases we're better off rethinking design than slapping a mutex on everything. In particular, I'd think these methods don't need synchronization mechanisms (under what circumstances would they be called concurrently or the mutated fields accessed concurrently?). A common anti-pattern we face is each test case not cleaning up after itself, and subsequent tests causing data races. Meddling in internals too much is another one. It's easy to ignore these issue without the race detector. I know you weren't suggesting to turn it off though.

I'm always in favor of designing code, both product and test, in such a way that it is lock-free. That's obv not always possible, just saying.

Makes sense. As long as we're not reusing a testNode between tests, we'll be good.

buck54321

Obviously a lot going to conflict with #1301, but this was created first, so I can handle the rebase if this is ready before.

buck54321 · 2021-11-28T12:48:36Z

client/asset/eth/eth.go

+	suggestedGasTipCap, err := eth.node.suggestGasTipCap(eth.ctx)
+	if err != nil {
+		return fail(fmt.Errorf("Swap: failed to get suggested gas tip cap %w", err))
+	}


I still haven't figured out how exactly this tip is evaluated, but my assumption is that we'll need to do some math with the dex.Asset.MaxFeeRate in order to guarantee that the transaction comes in under target.

GasFeeCap will always be the maximum fee used. The actual gas used will be BaseFee + Tip. The tip will only be less than the the GasTipCap if for example BaseFee=99, GasFeeCap=100, and GasTipCap=2. In this case, only 1 gwei will be spent for the tip.

My plan here was to have 2 be the default GasTipCap and the server can validate that clients put at least 2. Generally all wallets just use 1 for the GasTipCap though, 2 is for more safety and expediency. If the node recommends that the GasTipCap should be higher, then a higher one is used, but the dex.Asset.MaxFeeRate will still never be exceeded.

buck54321 · 2021-11-28T13:03:06Z

client/asset/eth/eth_test.go

+	swaps = asset.Swaps{
+		Inputs:     inputs,
+		Contracts:  contracts,
+		FeeRate:    200,
+		LockChange: false,
+	}
+	testSwap("one contract, don't lock change", swaps, false)


Probably OK, but we really shouldn't need a change coin if LockChange is false. core shouldn't require it. It does look like we are returning the change coin for other assets. I don't think that's on purpose.

buck54321 · 2021-11-28T13:09:49Z

server/asset/eth/common.go

+func CreateAmountCoinID(address common.Address, amount uint64) *AmountCoinID {
+	var nonce [8]byte
+	copy(nonce[:], encode.RandomBytes(8))
+	return &AmountCoinID{
+		Address: address,
+		Amount:  amount,
+		Nonce:   nonce,
+	}
+}
+


As per convo elsewhere, we will probably only use this client-side. Can move and unexport here or separately. Up to you.

I'll do it separately.

buck54321 · 2021-11-28T13:12:04Z

client/asset/eth/eth.go

+// Coin returns the coin used to fund the swap.
+func (r *swapReceipt) Coin() asset.Coin {
+	return &coin{
+		value: r.value,
+		id:    dex.Bytes(r.txHash[:]),
+	}
+}


Probably just the txHash here.

buck54321 · 2021-11-28T13:13:37Z

client/asset/eth/eth.go

+
+// Contract returns the swap's secret hash.
+func (r *swapReceipt) Contract() dex.Bytes {
+	return dex.Bytes(r.secretHash[:])


[]byte is about the only thing in Go that you can cast implicitly. Just return r.secretHash[:].

buck54321 · 2021-11-28T13:16:25Z

client/asset/eth/eth.go

+
+// String returns a string representation of the swapReceipt.
+func (r *swapReceipt) String() string {
+	return fmt.Sprintf("tx hash: %x, secret hash: %x", r.txHash, r.secretHash)


How do you feel about wrapping the output in a delimiter?

return fmt.Sprintf("{ tx hash: %x, secret hash: %x }", r.txHash, r.secretHash)

Sounds good to me.

buck54321 · 2021-11-28T13:16:56Z

client/asset/eth/eth.go

+}
+
+// SignedRefund returns an empty byte array. ETH does not support a pre-signed
+// redeem script becuase the nonce needed in the transaction cannot be previously


buck54321 · 2021-11-28T13:17:09Z

client/asset/eth/eth.go

+// redeem script becuase the nonce needed in the transaction cannot be previously
+// determined.
+func (*swapReceipt) SignedRefund() dex.Bytes {
+	return dex.Bytes{}


We'll have to do something, but we can punt for now.

Issue: #1311

buck54321 · 2021-11-28T13:18:38Z

client/asset/eth/eth_test.go

+			if len(receipt.Contract()) != srveth.SecretHashSize {
+				t.Fatalf("%v: expected length of contract to be %v but got %v",
+					testName, srveth.SecretHashSize, len(receipt.Contract()))
+			}


Length check is implicit in bytes.Equal.

This diff implements the Swap function in the eth client wallet. In addition, the eth client wallet no longer does gas estimations using the eth client and uses hardcoded values instead. Gas estimations do not vary unless the eth client is updated, and these changed will be caught by the harness tests, so it is safe to just use the hardcoded values.

martonp marked this pull request as draft October 23, 2021 12:01

martonp force-pushed the ethSwap branch from 8c2216d to ee847d0 Compare November 3, 2021 16:50

chappjc added the ETH label Nov 12, 2021

martonp mentioned this pull request Nov 14, 2021

[for discussion] eth: locked amount accounting updates #1289

Closed

chappjc reviewed Nov 14, 2021

View reviewed changes

martonp force-pushed the ethSwap branch 2 times, most recently from 4019cab to d4600fe Compare November 16, 2021 11:14

martonp mentioned this pull request Nov 16, 2021

eth/client: Replace gas estimates with hardcoded values #1290

Closed

martonp force-pushed the ethSwap branch from d4600fe to a621a40 Compare November 16, 2021 12:49

martonp force-pushed the ethSwap branch from a621a40 to 9d7fc81 Compare November 24, 2021 14:51

martonp marked this pull request as ready for review November 24, 2021 14:58

JoeGruffins reviewed Nov 25, 2021

View reviewed changes

martonp force-pushed the ethSwap branch from 9d7fc81 to 4606674 Compare November 25, 2021 15:48

buck54321 requested changes Nov 28, 2021

View reviewed changes

chappjc added this to the 0.5 milestone Nov 28, 2021

martonp added 2 commits November 29, 2021 15:27

Updates based on review.

b525e86

martonp force-pushed the ethSwap branch from 4606674 to b525e86 Compare November 29, 2021 08:46

chappjc approved these changes Nov 29, 2021

View reviewed changes

chappjc merged commit 902495d into decred:master Nov 29, 2021

chappjc removed this from the 0.5 milestone Aug 26, 2022

martonp deleted the ethSwap branch December 20, 2022 22:10

		// Swap sends the swaps in a single transaction. The Receipts returned can
		// be used to refund a failed transaction. The fees used returned are the max

		Value: big.NewInt(0).SetUint64(totalContractValue),
		GasFeeCap: big.NewInt(0).SetUint64(swaps.FeeRate),

eth/client: Swap #1248

eth/client: Swap #1248

Conversation

martonp commented Oct 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

JoeGruffins 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

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 Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Nov 25, 2021 • edited Loading

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

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 Oct 23, 2021 •

edited

Loading

chappjc Nov 14, 2021 •

edited

Loading

martonp Nov 25, 2021 •

edited

Loading

chappjc Nov 25, 2021 •

edited

Loading