Improve RPC API compatibility for Ethers.js (#1957)

* Add minimal ts project

* Add ethers.js dependency

* Add etherejs api test (currently failing)

Test tries to get the block

* Add dummy data for gas limit field

* Build ethersjs project before executing test

* Return real gas limit in place of dummy data

* Return baseFee if it is acessible

* Use framework for ethers.js compatibility tests

Instead of running a script we now use the mocha testing framework which
provides much better output and makes it much easier to add testcases.

* Store package lock

* Add make rule to install ethersjs project deps

* Update ci to prepare ehtersjs project

* Add dockerfile for a node/golang CI executor

Update CI to use new docker image, since we now need to run node and npm
as part of our tests.

* Run e2e coverage CI tests verbosely

* Add debug

* Add more debug

* Change address format

The IPV6 addresses returned from the geth node were not working on CI so
we convert them to IPV4 addresses.

* Update log message for clarity

* Update node-golang dockerfile

Add installed go binaries location to user's path

* Update readme to specify a minimum go version

This should mitigate the readme going out of date whenever we update go.

* Default to single quotes in typescript

* Flag to disable RPC compatibilty fields

If the flag is set then the 'gasLimit' and 'baseFeePerGas' fields will
not be returned on RPC blocks.

* Split the tests under 2 describe headings

One for tests with state one for tests without.

This makes executing each batch of tests easier because we can just grep
for the describe headings.

* Document that js tests shoudn't be run standalone

* Move ethersjs test project under e2e_test

Just to further signify that it is not for standalone use.

* Do not return block fields that cant be retreived

If the state is missing for gasLimit or baseFeePerGas or if there is
some other failure when retrieving them then do not add them as fields
to the block. Preivously in some cases a default value would have been
added to the block and returned.

* Fix typos

* Remove unused code

Co-authored-by: Pasto <hbandura@gmail.com>
Co-authored-by: Gaston Ponti <ponti@clabs.co>

.circleci/config.yml

@@ -114,7 +114,7 @@ parameters:
 executors:
   golang:
     docker:
-      - image: circleci/golang:1.16
+      - image: "us.gcr.io/celo-testnet/circleci-node12-golang1.17.5"
     working_directory: ~/repos/geth
     environment:
       # Change the go modules to be cached under ~/repos so that we can add
@@ -309,6 +309,7 @@ jobs:
       - attach_workspace:
           at: ~/repos
       - *restore-go-mod-cache
+      - run: make prepare-ethersjs-project
       - run: go get github.com/jstemmer/go-junit-report
       - run:
           name: Run tests
@@ -330,6 +331,7 @@ jobs:
       - attach_workspace:
           at: ~/repos
       - *restore-go-mod-cache
+      - run: make prepare-ethersjs-project
       - run: go get github.com/jstemmer/go-junit-report
       - run:
           name: Run tests
@@ -351,11 +353,12 @@ jobs:
       - attach_workspace:
           at: ~/repos
       - *restore-go-mod-cache
+      - run: make prepare-ethersjs-project
         # Run the tests with coverage parse the coverage and output the summary
       - run:
           name: Run tests and print coverage summary
           command: |
-            go test -coverprofile cov.out -coverpkg ./consensus/istanbul/... ./e2e_test 
+            go test -v -coverprofile cov.out -coverpkg ./consensus/istanbul/... ./e2e_test 
             go run tools/parsecov/main.go -packagePrefix github.com/celo-org/celo-blockchain/ cov.out > summary
             cat summary
 

.circleci/node-golang-dockerfile/Dockerfile

@@ -0,0 +1,7 @@
+# This has been pushed to us.gcr.io/celo-testnet/circleci-node12-golang1.17.5
+FROM circleci/node:12
+
+COPY --from=circleci/golang:1.17.5 /usr/local/go/ /usr/local/go/
+
+ENV PATH="/home/circleci/go/bin:${PATH}"
+ENV PATH="/usr/local/go/bin:${PATH}"

Makefile

@@ -7,7 +7,7 @@
 .PHONY: geth-linux-arm geth-linux-arm-5 geth-linux-arm-6 geth-linux-arm-7 geth-linux-arm64
 .PHONY: geth-darwin geth-darwin-amd64
 .PHONY: geth-windows geth-windows-386 geth-windows-amd64
-.PHONY: prepare-system-contracts
+.PHONY: prepare prepare-system-contracts prepare-ethersjs-project
 
 GOBIN = ./build/bin
 GO ?= latest
@@ -45,6 +45,13 @@ geth:
 	@echo "Done building."
 	@echo "Run \"$(GOBIN)/geth\" to launch geth."
 
+prepare: prepare-system-contracts prepare-ethersjs-project
+
+prepare-ethersjs-project: ./e2e_test/ethersjs-api-check/node_modules
+
+./e2e_test/ethersjs-api-check/node_modules: ./e2e_test/ethersjs-api-check/package.json ./e2e_test/ethersjs-api-check/package-lock.json
+	@cd ./e2e_test/ethersjs-api-check && npm ci
+
 # This rule checks out celo-monorepo under MONOREPO_PATH at the commit contained in
 # monorepo_commit and compiles the system solidity contracts. It then copies the
 # compiled contracts from the monorepo to the compiled-system-contracts, so

README.md

@@ -14,7 +14,7 @@ Most functionality of this client is similar to `go-ethereum`, also known as `ge
 
 ## Building the source
 
-Building `geth` requires both a Go (version 1.16) and a C compiler.
+Building `geth` requires both Go (min version 1.15) and a C compiler.
 You can install them using your favourite package manager. Once the dependencies are installed, run
 
 ```shell
@@ -58,7 +58,12 @@ The Celo blockchain client comes with several wrappers/executables found in the
 
 ## Running tests
 
-Prior to running tests you will need to run `make prepare-system-contracts`.
+Prior to running tests you will need to run `make prepare`, this will run two sub rules.
+
+Without first running this `make prepare`, certain tests will fail.
+
+### prepare-system-contracts
+
 This will shallow checkout the
 [celo-monorepo](https://github.com/celo-org/celo-monorepo) under
 `../.celo-blockchain-monorepo-checkout` relative to this project's root at the
@@ -86,8 +91,8 @@ those checked out by the `prepare-system-contracts` rule, and the checkouts
 created by the `prepare-system-contracts` rule should not be manually modifed,
 aside from changing the contract source.
 
-
-Without first running this make rule, certain tests will fail.
+### prepare-ethersjs-project
+This will install dependencies for the `ethersjs-api-check` typescript project.
 
 ## Running Celo
 

cmd/geth/main.go

@@ -164,6 +164,7 @@ var (
 	}
 
 	rpcFlags = []cli.Flag{
+		utils.DisableRPCETHCompatibility,
 		utils.HTTPEnabledFlag,
 		utils.HTTPListenAddrFlag,
 		utils.HTTPPortFlag,

cmd/utils/flags.go

@@ -465,6 +465,10 @@ var (
 		Usage: "Disables db compaction after import",
 	}
 	// RPC settings
+	DisableRPCETHCompatibility = cli.BoolFlag{
+		Name:  "disablerpcethcompatibility",
+		Usage: "If set, blocks returned from the RPC api will not contain the 'gasLimit' and 'baseFeePerGas' fields, which were added to the RPC block representation in order to improve compatibility with ethereum tooling. Note these fields do not currently exist on the internal block representation so they should be disregarded for the purposes of hashing or signing",
+	}
 
 	IPCDisabledFlag = cli.BoolFlag{
 		Name:  "ipcdisable",
@@ -1762,6 +1766,11 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) {
 		cfg.RPCTxFeeCap = ctx.GlobalFloat64(RPCGlobalTxFeeCapFlag.Name)
 	}
 
+	cfg.RPCEthCompatibility = true
+	if ctx.GlobalIsSet(DisableRPCETHCompatibility.Name) {
+		cfg.RPCEthCompatibility = false
+	}
+
 	// Disable DNS discovery by default (by using the flag's value even if it hasn't been set and so
 	// has the default value ""), since we don't have DNS discovery set up for Celo.
 	// Note that passing --discovery.dns "" is the way the Geth docs specify for disabling DNS discovery,

contracts/blockchain_parameters/blockchain_parameters.go

@@ -79,16 +79,16 @@ func getIntrinsicGasForAlternativeFeeCurrency(vmRunner vm.EVMRunner) (uint64, er
 // GetBlockGasLimitOrDefault retrieves the block max gas limit
 // In case of error, it returns the default value
 func GetBlockGasLimitOrDefault(vmRunner vm.EVMRunner) uint64 {
-	val, err := getBlockGasLimit(vmRunner)
+	val, err := GetBlockGasLimit(vmRunner)
 	if err != nil {
 		logError("blockGasLimit", err)
 		return params.DefaultGasLimit
 	}
 	return val
 }
 
-// getBlockGasLimit retrieves the block max gas limit
-func getBlockGasLimit(vmRunner vm.EVMRunner) (uint64, error) {
+// GetBlockGasLimit retrieves the block max gas limit
+func GetBlockGasLimit(vmRunner vm.EVMRunner) (uint64, error) {
 	var gasLimit *big.Int
 	err := blockGasLimitMethod.Query(vmRunner, &gasLimit)
 	if err != nil {

contracts/blockchain_parameters/blockchain_parameters_test.go

@@ -71,8 +71,8 @@ func TestGetIntrinsicGasForAlternativeFeeCurrencyOrDefault(t *testing.T) {
 }
 
 func TestGetBlockGasLimit(t *testing.T) {
-	testutil.TestFailOnFailingRunner(t, getBlockGasLimit)
-	testutil.TestFailsWhenContractNotDeployed(t, contracts.ErrSmartContractNotDeployed, getBlockGasLimit)
+	testutil.TestFailOnFailingRunner(t, GetBlockGasLimit)
+	testutil.TestFailsWhenContractNotDeployed(t, contracts.ErrSmartContractNotDeployed, GetBlockGasLimit)
 	t.Run("should return block gas limit", func(t *testing.T) {
 		g := NewGomegaWithT(t)
 
@@ -84,7 +84,7 @@ func TestGetBlockGasLimit(t *testing.T) {
 			},
 		)
 
-		gas, err := getBlockGasLimit(runner)
+		gas, err := GetBlockGasLimit(runner)
 		g.Expect(err).ToNot(HaveOccurred())
 		g.Expect(gas).To(Equal(uint64(50000)))
 	})

contracts/gasprice_minimum/gasprice_minimum.go

@@ -17,6 +17,7 @@
 package gasprice_minimum
 
 import (
+	"fmt"
 	"math/big"
 
 	"github.com/celo-org/celo-blockchain/common"
@@ -89,6 +90,32 @@ func GetGasPriceMinimum(vmRunner vm.EVMRunner, currency *common.Address) (*big.I
 	return gasPriceMinimum, err
 }
 
+// GetRealGasPriceMinimum is similar to GetRealGasPriceMinimum but if there is
+// a problem retrieving the gas price minimum it will return the error and a
+// nil gas price minimum.
+func GetRealGasPriceMinimum(vmRunner vm.EVMRunner, currency *common.Address) (*big.Int, error) {
+	var currencyAddress common.Address
+	var err error
+
+	if currency == nil {
+		currencyAddress, err = contracts.GetRegisteredAddress(vmRunner, params.GoldTokenRegistryId)
+
+		if err != nil {
+			return nil, fmt.Errorf("failed to retrieve gold token address: %w", err)
+		}
+	} else {
+		currencyAddress = *currency
+	}
+
+	var gasPriceMinimum *big.Int
+	err = getGasPriceMinimumMethod.Query(vmRunner, &gasPriceMinimum, currencyAddress)
+	if err != nil {
+		return nil, fmt.Errorf("failed to retrieve gas price minimum for currency %v, error: %w", currencyAddress.String(), err)
+	}
+
+	return gasPriceMinimum, nil
+}
+
 func GetGasPriceMinimumFloor(vmRunner vm.EVMRunner) (*big.Int, error) {
 	var err error
 

e2e_test/e2e_test.go

@@ -5,6 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"math/big"
+	"os/exec"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -451,3 +453,95 @@ func pruneStateOfBlock(ctx context.Context, node *test.Node, blockHash common.Ha
 
 	return nil
 }
+
+func TestEthersJSCompatibility(t *testing.T) {
+	ac := test.AccountConfig(1, 1)
+	gc, ec, err := test.BuildConfig(ac)
+	require.NoError(t, err)
+	network, shutdown, err := test.NewNetwork(ac, gc, ec)
+	require.NoError(t, err)
+	defer shutdown()
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*20)
+	defer cancel()
+
+	num, err := network[0].WsClient.BlockNumber(ctx)
+	require.NoError(t, err)
+
+	// Execute typescript tests to check ethers.js compatibility.
+	//
+	// The '--networkaddr' and '--blocknum' flags are npm config variables, the
+	// values become available under 'process.env.npm_config_networkaddr' and
+	// 'process.env.npm_config_blocknum' in typescript test. Everything after
+	// '--' are flags that are passed to mocha and these flags are controlling
+	// which tests to run.
+
+	// The tests don't seem to work on CI with IPV6 addresses so we convert to IPV4 here
+	addr := strings.Replace(network[0].Node.HTTPEndpoint(), "[::]", "127.0.0.1", 1)
+
+	cmd := exec.Command("npm", "run", "test", "--networkaddr="+addr, "--blocknum="+hexutil.Uint64(num).String(), "--", "--grep", "ethers.js compatibility tests with state")
+	cmd.Dir = "./ethersjs-api-check/"
+	println("executing mocha test with", cmd.String())
+	output, err := cmd.CombinedOutput()
+	println(string(output))
+	require.NoError(t, err)
+
+	err = network[0].Tracker.AwaitBlock(ctx, num+1)
+	require.NoError(t, err)
+	block := network[0].Tracker.GetProcessedBlock(num)
+
+	// Prune state
+	err = pruneStateOfBlock(ctx, network[0], block.Hash())
+	require.NoError(t, err)
+
+	// Execute typescript tests to check what happens with a pruned block.
+	cmd = exec.Command("npm", "run", "test", "--networkaddr="+addr, "--blocknum="+hexutil.Uint64(num).String(), "--", "--grep", "ethers.js compatibility tests with no state")
+	cmd.Dir = "./ethersjs-api-check/"
+	println("executing mocha test with", cmd.String())
+	output, err = cmd.CombinedOutput()
+	println(string(output))
+	require.NoError(t, err)
+}
+
+// This test checks the functionality of the configuration to enable/disable
+// returning the 'gasLimit' and 'baseFeePerGas' fields on RPC blocks.
+func TestEthersJSCompatibilityDisable(t *testing.T) {
+	ac := test.AccountConfig(1, 1)
+	gc, ec, err := test.BuildConfig(ac)
+	require.NoError(t, err)
+
+	// Check fields present (compatibility set by default)
+	network, shutdown, err := test.NewNetwork(ac, gc, ec)
+	require.NoError(t, err)
+	defer shutdown()
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*20)
+	defer cancel()
+
+	result := make(map[string]interface{})
+	err = network[0].WsClient.GetRPCClient().CallContext(ctx, &result, "eth_getBlockByNumber", "latest", true)
+	require.NoError(t, err)
+
+	_, ok := result["gasLimit"]
+	assert.True(t, ok, "gasLimit field should be present on RPC block")
+	_, ok = result["baseFeePerGas"]
+	assert.True(t, ok, "baseFeePerGas field should be present on RPC block")
+
+	// Turn of compatibility and check fields are not present
+	ec.RPCEthCompatibility = false
+	network, shutdown, err = test.NewNetwork(ac, gc, ec)
+	require.NoError(t, err)
+	defer shutdown()
+
+	ctx, cancel = context.WithTimeout(context.Background(), time.Second*20)
+	defer cancel()
+
+	result = make(map[string]interface{})
+	err = network[0].WsClient.GetRPCClient().CallContext(ctx, &result, "eth_getBlockByNumber", "latest", true)
+	require.NoError(t, err)
+
+	_, ok = result["gasLimit"]
+	assert.False(t, ok, "gasLimit field should not be present on RPC block")
+	_, ok = result["baseFeePerGas"]
+	assert.False(t, ok, "baseFeePerGas field should not be present on RPC block")
+}

e2e_test/ethersjs-api-check/.gitignore

@@ -0,0 +1,3 @@
+node_modules
+dist
+