client/eth: refactor eth client #1301

buck54321 · 2021-11-16T20:06:47Z

The client/asset/eth package was using go-ethereum like an RPC client. That's
not crazy, considering we have still an *rpc.Client, but in reality,
there is no RPC server, and the client is just a shortcut to RPC handlers
that call functions that we already have access to through e.g.
*les.LightEthereum or *keystore.KeyStore. This PR removes all
CallContext calls in favor of calling the underlying methods directly.
There are probably more *rpc.Client calls we could bypass as well.

Moves some common types to dex/networks/eth. As a general rule, if
the client/asset/ imports from server/asset, we should just move it
to dex/networks.

Creates the Contractor interface, which serves as a clear boundary
between dex-space and abigen-space. Implements contract versions.

Switches to EIP1559 transactions and removes hard-coded base fee rate
from harness. Due to this change, some live tests are still busted because
balance changes need to be fixes. Leaving in draft until that's done.

I realize this a big change in an area where we have a lot of hands right now,
but I'd rather do it earlier than later.

buck54321 · 2021-11-16T20:12:14Z

client/asset/eth/nodeclient.go

+	nonce, err := n.ec.NonceAt(ctx, n.creds.addr, nil)
+	if err != nil {
+		return nil, fmt.Errorf("error getting nonce: %v", err)
+	}


It's not impossible that this is insufficient to guarantee a unique nonce. I haven't verified that this is incremented before the transaction is mined.

buck54321 · 2021-11-16T21:58:39Z

This PR removes all CallContext calls in favor of calling the underlying methods directly. There are probably more *rpc.Client calls we could bypass as well.

I went ahead and removed the rest of the ethclient.Client => rcp.Client calls. No more pointless encode->decodes. We could get rid of the ethclient.Client altogether if it wasn't for a single function EstimateGas in an internal package that I really don't want to copy.

buck54321 · 2021-11-16T22:03:32Z

client/asset/eth/nodeclient.go

 	return b, weiToGwei(tip), nil
 }

 // getCodeAt retrieves the runtime bytecode at a certain address.
