ETH transfer fixes and sweep ETH addition (#481)

ethereum/consts.go

@@ -17,5 +17,5 @@ const (
 // constants that are interesting to track, but not used by swaps
 const (
 	maxSwapCreatorDeployGas = 1094089
-	maxTestERC20DeployGas   = 798286 // using long token names or symbols will increase this
+	maxTestERC20DeployGas   = 905727 // using long token names or symbols will increase this
 )

ethereum/contracts/TestERC20.sol

@@ -6,7 +6,7 @@ import {ERC20} from "./ERC20.sol";
 
 // ERC20 token for testing purposes
 contract TestERC20 is ERC20 {
-    uint8 private _decimals;
+    uint8 private immutable _decimals;
 
     constructor(
         string memory name,
@@ -38,4 +38,13 @@ contract TestERC20 is ERC20 {
     function approveInternal(address owner, address spender, uint256 value) public {
         _approve(owner, spender, value);
     }
+
+    // You can send a zero-value transfer directly to the contract address to
+    // get a 100 standard unit tokens.
+    receive() external payable {
+        mint(msg.sender, 100 * 10 ** uint(_decimals));
+        if (msg.value > 0) {
+            payable(msg.sender).transfer(msg.value);
+        }
+    }
 }

ethereum/extethclient/eth_wallet_client.go

@@ -8,6 +8,7 @@ package extethclient
 import (
 	"context"
 	"crypto/ecdsa"
+	"errors"
 	"fmt"
 	"math/big"
 	"sync"
@@ -17,6 +18,7 @@ import (
 	ethcommon "github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/ethclient"
+	"github.com/ethereum/go-ethereum/params"
 	logging "github.com/ipfs/go-log"
 
 	"github.com/athanorlabs/atomic-swap/coins"
@@ -25,7 +27,7 @@ import (
 	"github.com/athanorlabs/atomic-swap/ethereum/block"
 )
 
-var log = logging.Logger("extethclient")
+var log = logging.Logger("ethereum/extethclient")
 
 // EthClient provides management of a private key and other convenience functions layered
 // on top of the go-ethereum client. You can still access the raw go-ethereum client via
@@ -51,12 +53,26 @@ type EthClient interface {
 	Lock()   // Lock the wallet so only one transaction runs at at time
 	Unlock() // Unlock the wallet after a transaction is complete
 
-	// transfers ETH to the given address
-	// does not need locking, as it locks internally
-	Transfer(ctx context.Context, to ethcommon.Address, amount *coins.WeiAmount) (ethcommon.Hash, error)
+	// Transfer transfers ETH to the given address, nonce Lock()/Unlock()
+	// handling is done internally. The gasLimit field when the destination
+	// address is not a contract.
+	Transfer(
+		ctx context.Context,
+		to ethcommon.Address,
+		amount *coins.WeiAmount,
+		gasLimit *uint64,
+	) (*ethtypes.Receipt, error)
 
-	// attempts to cancel a transaction with the given nonce by sending a zero-value tx to ourselves
-	CancelTxWithNonce(ctx context.Context, nonce uint64, gasPrice *big.Int) (ethcommon.Hash, error)
+	// Sweep transfers all funds to the given address, nonce Lock()/Unlock()
+	// handling is done internally. Dust may be left is sending to a contract
+	// address, otherwise the balance afterward will be zero.
+	Sweep(ctx context.Context, to ethcommon.Address) (*ethtypes.Receipt, error)
+
+	// CancelTxWithNonce attempts to cancel a transaction with the given nonce
+	// by sending a zero-value tx to ourselves. Since the nonce is fixed, no
+	// locking is done. You can even intentionally run this method to fail some
+	// other method that has the lock and is waiting for a receipt.
+	CancelTxWithNonce(ctx context.Context, nonce uint64, gasPrice *big.Int) (*ethtypes.Receipt, error)
 
 	WaitForReceipt(ctx context.Context, txHash ethcommon.Hash) (*ethtypes.Receipt, error)
 	WaitForTimestamp(ctx context.Context, ts time.Time) error
@@ -301,62 +317,136 @@ func (c *ethClient) Raw() *ethclient.Client {
 	return c.ec
 }
 
-func (c *ethClient) CancelTxWithNonce(
-	ctx context.Context,
-	nonce uint64,
-	gasPrice *big.Int,
-) (ethcommon.Hash, error) {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	tx := ethtypes.NewTransaction(nonce, c.ethAddress, big.NewInt(0), 21000, gasPrice, nil)
-
-	signer := ethtypes.LatestSignerForChainID(c.chainID)
-	signedTx, err := ethtypes.SignTx(tx, signer, c.ethPrivKey)
-	if err != nil {
-		return ethcommon.Hash{}, fmt.Errorf("failed to sign tx: %w", err)
-	}
-
-	err = c.ec.SendTransaction(ctx, signedTx)
-	if err != nil {
-		return ethcommon.Hash{}, fmt.Errorf("failed to send tx: %w", err)
-	}
-
-	return signedTx.Hash(), nil
-}
-
+// Transfer transfers ETH to the given address, nonce Lock()/Unlock() handling
+// is done internally. The gasLimit parameter is required when transferring to a
+// contract and ignored otherwise.
 func (c *ethClient) Transfer(
 	ctx context.Context,
 	to ethcommon.Address,
 	amount *coins.WeiAmount,
-) (ethcommon.Hash, error) {
+	gasLimit *uint64,
+) (*ethtypes.Receipt, error) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
 	nonce, err := c.ec.NonceAt(ctx, c.ethAddress, nil)
 	if err != nil {
-		return ethcommon.Hash{}, fmt.Errorf("failed to get nonce: %w", err)
+		return nil, fmt.Errorf("failed to get nonce: %w", err)
 	}
 
 	gasPrice, err := c.ec.SuggestGasPrice(ctx)
 	if err != nil {
-		return ethcommon.Hash{}, fmt.Errorf("failed to get gas price: %w", err)
+		return nil, fmt.Errorf("failed to get gas price: %w", err)
 	}
 
-	tx := ethtypes.NewTransaction(nonce, to, amount.BigInt(), 21000, gasPrice, nil)
-
-	signer := ethtypes.LatestSignerForChainID(c.chainID)
-	signedTx, err := ethtypes.SignTx(tx, signer, c.ethPrivKey)
+	isContract, err := c.isContractAddress(ctx, to)
 	if err != nil {
-		return ethcommon.Hash{}, fmt.Errorf("failed to sign tx: %w", err)
+		return nil, fmt.Errorf("failed to determine if dest address is contract: %w", err)
 	}
 
-	err = c.ec.SendTransaction(ctx, signedTx)
+	if !isContract {
+		gasLimit = new(uint64)
+		*gasLimit = params.TxGas
+	} else {
+		if gasLimit == nil {
+			return nil, errors.New("gas limit is required when transferring to a contract")
+		}
+	}
+
+	return transfer(&transferConfig{
+		ctx:      ctx,
+		ec:       c.ec,
+		pk:       c.ethPrivKey,
+		destAddr: to,
+		amount:   amount,
+		gasLimit: *gasLimit,
+		gasPrice: coins.NewWeiAmount(gasPrice),
+		nonce:    nonce,
+	})
+}
+
+func (c *ethClient) Sweep(ctx context.Context, to ethcommon.Address) (*ethtypes.Receipt, error) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	nonce, err := c.ec.NonceAt(ctx, c.ethAddress, nil)
 	if err != nil {
-		return ethcommon.Hash{}, fmt.Errorf("failed to send transaction: %w", err)
+		return nil, fmt.Errorf("failed to get nonce: %w", err)
 	}
 
-	return signedTx.Hash(), nil
+	gasPrice, err := c.ec.SuggestGasPrice(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get gas price: %w", err)
+	}
+
+	isContract, err := c.isContractAddress(ctx, to)
+	if err != nil {
+		return nil, fmt.Errorf("failed to determine if dest address is contract: %w", err)
+	}
+
+	// Sweeping to a contract address is problematic, as any overestimation of
+	// the needed gas leaves non-spendable dust in the wallet. If someone has a
+	// use-case in the future, we can add the feature.
+	if isContract {
+		return nil, errors.New("sweeping to contract addresses is not currently supported")
+	}
+
+	balance, err := c.Balance(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to determine balance: %w", err)
+	}
+
+	fees := coins.NewWeiAmount(new(big.Int).Mul(gasPrice, big.NewInt(int64(params.TxGas))))
+
+	if balance.Cmp(fees) <= 0 {
+		return nil, fmt.Errorf("balance of %s ETH too small for fees (%d gas * %s gas-price = %s ETH",
+			balance.AsEtherString(), params.TxGas, coins.NewWeiAmount(gasPrice).AsEtherString(), fees.AsEtherString())
+	}
+
+	amount := coins.NewWeiAmount(new(big.Int).Sub(balance.BigInt(), fees.BigInt()))
+
+	return transfer(&transferConfig{
+		ctx:      ctx,
+		ec:       c.ec,
+		pk:       c.ethPrivKey,
+		destAddr: to,
+		amount:   amount,
+		gasLimit: params.TxGas,
+		gasPrice: coins.NewWeiAmount(gasPrice),
+		nonce:    nonce,
+	})
+}
+
+// CancelTxWithNonce attempts to cancel a transaction with the given nonce
+// by sending a zero-value tx to ourselves. Since the nonce is fixed, no
+// locking is done. You can even intentionally run this method to fail some
+// other method that has the lock and is waiting for a receipt.
+func (c *ethClient) CancelTxWithNonce(
+	ctx context.Context,
+	nonce uint64,
+	gasPrice *big.Int,
+) (*ethtypes.Receipt, error) {
+	// no locking, nonce is fixed and we are not protecting it
+	return transfer(&transferConfig{
+		ctx:      ctx,
+		ec:       c.ec,
+		pk:       c.ethPrivKey,
+		destAddr: c.ethAddress,                      // ourself
+		amount:   coins.NewWeiAmount(big.NewInt(0)), // zero ETH
+		gasLimit: params.TxGas,
+		gasPrice: coins.NewWeiAmount(gasPrice),
+		nonce:    nonce,
+	})
+}
+
+func (c *ethClient) isContractAddress(ctx context.Context, addr ethcommon.Address) (bool, error) {
+	bytecode, err := c.Raw().CodeAt(ctx, addr, nil)
+	if err != nil {
+		return false, err
+	}
+
+	isContract := len(bytecode) > 0
+	return isContract, nil
 }
 
 func validateChainID(env common.Environment, chainID *big.Int) error {

ethereum/extethclient/eth_wallet_client_test.go

@@ -4,15 +4,122 @@
 package extethclient
 
 import (
+	"context"
 	"math/big"
 	"testing"
 
+	"github.com/ethereum/go-ethereum/crypto"
+	"github.com/ethereum/go-ethereum/params"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/athanorlabs/atomic-swap/cliutil"
+	"github.com/athanorlabs/atomic-swap/coins"
 	"github.com/athanorlabs/atomic-swap/common"
+	contracts "github.com/athanorlabs/atomic-swap/ethereum"
+	"github.com/athanorlabs/atomic-swap/tests"
 )
 
+func init() {
+	cliutil.SetLogLevels("debug")
+}
+
+func Test_ethClient_Transfer(t *testing.T) {
+	ctx := context.Background()
+
+	senderKey := tests.GetTestKeyByIndex(t, 0)
+	receiverKey, err := crypto.GenerateKey()
+	require.NoError(t, err)
+
+	senderEC := CreateTestClient(t, senderKey)
+	receiverEC := CreateTestClient(t, receiverKey)
+
+	transferAmt := coins.EtherToWei(coins.StrToDecimal("0.123456789012345678"))
+
+	receipt, err := senderEC.Transfer(ctx, receiverEC.Address(), transferAmt, nil)
+	require.NoError(t, err)
+	require.Equal(t, receipt.GasUsed, params.TxGas)
+
+	// balance is exactly equal to the transferred amount
+	receiverBal, err := receiverEC.Balance(ctx)
+	require.NoError(t, err)
+	require.Equal(t, receiverBal.AsEtherString(), transferAmt.AsEtherString())
+}
+
+func Test_ethClient_Transfer_toContract(t *testing.T) {
+	ctx := context.Background()
+	pk := tests.GetTestKeyByIndex(t, 0)
+	ec := CreateTestClient(t, pk)
+
+	token := contracts.GetMockTether(t, ec.Raw(), pk)
+	startTokenBal, err := ec.ERC20Balance(ctx, token.Address)
+	require.NoError(t, err)
+
+	zero := new(coins.WeiAmount)
+	gasLimit := params.TxGas * 2
+
+	_, err = ec.Transfer(ctx, token.Address, zero, &gasLimit)
+	require.NoError(t, err)
+
+	// our test token contract mints you 100 standard token units when sending
+	// it a zero value transaction.
+	endTokenBal, err := ec.ERC20Balance(ctx, token.Address)
+	require.NoError(t, err)
+	require.Greater(t, endTokenBal.AsStd().Cmp(startTokenBal.AsStd()), 0)
+}
+
+func Test_ethClient_Sweep(t *testing.T) {
+	ctx := context.Background()
+	srcBal := coins.EtherToWei(coins.StrToDecimal("0.5"))
+
+	// We don't want to completely drain a ganache key, so we need to generate a
+	// new key for the sweep sender and then fund the account.
+	testFunder := tests.GetTestKeyByIndex(t, 0)
+	sweepSrcKey, err := crypto.GenerateKey()
+	require.NoError(t, err)
+	sweepDestKey, err := crypto.GenerateKey()
+	require.NoError(t, err)
+
+	funderEC := CreateTestClient(t, testFunder)
+	sourceEC := CreateTestClient(t, sweepSrcKey)
+	destEC := CreateTestClient(t, sweepDestKey)
+
+	// fund the sweep source account with 0.5 ETH
+	_, err = funderEC.Transfer(ctx, sourceEC.Address(), srcBal, nil)
+	require.NoError(t, err)
+
+	receipt, err := sourceEC.Sweep(ctx, destEC.Address())
+	require.NoError(t, err)
+	require.Equal(t, receipt.GasUsed, params.TxGas)
+
+	fees := new(big.Int).Mul(receipt.EffectiveGasPrice, big.NewInt(int64(receipt.GasUsed)))
+	expectedDestBal := coins.NewWeiAmount(new(big.Int).Sub(srcBal.BigInt(), fees))
+
+	destBal, err := destEC.Balance(ctx)
+	require.NoError(t, err)
+	require.Equal(t, expectedDestBal.AsEtherString(), destBal.AsEtherString())
+}
+
+// Unfortunately, ganache does not have a mempool, so we can't do a meaningful
+// test that does actual cancellation. We just test it as sending a transaction
+// that doesn't cancel a nonce in the mempool.
+func Test_ethClient_CancelTxWithNonce(t *testing.T) {
+	ctx := context.Background()
+	pk := tests.GetTestKeyByIndex(t, 0)
+	ec := CreateTestClient(t, pk)
+
+	nonce, err := ec.Raw().NonceAt(ctx, ec.Address(), nil)
+	require.NoError(t, err)
+
+	gasPrice, err := ec.SuggestGasPrice(ctx)
+	require.NoError(t, err)
+
+	receipt, err := ec.CancelTxWithNonce(ctx, nonce, gasPrice)
+	require.NoError(t, err)
+
+	require.Equal(t, receipt.EffectiveGasPrice.String(), gasPrice.String())
+}
+
 func Test_validateChainID_devSuccess(t *testing.T) {
 	err := validateChainID(common.Development, big.NewInt(common.GanacheChainID))
 	require.NoError(t, err)

ethereum/extethclient/transfer.go

@@ -0,0 +1,72 @@
+package extethclient
+
+import (
+	"context"
+	"crypto/ecdsa"
+	"fmt"
+
+	ethcommon "github.com/ethereum/go-ethereum/common"
+	ethtypes "github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/ethclient"
+
+	"github.com/athanorlabs/atomic-swap/coins"
+	"github.com/athanorlabs/atomic-swap/common"
+	"github.com/athanorlabs/atomic-swap/ethereum/block"
+)
+
+type transferConfig struct {
+	ctx      context.Context
+	ec       *ethclient.Client
+	pk       *ecdsa.PrivateKey
+	destAddr ethcommon.Address
+	amount   *coins.WeiAmount
+	gasLimit uint64
+	gasPrice *coins.WeiAmount
+	nonce    uint64
+}
+
+// transfer handles almost any use case for transferring ETH by having all the
+// configurable values (nonce, gas-price, etc.) set by the caller.
+func transfer(cfg *transferConfig) (*ethtypes.Receipt, error) {
+	ctx := cfg.ctx
+	ec := cfg.ec
+
+	tx := ethtypes.NewTx(&ethtypes.LegacyTx{
+		Nonce:    cfg.nonce,
+		To:       &cfg.destAddr,
+		Value:    cfg.amount.BigInt(),
+		Gas:      cfg.gasLimit,
+		GasPrice: cfg.gasPrice.BigInt(),
+	})
+
+	chainID, err := ec.ChainID(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	signer := ethtypes.LatestSignerForChainID(chainID)
+	signedTx, err := ethtypes.SignTx(tx, signer, cfg.pk)
+
+	if err != nil {
+		return nil, fmt.Errorf("failed to sign tx: %w", err)
+	}
+
+	txHash := signedTx.Hash()
+
+	err = ec.SendTransaction(ctx, signedTx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to send transfer transaction: %w", err)
+	}
+
+	log.Infof("transfer of %s ETH to %s sent to mempool with txID %s, nonce %d, gas-price %s ETH",
+		cfg.amount.AsStdString(), cfg.destAddr, txHash, cfg.nonce, cfg.gasPrice.AsStdString())
+
+	receipt, err := block.WaitForReceipt(ctx, ec, txHash)
+	if err != nil {
+		return nil, fmt.Errorf("failed waiting for txID %s receipt: %w", txHash, err)
+	}
+
+	log.Infof("transfer included in chain %s", common.ReceiptInfo(receipt))
+
+	return receipt, nil
+}