Merge pull request #43 from renproject/feat/gas-base-gas-price

Fix the use of gas price and gas cap

Author: jazg

.github/workflows/test.yml

@@ -21,8 +21,11 @@ jobs:
           path: ~/go/pkg/mod
           key: ${{ runner.os }}-go-aw-${{ hashFiles('**/go.sum') }}
 
+      # Remove apt repos that are known to break from time to time
+      # See https://github.com/actions/virtual-environments/issues/323
       - name: Install dependency packages
         run: |
+          for apt_file in `grep -lr microsoft /etc/apt/sources.list.d/`; do sudo rm $apt_file; done
           sudo apt-get update
           sudo apt-get install -y build-essential
           sudo apt-get install -y jq mesa-opencl-icd ocl-icd-opencl-dev pkg-config

api/account/account.go

@@ -56,7 +56,7 @@ type Tx interface {
 // information, and this should be accepted during the construction of the
 // chain-specific transaction builder.
 type TxBuilder interface {
-	BuildTx(ctx context.Context, from, to address.Address, value, nonce, gasLimit, gasPrice pack.U256, payload pack.Bytes) (Tx, error)
+	BuildTx(ctx context.Context, from, to address.Address, value, nonce, gasLimit, gasPrice, gasCap pack.U256, payload pack.Bytes) (Tx, error)
 }
 
 // The Client interface defines the functionality required to interact with a

api/gas/gas.go

@@ -11,15 +11,27 @@ import (
 )
 
 // The Estimator interface defines the functionality required to know the
-// current recommended gas prices.
+// current recommended gas price per gas unit and gas cap per gas unit. Not all
+// chains have the concept of a gas cap, in which case it should be set to be
+// equal to the gas price.
 type Estimator interface {
-	// EstimateGasPrice that should be used to get confirmation within a
-	// reasonable amount of time. The precise definition of "reasonable amount
-	// of time" varies from chain to chain, and so is left open to
-	// interpretation by the implementation. For example, in Bitcoin, it would
-	// be the recommended SATs-per-byte required to get a transaction into the
-	// next block. In Ethereum, it would be the recommended GWEI-per-gas
-	// required to get a transaction into one of the next few blocks (because
-	// blocks happen a lot faster).
-	EstimateGasPrice(context.Context) (pack.U256, pack.U256, error)
+	// EstimateGas base/price that should be used in order to get a transaction
+	// confirmed within a reasonable amount of time. The precise definition of
+	// "reasonable amount of time" varies from chain to chain, and so is left
+	// open to interpretation by the implementation.
+
+	// For example, in Bitcoin, the gas price (and gas cap) would be the
+	// recommended SATs-per-byte required to get a transaction into the next
+	// block.
+
+	// In Ethereum without EIP-1559, the gas price (and gas cap) would be the
+	// recommended GWEI-per-gas required to get a transaction into one of the
+	// next few blocks (because blocks happen a lot faster). In Ethereum with
+	// EIP1559, the gas price and gas cap would be back calculated given the gas
+	// cap and an estimate of the current gas base.
+	//
+	// The assumption is that the total gas cost will be gasLimit * gasPrice <
+	// gasLimit * gasCap. If chain nodes give back values based on different
+	// assumptions, then the values must be normalised as needed.
+	EstimateGas(context.Context) (gasPrice, gasCap pack.U256, err error)
 }

chain/bitcoin/gas.go

@@ -30,20 +30,20 @@ func NewGasEstimator(client Client, numBlocks int64) GasEstimator {
 	}
 }
 