-func (n *nodeClient) getCodeAt(ctx context.Context, contractAddr *common.Address) ([]byte, error) {
-	bn, err := n.ec.BlockNumber(ctx)
+func (n *nodeClient) getCodeAt(ctx context.Context, contractAddr common.Address) ([]byte, error) {


This one's busted too and I don't know why yet. It's busted if I use the ethclient.Client method too, but that's not ruling out some other mistake I made.

Works fine for me.. maybe try restarting the eth harness?

Hmm. I'm still failing. What version of geth are you running?

Nevermind. I found the problem.

What was wrong?

I think I've forgotten now. Sorry I missed this question.

martonp

This looks great.
I say let's get the #1262 and #1274 in first since that would finalize the contract, and then put this one before any other ones.

client/asset/eth/eth.go

client/asset/eth/nodeclient.go

martonp · 2021-11-17T07:26:24Z

dex/networks/eth/params.go

+	// called and the gas used for each. (◍•﹏•)
+	InitGas = 158000 // gas
+
+	RedeemGas = 63000 // gas


Since the contracts are versioned now, these gas estimates will also need to be versioned.

buck54321 · 2021-11-17T15:10:52Z

client/asset/eth/eth.go

+func (eth *ExchangeWallet) Withdraw(addr string, value, _ uint64) (asset.Coin, error) {
+	_, err := eth.sendToAddr(common.HexToAddress(addr), value)


We should revisit this at some point. If a user tries to withdraw their entire balance, it will fail and they'll lose the tx fees, I believe.

Yikes, how does one sweep an ETH wallet completely, leaving nothing left if gas doesn't eat up whatever was allocated to gas?

Maybe the wallet won't allow the transaction to be sent the way I've described. I'm not certain.

for ref: https://www.reddit.com/r/ethdev/comments/pits6s/sweep_all_eth_using_dynamic_fees_eip1559_and/
EIP-1559 does make this a headache if not impossible to do without leaving dust

Out of curiosity I tried out a popular eth wallet and despite being eip-1559 aware, it's support for sweep is non-existent. A tx draft wavers between leaving dust and having insufficient funds, while the base fee fluctuates.

Haha, basically the way to sweep it is to give the dust to the miners.

For the original issue of the failing transaction, I would think that the node would have a check that refuses to submit a transaction where the gasLimit*gasPrice + value > balance, but I haven't tried it out yet.

buck54321 · 2021-11-17T16:31:20Z

client/asset/eth/nodeclient_harness_test.go

+
+			state := dexeth.SwapStep(swap.State)
+			if test.success && state != dexeth.SSInitiated {
+				t.Fatalf("unexpected swap state for test %v: want %s got %s", test.name, dexeth.SSInitiated, state)


I cannot figure out why this isn't passing. I'm verifying the secret hash matches thoughout. There are no errors. I can see the transaction with eth.getTransaction afterwards, and I can see the secret hash in the tx data, but the ETHSwapSwap is completely zeroed.

I've been trying to figure it out what's wrong.. haven't had any luck yet.

I also saw that you removed the addSignerToOpts calls from the functions that submit transactions (initiate, redeem, etc.) and you're calling them from the test code, but in some cases you forgot to call it. What's wrong with this going into intiate and redeem?

~~Here's a clue.~~

https://github.com/decred/dcrdex/compare/master...buck54321:fail-init-real-balance?expand=1

~~That fails too.~~

Nevermind. That failed because I wasn't setting the TransactOpts value

This branch is failing for > 1 swap. Maybe another manifestation of #1304?

Update: I've got everything passing except for TestReplayAttack.

Nice, what was wrong? I was getting a headache from that.

I think I was handling a *big.Int wrong. Not making a copy when I should have or something.

buck54321 · 2021-11-17T17:30:35Z

Rebased and un-renamed the rpcclient*.go files back to the original names in an attempt to provide a usable diff for review. Will rename again later.

I'm still banging my head against the wall on the swap state issue, but I'm gonna step away for a minute. @martonp, if you have a sec to take a look, I'd really appreciate it.

JoeGruffins · 2021-11-23T09:10:38Z

dex/testing/eth/create-node.sh

@@ -82,7 +82,6 @@ if [ "${CHAIN_ADDRESS}" != "_" ]; then

 [Eth.Miner]
 Etherbase = "0x${CHAIN_ADDRESS}"
-GasPrice = 81000000000


Yeah, sorry I couldn't answer you somewhere where you asked about this, I don't know. I'm not up to speed on the base fee, thing. The miners can set the gas price over time is all I think I know. Also, I think geth complained without setting this, maybe it doesn't now.

JoeGruffins · 2021-11-23T12:42:31Z

So, the reentry test, it doesn't look like it's communicating with the contract. The balances aren't the simnet's contract's balance, and the initiations aren't being sent to the contract. Or, did you just fix it?

client/asset/eth/rpcclient.go

buck54321 · 2021-11-24T20:27:13Z

So, the reentry test, it doesn't look like it's communicating with the contract. The balances aren't the simnet's contract's balance, and the initiations aren't being sent to the contract. Or, did you just fix it?

Thanks, @JoeGruffins. The contract address must've been part of it. It's partly passing now, but still seeing an unexpected change in the contract balance. Still missing something.

JoeGruffins · 2021-11-25T06:22:57Z

Passing for me. Please see the rpcclient.balance comment I made. *addr

JoeGruffins

The Contractor interface looks good.

I'm not completely sold on the rpcclient changes. I think it was less work for us before. But it's fine I guess.

Also, some conflicts with #1309 so hopefully mine goes in first :P

JoeGruffins · 2021-11-26T04:56:01Z

client/asset/eth/rpcclient.go

+	node, err := prepareNode(&nodeConfig{
+		net:    net,
+		appDir: dir,
+		logger: log.SubLogger("NODE"),


It looks like you already make this logger at the call site.

client/asset/eth/rpcclient.go

JoeGruffins · 2021-11-26T05:08:13Z

client/asset/eth/rpcclient.go

 // balance gets the current balance of an address.
-func (c *rpcclient) balance(ctx context.Context, addr *common.Address) (*big.Int, error) {
-	return c.ec.BalanceAt(ctx, *addr, nil)
+func (n *nodeClient) balance(ctx context.Context) (*big.Int, error) {
+	return n.addressBalance(ctx, n.creds.addr)
+}


This is the balance for the wallet only correct? Can change doc.

client/asset/eth/rpcclient.go

JoeGruffins · 2021-11-26T05:12:22Z

client/asset/eth/rpcclient.go

+	return status != "Unlocked"
+}
+
+// sendToAddr sends funds from acct to addr.


acct is no longer a parameter, so from the internal account maybe?

JoeGruffins · 2021-11-26T05:33:02Z

client/asset/eth/rpcclient.go

+	}), n.chainID)
+
+	if err != nil {
+		return nil, fmt.Errorf("signing error: %v", err)


Depending on whether eth increments the nonce, this could cause problems if erroring if it does increment. I guess it would increment maybe when the tx is sent though? In that case it would be fine to let this error. But if it already marked it as used I don't know if it would get stuck trying to send transactions/swaps with an out of order nonce.

See other comment about nonce too, but go-ethereum seems to operate with the assumption that GetPoolNonce is accurate as soon as SendTx returns.

client/asset/eth/rpcclient.go

dex/networks/eth/contractor.go

JoeGruffins · 2021-11-26T06:39:08Z

dex/networks/eth/contractor.go

+			Secret:     secret,
+			SecretHash: sha256.Sum256(secret[:]),


These couldn't be fake? I think there was some discussion about this but I forget. EstimateInit is using fake secret hashes.

Apparently not.

It was wrong with random bytes? I would expect only the length to be important, not the content.

client/asset/eth/eth.go

buck54321 · 2021-11-26T17:50:27Z

client/asset/eth/eth.go

-	transactionReceipt(ctx context.Context, txHash common.Hash) (*types.Receipt, error)
-	sendTransaction(ctx context.Context, tx map[string]string) (common.Hash, error)
+	connect(ctx context.Context) error
+	initiate(ctx context.Context, contracts []*asset.Contract, maxFeeRate uint64, contractVer uint32) (*types.Transaction, error)


The contractVer doesn't need to map precisely to the dex.Asset.Version, but as long as we make a rule that a change in contract always accompanies a bump in dex.Asset.Version, then we can find a scheme to get the contract version from the asset version and we don't need any new data.

chappjc

A few quick comments, will have a proper review later today.

client/asset/eth/rpcclient.go

dex/networks/eth/contractor.go

chappjc · 2021-11-26T18:43:50Z

dex/networks/eth/contractor.go

+// contractorv0 is the Contractor for contract version 0.
+// Redeem and Refund methods of ETHSwap already have suitable return types.
+type contractorV0 struct {
+	*ETHSwap


We should save the following suggestion for another day, but since ETHSwap is the v0 contract API compiled from it's solidity, we'll probably want to move dex/networks/eth/contract.go -> dex/networks/eth/contracts/v0.go or dex/networks/eth/contracts/apis/v0.go

chappjc

A bit more. Sorry I'm reviewing this in 20 minute stints.

dex/networks/eth/params.go

client/asset/eth/eth.go

dex/networks/eth/params.go

chappjc · 2021-11-27T17:26:39Z

dex/networks/eth/params.go

+	// state of a swap.
+	SSNone SwapStep = iota
+	// SSInitiated indicates that the swap has been initiated.
+	SSInitiated


Just a note that like contract.go, these SwapSteps are potentially contract version-specific and we'll have to address that when we move contract.go into subfolders for the different contract api versions as described in my other comment. Same deal with SwapState. The v0 Contractor uses these types, but it's not clear they will be the same for all future contract versions.

We're taking a leap with the definition of Swap(ctx context.Context, secretHash [32]byte) (*SwapState, error) in the Contractor interface that the swap state will always have the same definition, or that we can shoe-horn any version's swap "state" into this definition. I hope that's the case.

The SwapState is already a mirror of the ETHSwapState, which is the contract-specific struct. The Contractor is translating. SwapStep and SwapState are meant to defined for the needs of the DEX protocol, so in theory, shouldn't need modification with future contract changes. I wouldn't rule out that we do need to modify this stuff someday though, esp. SwapState, but it's a struct so we can always make it work, I think.

Understood that it's supposed to be a version-independent translation of ETHSwapState. I'm just crossing my fingers that it will always be adequate (all fields applicable and/or sufficient).
But meh, we can change the struct in the future so a translation is good.

chappjc · 2021-11-27T17:39:33Z

client/asset/eth/eth.go

-	_, err := eth.sendToAddr(common.HexToAddress(addr),
-		bigVal.Mul(bigVal, gweiFactorBig), big.NewInt(0).SetUint64(defaultGasFee))
+func (eth *ExchangeWallet) Withdraw(addr string, value, _ uint64) (asset.Coin, error) {
+	_, err := eth.node.sendToAddr(eth.ctx, common.HexToAddress(addr), value)


We seem to be deviating from the withdraw semantics we have been following with the other assets. That is, this gives to addr a certain value, whereas with the other assets we've had it reduce the wallet balance by value, giving addr something less than value by network fees. I'm basing this comment on my understanding of (*nodeClient).sendToAddr.
It does not seem to be an issue with this PR, but something we should resolve before production.

Continuation of previous conversation, I think, but yes. We need to address this.

chappjc

Looks good. Holding out for #1248, and for the outstanding reviewers to circle back.

JoeGruffins · 2021-11-29T01:53:16Z

dex/networks/eth/params.go

+// GweiToWei converts *big.Int Wei to uint64 Gwei.
+func WeiToGwei(v *big.Int) uint64 {
+	return new(big.Int).Div(v, BigGweiFactor).Uint64()
+}


The original value could potentially be negative or overflow the returned uint64. There is also already a ToGwei in cointypes.go

martonp

LGTM

The client/asset/eth package was using geth like an RPC client. That's not crazy, considering we have still an *rpc.Client, but in reality, there is no RPC server, and the client is just a shortcut to RPC handlers that call functions that we already have access to through e.g. *les.LightEthereum or *keystore.KeyStore. This removes most "rpc" calls in favor of calling the underlying methods directly. Moves some common types to dex/networks/eth. As a general rule, if the client/asset/ imports from server/asset, we should just move it to dex/networks. Creates the Contractor interface, which serves as a clear boundary between dex-space and abigen-space. This also enables clean upgrades, implemented here. Remove extraneous methods from the ethFetcher interface. Convert Contractor redemption gas estimation to batch. Move node.Node and KeyStore initialization to nodeClient constructor and remove unused ExchangeWallet fields. Move Contractor to client/asset/eth and unexport. Add params tests. Use new(big.Int) instead of bit.NewInt(0). Version the gas values. rename rpcclient files add nonce-send mutex fix atomic whoopsie

martonp · 2021-11-30T10:27:48Z

client/asset/eth/eth_test.go

@@ -1085,58 +941,9 @@ func TestSwap(t *testing.T) {
 					testName, receipt.Contract(), contract.SecretHash)
 			}

-			if !bytes.Equal(node.lastInitiation.hash.Bytes(), receipt.Coin().ID()) {


Why did you remove these? Shouldn't we be checking that initiate is called with correct arguments?

Good call. You probably see the PR, but I'll link for all: #1318

buck54321 commented Nov 16, 2021

View reviewed changes

buck54321 force-pushed the eth-contract-versioning branch from f4b38e9 to ff5f9ef Compare November 16, 2021 20:15

buck54321 commented Nov 16, 2021

View reviewed changes

martonp reviewed Nov 17, 2021

View reviewed changes

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

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

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

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

martonp reviewed Nov 17, 2021

View reviewed changes

buck54321 commented Nov 17, 2021

View reviewed changes

buck54321 force-pushed the eth-contract-versioning branch 2 times, most recently from 196a375 to 145481a Compare November 17, 2021 17:23

buck54321 force-pushed the eth-contract-versioning branch 3 times, most recently from 1cb84e8 to c2b1f43 Compare November 18, 2021 18:04

This was referenced Nov 18, 2021

server/asset/eth: update server coin ID parsing #1307

Merged

server/eth: Implement ValidateContract. #1273

Merged

chappjc modified the milestone: 0.4 Nov 22, 2021

chappjc added the ETH label Nov 22, 2021

JoeGruffins reviewed Nov 23, 2021

View reviewed changes

buck54321 force-pushed the eth-contract-versioning branch from c2b1f43 to b975ead Compare November 23, 2021 11:12

JoeGruffins reviewed Nov 23, 2021

View reviewed changes

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

buck54321 force-pushed the eth-contract-versioning branch from b975ead to 5a7d8cf Compare November 24, 2021 20:24

buck54321 force-pushed the eth-contract-versioning branch from dbc278b to 4bf6adc Compare November 25, 2021 17:58

buck54321 marked this pull request as ready for review November 25, 2021 17:58

JoeGruffins reviewed Nov 26, 2021

View reviewed changes

chappjc self-requested a review November 26, 2021 16:34

chappjc mentioned this pull request Nov 26, 2021

eth: compare parsed address in OwnsAddress #1306

Merged

buck54321 commented Nov 26, 2021

View reviewed changes

chappjc requested changes Nov 26, 2021

View reviewed changes

chappjc reviewed Nov 26, 2021

View reviewed changes

dex/networks/eth/contractor.go Outdated Show resolved Hide resolved

chappjc reviewed Nov 26, 2021

View reviewed changes

chappjc requested changes Nov 27, 2021

View reviewed changes

buck54321 force-pushed the eth-contract-versioning branch 4 times, most recently from 19d93a0 to 616fd74 Compare November 28, 2021 21:51

chappjc approved these changes Nov 28, 2021

View reviewed changes

chappjc added this to the 0.5 milestone Nov 28, 2021

JoeGruffins mentioned this pull request Nov 29, 2021

networks/eth: Move generated files to swap. #1309

Merged

JoeGruffins approved these changes Nov 29, 2021

View reviewed changes

martonp approved these changes Nov 29, 2021

View reviewed changes

buck54321 force-pushed the eth-contract-versioning branch from 695ac56 to 920fc78 Compare November 29, 2021 21:12

chappjc merged commit b66b579 into decred:master Nov 29, 2021

martonp reviewed Nov 30, 2021

View reviewed changes

This was referenced Nov 30, 2021

client/asset/eth: re-implement translation tests for abigen types #1318

Merged

eth/client: Redeem #1302

Merged

eth/client: Refund #1325

Merged

		func (eth *ExchangeWallet) Withdraw(addr string, value, _ uint64) (asset.Coin, error) {
		_, err := eth.sendToAddr(common.HexToAddress(addr), value)

client/eth: refactor eth client #1301

client/eth: refactor eth client #1301

Conversation

buck54321 commented Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

buck54321 commented Nov 16, 2021

buck54321 Nov 16, 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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

buck54321 Nov 18, 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

buck54321 commented Nov 17, 2021

Choose a reason for hiding this comment

JoeGruffins commented Nov 23, 2021

buck54321 commented Nov 24, 2021 • edited Loading

JoeGruffins commented Nov 25, 2021 • edited Loading

JoeGruffins left a comment • 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

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

chappjc left a comment

Choose a reason for hiding this comment

chappjc Nov 26, 2021 • 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

Choose a reason for hiding this comment

chappjc Nov 28, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc Nov 27, 2021 • edited Loading

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

Choose a reason for hiding this comment

martonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 commented Nov 16, 2021 •

edited

Loading

buck54321 Nov 16, 2021 •

edited

Loading

chappjc Nov 17, 2021 •

edited

Loading

chappjc Nov 17, 2021 •

edited

Loading

buck54321 Nov 17, 2021 •

edited

Loading

buck54321 Nov 18, 2021 •

edited

Loading

buck54321 Nov 18, 2021 •

edited

Loading

buck54321 commented Nov 24, 2021 •

edited

Loading

JoeGruffins commented Nov 25, 2021 •

edited

Loading

JoeGruffins left a comment •

edited

Loading

chappjc Nov 26, 2021 •

edited

Loading

chappjc Nov 28, 2021 •

edited

Loading

chappjc Nov 27, 2021 •

edited

Loading