eth: Fix init and redeem. #1414

JoeGruffins · 2022-01-14T08:21:57Z

With these changes I was able to successfully swap.

A few of the current problems are pointed out.

JoeGruffins · 2022-01-14T08:23:01Z

client/asset/eth/eth.go

 // DecodeCoinID creates a human-readable representation of a coin ID for Ethereum.
 func (d *Driver) DecodeCoinID(coinID []byte) (string, error) {
+	const invalid = "<invalid coin>"
+	if len(coinID) == common.HashLength {
+		id, err := dexeth.DecodeCoinID(coinID)
+		if err != nil {
+			return invalid, err
+		}
+		return id.String(), nil
+	}
 	id, err := decodeFundingCoinID(coinID)
 	if err != nil {
-		return "<invalid coin>", err
+		return invalid, err
 	}
 	return id.String(), nil
 }


Sometimes a tx hash, and we can gauge that by length. Otherwise a funding coin.

JoeGruffins · 2022-01-14T08:23:36Z

client/asset/eth/eth.go

@@ -689,7 +697,7 @@ func (eth *ExchangeWallet) Swap(swaps *asset.Swaps) ([]asset.Receipt, asset.Coin
 		return nil, nil, 0, fmt.Errorf("unfunded contract. %d < %d", totalInputValue, totalSpend)
 	}

-	tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, swaps.FeeRate, swaps.AssetVersion)
+	tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, 200 /*swaps.FeeRate*/, swaps.AssetVersion)


Here and in redeem, this was too low. Will fix this better.

I'm not sure how to fix this. For one there is #1336 which is still my opinion. Just use the user's max fee. There is also #1372 which is a book.

I think I'll leave it in this unfinished state for this pr and let that get solved in another pr.

I'm of the opinion that the server should just set the Match.{FeeRateBase,FeeRateQuote} to the (dex.Asset).MaxFeeRate for eth at match time. The current base fee rate is not relevant to the validation of the transaction. In theory, the client could just log the MaxFeeRate at order time and persist that info and use it as an input to the Swap method. But I don't see how that makes things easier for anyone. Instead, we can just think of the Match.{FeeRateBase,FeeRateQuote} as the "validation rate", or the rate that we are going to validate in their transaction. This fits into our current architecture without changes, and the relevant values are persisted.

Agree completely with that @buck54321. Ref, your comment in my issue on the matter: #1372 (comment)

Will leave for now if that's ok.

OK, when we implement the changes discussed here and #1372 (comment) we will switch this back to swaps.FeeRate

client/asset/eth/eth.go

JoeGruffins · 2022-01-14T08:29:05Z

client/asset/eth/eth.go

+	// Time is not yet set for uninitiated swaps.
+	if swap.State == dexeth.SSNone {
+		return false, time.Time{}, asset.ErrSwapNotInitiated
+	}


Useful for not logging this as an error, as it is expected until the init is mined.

client/asset/eth/nodeclient.go

JoeGruffins · 2022-01-14T08:31:43Z

server/swap/swap.go

+		log.Infof("Secret validation failed (match id=%v, maker=%v, secret=%v)",
 			matchID, actor.isMaker, params.Secret)


%v prints hex and I think this is info. It is the fault of the user if the redemption secret is incorrect.

Good call on both accounts. I fell into the %x-with-a-Stringer pitfall that I like to remind others about.

JoeGruffins · 2022-01-14T08:34:57Z

Also, starting the node from seed every time isn't really making things faster. As commented in the nodeclient harness tests, the node has trouble figuring out the correct account nonce it seems, and always starts with zero, even if it cannot send txs using zero. Maybe there is a way fix for this, but often need a new eth harness if you really need to start over.

edit: Actually seems alright now. Maybe just need to wait for the [INF] CORE: Wallet synced for asset eth log? I have been trading fine with restored nodes today.

Also this does not touch refunds which may not work yet.

martonp

Looks good, I was able to successfully swap dcr<->eth on simnet.

buck54321

Brilliant! My first swap with eth. Do whatever with the ErrSwapNotInitiated I guess, but we need a comprehensive plan there.

client/cmd/dexc/log.go

buck54321 · 2022-01-20T15:23:13Z

client/core/trade.go

+		if err != nil && !errors.Is(err, asset.ErrSwapNotInitiated) {
+			// No need to log an error if swap not initiated as this
+			// is expected for newly made swaps involving contracts.
+			t.dc.log.Errorf("isSwappable: error getting confirmation for our own swap transaction: %v", err)
 		}


This is closely related to #1425, and ErrSwapNotInitiated is analogous to CoinNotFoundError, as far as I can tell. There was some discussion on this on Element too. On that end, chappjc decided to just return nil error and zero confs in that case. Is there a strong argument to do it differently here?

