client/eth: Get receipt before checking status.

[JoeGruffins]

checkTxStatus would sometimes return a receipt on error, which caused a
panic in tests sometimes. Get the receipt separately to avoid this.

closes #1577

[chappjc]

I got the impression this was used in upcoming work: #1523 (comment)
@buck54321 you gave the thumbs up, but it wasn't clear if you meant "yeah, will be used" or "right, will remove".
EDIT: oh, yeah, used: https://github.com/decred/dcrdex/pull/1399/files#diff-671406d18db2f8e1a281545be05fcdefd789b63614398d678d38e2b2814cf788R2526

[chappjc]

NVM, I see it used in #1399

[JoeGruffins]

Oh shoot. Well... I'll go comment over there and convert this to draft for the time being.

[JoeGruffins]

Forgot to comment on this. Fixing here.

[chappjc]

OK, but could you point out the panic. At a glance I don't see what incorrect logic was there.

[JoeGruffins]

could you point out the panic.

dcrdex/client/asset/eth/nodeclient.go

Lines 383 to 400 in e500d1c

	func (n *nodeClient) checkTxStatus(ctx context.Context, tx *types.Transaction, txOpts *bind.TransactOpts) (*types.Receipt, error) {
	// It appears the receipt is only accessible after the tx is mined.
	receipt, err := n.transactionReceipt(ctx, tx.Hash())
	if err != nil {
	return nil, fmt.Errorf("error getting transaction receipt: %v", err)
	}
	if receipt.Status != types.ReceiptStatusSuccessful {
	return receipt, fmt.Errorf("transaction status failed")
	}
	if receipt.GasUsed > txOpts.GasLimit {
	return receipt, fmt.Errorf("gas used, %d, appears to have exceeded limit of %d", receipt.GasUsed, txOpts.GasLimit)
	}
	return receipt, nil
	}

Imagine n.transactionReceipt fails here, as it usually does when testing on testnet. We get an error and nil receipt return, which is good. The second two errors will return a populated receipt and an error.

Now here in tests:

dcrdex/client/asset/eth/nodeclient_harness_test.go

Lines 1065 to 1072 in e500d1c

	receipt, err := ethClient.checkTxStatus(ctx, tx, txOpts)
	if err != nil && test.success {
	spew.Dump(tx)
	spew.Dump(receipt)
	t.Fatalf("%s: failed transaction status: %v", test.name, err)
	} else if receipt.GasUsed > expGas {
	t.Fatalf("%s: gas used %d exceeds expected max %d", test.name, receipt.GasUsed, expGas)
	}

So if a test where test.success was false and there was an error it goes to the second condition and tries to take the gas used which causes the panic because the receipt was nil.

The test was expected to fail, but failed for an unexpected reason. I think that simply sticking to the "rule" that if error expect bad returns is best and solves the problem.