-// EstimateGasPrice returns the number of SATs-per-byte that is needed in order
-// to confirm transactions with an estimated maximum delay of `numBlocks` block.
-// It is the responsibility of the caller to know the number of bytes in their
-// transaction. This method calls the `estimatesmartfee` RPC call to the node,
-// which based on a conservative (considering longer history) strategy returns
-// the estimated BTC per kilobyte of data in the transaction. An error will be
-// returned if the bitcoin node hasn't observed enough blocks to make an
-// estimate for the provided target `numBlocks`.
-func (gasEstimator GasEstimator) EstimateGasPrice(ctx context.Context) (pack.U256, pack.U256, error) {
+// EstimateGas returns the number of SATs-per-byte (for both price and cap) that
+// is needed in order to confirm transactions with an estimated maximum delay of
+// `numBlocks` block. It is the responsibility of the caller to know the number
+// of bytes in their transaction. This method calls the `estimatesmartfee` RPC
+// call to the node, which based on a conservative (considering longer history)
+// strategy returns the estimated BTC per kilobyte of data in the transaction.
+// An error will be returned if the bitcoin node hasn't observed enough blocks
+// to make an estimate for the provided target `numBlocks`.
+func (gasEstimator GasEstimator) EstimateGas(ctx context.Context) (pack.U256, pack.U256, error) {
 	feeRate, err := gasEstimator.client.EstimateSmartFee(ctx, gasEstimator.numBlocks)
 	if err != nil {
 		return pack.NewU256([32]byte{}), pack.NewU256([32]byte{}), err
 	}
 
 	satsPerByte := uint64(feeRate * btcToSatoshis / kilobyteToByte)
-	return pack.NewU256FromU64(pack.NewU64(satsPerByte)), pack.NewU256([32]byte{}), nil
+	return pack.NewU256FromU64(pack.NewU64(satsPerByte)), pack.NewU256FromU64(pack.NewU64(satsPerByte)), nil
 }

chain/bitcoin/gas_test.go

@@ -19,17 +19,17 @@ var _ = Describe("Gas", func() {
 
 			// estimate fee to include tx within 1 block.
 			gasEstimator1 := bitcoin.NewGasEstimator(client, 1)
-			gasPrice1, _, err := gasEstimator1.EstimateGasPrice(ctx)
+			gasPrice1, _, err := gasEstimator1.EstimateGas(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
 			// estimate fee to include tx within 10 blocks.
 			gasEstimator2 := bitcoin.NewGasEstimator(client, 10)
-			gasPrice2, _, err := gasEstimator2.EstimateGasPrice(ctx)
+			gasPrice2, _, err := gasEstimator2.EstimateGas(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
 			// estimate fee to include tx within 100 blocks.
 			gasEstimator3 := bitcoin.NewGasEstimator(client, 100)
-			gasPrice3, _, err := gasEstimator3.EstimateGasPrice(ctx)
+			gasPrice3, _, err := gasEstimator3.EstimateGas(ctx)
 			Expect(err).NotTo(HaveOccurred())
 
 			// expect fees in this order at the very least.

chain/cosmos/gas.go

@@ -24,7 +24,9 @@ func NewGasEstimator(gasPerByte pack.U256) gas.Estimator {
 	}
 }
 
-// EstimateGasPrice returns gas required per byte for Cosmos-compatible chains.
-func (gasEstimator *GasEstimator) EstimateGasPrice(ctx context.Context) (pack.U256, pack.U256, error) {
-	return gasEstimator.gasPerByte, pack.NewU256([32]byte{}), nil
+// EstimateGas returns gas required per byte for Cosmos-compatible chains. This
+// value is used for both the price and cap, because Cosmos-compatible chains do
+// not have a distinct concept of cap.
+func (gasEstimator *GasEstimator) EstimateGas(ctx context.Context) (pack.U256, pack.U256, error) {
+	return gasEstimator.gasPerByte, gasEstimator.gasPerByte, nil
 }

chain/cosmos/tx.go

@@ -49,7 +49,7 @@ func NewTxBuilder(options TxBuilderOptions, client *Client) account.TxBuilder {
 // BuildTx consumes a list of MsgSend to build and return a cosmos transaction.
 // This transaction is unsigned, and must be signed before submitting to the
 // cosmos chain.
-func (builder txBuilder) BuildTx(ctx context.Context, from, to address.Address, value, nonce, gasLimit, gasPrice pack.U256, payload pack.Bytes) (account.Tx, error) {
+func (builder txBuilder) BuildTx(ctx context.Context, from, to address.Address, value, nonce, gasLimit, gasPrice, gasCap pack.U256, payload pack.Bytes) (account.Tx, error) {
 	fromAddr, err := types.AccAddressFromBech32(string(from))
 	if err != nil {
 		return nil, err

chain/filecoin/account.go

@@ -20,16 +20,15 @@ import (
 // broadcasted to the filecoin network. The TxBuilder is configured using a
 // gas price and gas limit.
 type TxBuilder struct {
-	gasPremium pack.U256
 }
 
 // NewTxBuilder creates a new transaction builder.
-func NewTxBuilder(gasPremium pack.U256) TxBuilder {
-	return TxBuilder{gasPremium}
+func NewTxBuilder() TxBuilder {
+	return TxBuilder{}
 }
 
 // BuildTx receives transaction fields and constructs a new transaction.
-func (txBuilder TxBuilder) BuildTx(ctx context.Context, from, to address.Address, value, nonce, gasLimit, gasFeeCap pack.U256, payload pack.Bytes) (account.Tx, error) {
+func (txBuilder TxBuilder) BuildTx(ctx context.Context, from, to address.Address, value, nonce, gasLimit, gasPrice, gasCap pack.U256, payload pack.Bytes) (account.Tx, error) {
 	filfrom, err := filaddress.NewFromString(string(from))
 	if err != nil {
 		return nil, fmt.Errorf("bad from address '%v': %v", from, err)
@@ -46,9 +45,9 @@ func (txBuilder TxBuilder) BuildTx(ctx context.Context, from, to address.Address
 			To:         filto,
 			Value:      big.Int{Int: value.Int()},
 			Nonce:      nonce.Int().Uint64(),
-			GasFeeCap:  big.Int{Int: gasFeeCap.Int()},
+			GasFeeCap:  big.Int{Int: gasCap.Int()},
 			GasLimit:   gasLimit.Int().Int64(),
-			GasPremium: big.Int{Int: txBuilder.gasPremium.Int()},
+			GasPremium: big.Int{Int: gasPrice.Int()},
 			Method:     methodNum,
 			Params:     payload,
 		},

chain/filecoin/filecoin_test.go

@@ -69,11 +69,11 @@ var _ = Describe("Filecoin", func() {
 			// get good gas estimates
 			gasLimit := uint64(2200000)
 			gasEstimator := filecoin.NewGasEstimator(client, int64(gasLimit))
-			gasFeeCap, gasPremium, err := gasEstimator.EstimateGasPrice(ctx)
+			gasPremium, gasFeeCap, err := gasEstimator.EstimateGas(ctx)
 			Expect(err).ToNot(HaveOccurred())
 
 			// construct the transaction builder
-			filTxBuilder := filecoin.NewTxBuilder(gasPremium)
+			filTxBuilder := filecoin.NewTxBuilder()
 
 			// build the transaction
 			sender := multichain.Address(pack.String(senderFilAddr.String()))
@@ -88,7 +88,8 @@ var _ = Describe("Filecoin", func() {
 				amount, // amount
 				nonce,  // nonce
 				pack.NewU256FromU64(pack.NewU64(gasLimit)), // gasLimit
-				gasFeeCap,       // gasFeeCap
+				gasPremium,
+				gasFeeCap,
 				pack.Bytes(nil), // payload
 			)
 			Expect(err).ToNot(HaveOccurred())

chain/filecoin/gas.go

@@ -32,11 +32,10 @@ func NewGasEstimator(client *Client, gasLimit int64) *GasEstimator {
 	}
 }
 
-// EstimateGasPrice returns gas fee cap and gas premium values being accepted
-// in the filecoin chain at present. These numbers may vary as the chain's
-// congestion level increases. It is safe to say that by using the fetched
-// values, a transaction will be included in a block with minimal delay.
-func (gasEstimator *GasEstimator) EstimateGasPrice(ctx context.Context) (pack.U256, pack.U256, error) {
+// EstimateGas returns an estimate of the current gas price (also known as gas
+// premium) and gas cap. These numbers change with congestion. These estimates
+// are often a little bit off, and this should be considered when using them.
+func (gasEstimator *GasEstimator) EstimateGas(ctx context.Context) (pack.U256, pack.U256, error) {
 	// Create a dummy "Send" message.
 	msgIn := types.Message{
 		Version:    types.MessageVersion,
@@ -68,5 +67,5 @@ func (gasEstimator *GasEstimator) EstimateGasPrice(ctx context.Context) (pack.U2
 	gasFeeCap := big.NewInt(0).SetBytes(gasFeeCapBytes)
 	gasPremium := big.NewInt(0).SetBytes(gasPremiumBytes)
 
-	return pack.NewU256FromInt(gasFeeCap), pack.NewU256FromInt(gasPremium), nil
+	return pack.NewU256FromInt(gasPremium), pack.NewU256FromInt(gasFeeCap), nil
 }

chain/filecoin/gas_test.go

@@ -33,7 +33,7 @@ var _ = Describe("Gas", func() {
 			gasEstimator := filecoin.NewGasEstimator(client, 2000000)
 
 			// estimate gas price
-			_, _, err = gasEstimator.EstimateGasPrice(ctx)
+			_, _, err = gasEstimator.EstimateGas(ctx)
 			Expect(err).ToNot(HaveOccurred())
 		})
 	})

chain/terra/terra_test.go

@@ -72,8 +72,9 @@ var _ = Describe("Terra", func() {
 					recipient,                             // to
 					amount,                                // amount
 					nonce,                                 // nonce
-					pack.NewU256FromU64(pack.U64(200000)), // gas
+					pack.NewU256FromU64(pack.U64(200000)), // gas limit
 					pack.NewU256FromU64(pack.U64(1)),      // gas price
+					pack.NewU256FromU64(pack.U64(1)),      // gas cap
 					payload,                               // memo
 				)
 				Expect(err).NotTo(HaveOccurred())

multichain_test.go

@@ -304,7 +304,7 @@ var _ = Describe("Multichain", func() {
 			rpcURL              pack.String
 			randomRecipientAddr func() multichain.Address
 			initialise          func(pack.String) (multichain.AccountClient, multichain.AccountTxBuilder)
-			txParams            func() (pack.U256, pack.U256, pack.U256, pack.Bytes)
+			txParams            func(multichain.AccountClient) (pack.U256, pack.U256, pack.U256, pack.U256, pack.Bytes)
 			chain               multichain.Chain
 		}{
 			{
@@ -355,12 +355,13 @@ var _ = Describe("Multichain", func() {
 
 					return client, txBuilder
 				},
-				func() (pack.U256, pack.U256, pack.U256, pack.Bytes) {
+				func(_ multichain.AccountClient) (pack.U256, pack.U256, pack.U256, pack.U256, pack.Bytes) {
 					amount := pack.NewU256FromU64(pack.U64(2000000))
 					gasLimit := pack.NewU256FromU64(pack.U64(100000))
 					gasPrice := pack.NewU256FromU64(pack.U64(1))
+					gasCap := pack.NewU256FromInt(gasPrice.Int())
 					payload := pack.NewBytes([]byte("multichain"))
-					return amount, gasLimit, gasPrice, payload
+					return amount, gasLimit, gasPrice, gasCap, payload
 				},
 				multichain.Terra,
 			},
@@ -419,20 +420,22 @@ var _ = Describe("Multichain", func() {
 					)
 					Expect(err).NotTo(HaveOccurred())
 
-					gasEstimator := filecoin.NewGasEstimator(client, 2189560)
-					_, gasPremium, err := gasEstimator.EstimateGasPrice(context.Background())
-					Expect(err).NotTo(HaveOccurred())
-
-					txBuilder := filecoin.NewTxBuilder(gasPremium)
+					txBuilder := filecoin.NewTxBuilder()
 
 					return client, txBuilder
 				},
-				func() (pack.U256, pack.U256, pack.U256, pack.Bytes) {
+				func(client multichain.AccountClient) (pack.U256, pack.U256, pack.U256, pack.U256, pack.Bytes) {
 					amount := pack.NewU256FromU64(pack.NewU64(100000000))
 					gasLimit := pack.NewU256FromU64(pack.NewU64(2189560))
-					gasPrice := pack.NewU256FromU64(pack.NewU64(300000))
+
+					// Fetch gas price and gas cap using the gas estimator.
+					filecoinClient := client.(*filecoin.Client)
+					gasPrice, gasCap, err := filecoin.NewGasEstimator(filecoinClient, gasLimit.Int().Int64()).
+						EstimateGas(context.Background())
+					Expect(err).NotTo(HaveOccurred())
+
 					payload := pack.Bytes(nil)
-					return amount, gasLimit, gasPrice, payload
+					return amount, gasLimit, gasPrice, gasCap, payload
 				},
 				multichain.Filecoin,
 			},
@@ -457,13 +460,13 @@ var _ = Describe("Multichain", func() {
 					Expect(err).NotTo(HaveOccurred())
 
 					// Build a transaction.
-					amount, gasLimit, gasPrice, payload := accountChain.txParams()
+					amount, gasLimit, gasPrice, gasCap, payload := accountChain.txParams(accountClient)
 
 					accountTx, err := txBuilder.BuildTx(
 						ctx,
 						multichain.Address(senderAddr),
 						recipientAddr,
-						amount, nonce, gasLimit, gasPrice,
+						amount, nonce, gasLimit, gasPrice, gasCap,
 						payload,
 					)
 					Expect(err).NotTo(HaveOccurred())