Just want to reiterate that #1425 is just a stopgap for v0.4, and if we do keep it nil when the swap isn't found (either txn with utxo assets or an entry in the contract's swaps map for eth), I suspect we'll want a found bool or similar from SwapConfirmations, with an error reserved for the case where it is actually unexpected to not find it, but of course when unconfirmed on eth it is expected since no state has changed to record the new swap (proper error semantics).

I am trying to avoid returning a false positive int the case of returning nil, and false negative in case of returning CoinNotFoundError, I guess. Not finding the swap could be a problem or it might not be. It's not that there is absolutely no problem, there may be a problem. Does that make sense? We could use the initiation tx instead of the contract instead and confidently return CoinNotFoundError like the server does.

Maybe it would be more correct to log as warn?

It depends on (a) the capabilities of the wallet backend and (b) time and blocks that have elapsed. I've beat this horse to death now, but I am adamantly opposed to logging a warning or error if it's normal for the wallet not to find the unconfirmed swap.

No need to change this now, but I think the ultimate solution involves more work in Core to be intelligent about gauging severity.

chappjc

Good to go after popping off the first two commits, rebasing and and squashing the last two.

On the client side: Allow decoding coins to discern between tx hashes and funding coin encodings. Temporarily set max fees to 200 so that they can be mined on our simnet. Return tx hashes for redeems instead of inputs because that's currently what the server needs to create swap coins. Add a new error type for uninitiated swaps. This is returned when checking swap confirmations and locktime for logging purposes so this is not considered an error for logging purposes. Add the ability to squelch eth's internal node because it is very loud on trace. For the server: Return TxData and SecretHash with contract data. TxData is the entire serialized transaction that initiates a swap.

JoeGruffins commented Jan 14, 2022

View reviewed changes

JoeGruffins changed the title ~~eth: Fix swap and redeem.~~ eth: Fix init and redeem. Jan 14, 2022

JoeGruffins mentioned this pull request Jan 17, 2022

eth: Funding coin encoding. #1392

Merged

JoeGruffins force-pushed the fixethswaps branch 3 times, most recently from 0aa37e4 to 167690d Compare January 19, 2022 08:53

JoeGruffins marked this pull request as ready for review January 19, 2022 08:58

chappjc requested review from buck54321 and martonp January 19, 2022 16:01

martonp approved these changes Jan 19, 2022

View reviewed changes

buck54321 approved these changes Jan 20, 2022

View reviewed changes

chappjc approved these changes Jan 21, 2022

View reviewed changes

JoeGruffins force-pushed the fixethswaps branch from 354918f to 7d05911 Compare January 22, 2022 03:34

chappjc merged commit ecc1f79 into decred:master Jan 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eth: Fix init and redeem. #1414

eth: Fix init and redeem. #1414

JoeGruffins commented Jan 14, 2022

JoeGruffins Jan 14, 2022 •

edited

Loading

JoeGruffins Jan 14, 2022

JoeGruffins Jan 18, 2022

buck54321 Jan 20, 2022

chappjc Jan 20, 2022

JoeGruffins Jan 21, 2022 •

edited

Loading

chappjc Jan 21, 2022

JoeGruffins Jan 14, 2022

JoeGruffins Jan 14, 2022

chappjc Jan 21, 2022

JoeGruffins commented Jan 14, 2022 •

edited

Loading

martonp left a comment

buck54321 left a comment

buck54321 Jan 20, 2022

chappjc Jan 20, 2022 •

edited

Loading

JoeGruffins Jan 21, 2022 •

edited

Loading

JoeGruffins Jan 21, 2022

chappjc Jan 21, 2022

chappjc Jan 21, 2022

chappjc left a comment

		log.Infof("Secret validation failed (match id=%v, maker=%v, secret=%v)",
		matchID, actor.isMaker, params.Secret)

eth: Fix init and redeem. #1414

eth: Fix init and redeem. #1414

Conversation

JoeGruffins commented Jan 14, 2022

JoeGruffins Jan 14, 2022 • 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 Jan 21, 2022 • 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 Jan 14, 2022 • edited Loading

martonp left a 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

chappjc Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

JoeGruffins Jan 21, 2022 • 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

chappjc left a comment

Choose a reason for hiding this comment

JoeGruffins Jan 14, 2022 •

edited

Loading

JoeGruffins Jan 21, 2022 •

edited

Loading

JoeGruffins commented Jan 14, 2022 •

edited

Loading

chappjc Jan 20, 2022 •

edited

Loading

JoeGruffins Jan 21, 2022 •

edited

Loading