Add eip1898 support #462

thomas-nguy · 2021-08-20T03:15:24Z

Closes: #460

Description

Add support for eip1898
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1898.md

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

fedekunze

I feel we can clean up the code to avoid code duplication. Also please add tests for UnmarshalJSON and include relevant code comments

fedekunze · 2021-08-20T07:45:53Z

ethereum/rpc/namespaces/eth/api.go

+	var blockNum rpctypes.BlockNumber
+	if blockNrOrHash.BlockHash != nil {
+		blockHeader, err := e.backend.HeaderByHash(*blockNrOrHash.BlockHash)
+		if err != nil {
+			return nil, err
+		}
+		blockNum = rpctypes.NewBlockNumber(blockHeader.Number)
+	} else if blockNrOrHash.BlockNumber != nil {
+		blockNum = *blockNrOrHash.BlockNumber
+	}
+


can we define this as a function to avoid code duplication?

like this?

var foundblock:=getBlock(blockHeader)

fedekunze · 2021-08-20T07:46:23Z

ethereum/rpc/namespaces/eth/api.go

+	} else if blockNrOrHash.BlockNumber != nil {
+		blockNum = *blockNrOrHash.BlockNumber
+	}
+
 	e.logger.Debug("eth_getStorageAt", "address", address.Hex(), "key", key, "block number", blockNum)


I'd define this before the blockNum statement you defined

ethereum/rpc/types/block.go

fedekunze · 2021-08-20T07:51:15Z

ethereum/rpc/types/block.go

+	type erased BlockNumberOrHash
+	e := erased{}
+	err := json.Unmarshal(data, &e)
+	if err == nil {
+		if e.BlockNumber != nil && e.BlockHash != nil {
+			return fmt.Errorf("cannot specify both BlockHash and BlockNumber, choose one or the other")
+		}
+		bnh.BlockNumber = e.BlockNumber
+		bnh.BlockHash = e.BlockHash
+		return nil
+	}


this can be grouped in one private function that checks for the unmarshaling of the full struct

fedekunze · 2021-08-20T07:51:52Z

ethereum/rpc/types/block.go

+	var input string
+	err = json.Unmarshal(data, &input)
+	if err != nil {
+		return err
+	}
+	switch input {
+	case "earliest":
+		bn := EthEarliestBlockNumber
+		bnh.BlockNumber = &bn
+		return nil
+	case "latest":
+		bn := EthLatestBlockNumber
+		bnh.BlockNumber = &bn
+		return nil
+	case "pending":
+		bn := EthPendingBlockNumber
+		bnh.BlockNumber = &bn
+		return nil
+	default:
+		if len(input) == 66 {
+			hash := common.Hash{}
+			err := hash.UnmarshalText([]byte(input))
+			if err != nil {
+				return err
+			}
+			bnh.BlockHash = &hash
+			return nil
+		}
+
+		blckNum, err := hexutil.DecodeUint64(input)
+		if err != nil {
+			return err
+		}
+		if blckNum > math.MaxInt64 {
+			return fmt.Errorf("blocknumber too high")
+		}
+		bn := BlockNumber(blckNum)
+		bnh.BlockNumber = &bn
+		return nil


this can also be grouped

leejw51crypto · 2021-08-20T08:14:59Z

ethereum/rpc/types/block.go

+		bnh.BlockNumber = &bn
+		return nil
+	default:
+		if len(input) == 66 {


why 66 length?

It is the size of the hash string in ethereum ( "0x" + 2 * 32)

leejw51crypto · 2021-08-20T08:15:47Z

ethereum/rpc/types/block.go

+		if err != nil {
+			return err
+		}
+		if blckNum > math.MaxInt64 {


int64 is enough ?

yes, blockNumber is coded into an int64

ethereum/rpc/types/block.go

leejw51crypto · 2021-08-20T08:18:23Z

ethereum/rpc/namespaces/eth/api.go

+		}
+		blockNum = rpctypes.NewBlockNumber(blockHeader.Number)
+	} else if blockNrOrHash.BlockNumber != nil {
+		blockNum = *blockNrOrHash.BlockNumber


as full word like blockNumber ?

codecov · 2021-08-23T02:13:12Z

Codecov Report

Merging #462 (56d3828) into main (c0ca43f) will decrease coverage by 2.47%.
The diff coverage is 70.68%.

@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
- Coverage   50.38%   47.91%   -2.48%     
==========================================
  Files          51       56       +5     
  Lines        5005     5374     +369     
==========================================
+ Hits         2522     2575      +53     
- Misses       2373     2683     +310     
- Partials      110      116       +6

Impacted Files	Coverage Δ
ethereum/rpc/types/block.go	`50.47% <70.68%> (ø)`
ethereum/rpc/types/addrlock.go	`0.00% <0.00%> (ø)`
ethereum/rpc/types/utils.go	`0.00% <0.00%> (ø)`
ethereum/rpc/types/query_client.go	`0.00% <0.00%> (ø)`
ethereum/rpc/types/types.go	`0.00% <0.00%> (ø)`

fedekunze · 2021-08-23T07:45:25Z

ethereum/rpc/namespaces/eth/api.go

+	if blockNrOrHash.BlockHash != nil {
+		blockHeader, err := e.backend.HeaderByHash(*blockNrOrHash.BlockHash)
+		if err != nil {
+			return blockNum, err
+		}
+		blockNum = rpctypes.NewBlockNumber(blockHeader.Number)
+	} else if blockNrOrHash.BlockNumber != nil {


please use switch with the following cases:

BlockHash == nil && BlockNumber == nil --> error
BlockHash != nil
BlockNumber != nil
default : EthLatestBlockNumber

fedekunze · 2021-08-23T07:46:03Z

ethereum/rpc/types/block.go

+
+func (bnh *BlockNumberOrHash) decodeFromString(input string) error {
+	switch input {
+	case "earliest":


don't we use constants for these strings?

not at the moment, but I can do the change

ethereum/rpc/types/block.go

ethereum/rpc/types/block_test.go

fedekunze

ACK, can you also update the docs on endpoints.md? 🙏

thomas-nguy requested review from fedekunze and khoslaventures as code owners August 20, 2021 03:15

fedekunze suggested changes Aug 20, 2021

View reviewed changes

leejw51crypto reviewed Aug 20, 2021

View reviewed changes

ethereum/rpc/types/block.go Show resolved Hide resolved

leejw51crypto reviewed Aug 20, 2021

View reviewed changes

fedekunze reviewed Aug 23, 2021

View reviewed changes

fedekunze approved these changes Aug 24, 2021

View reviewed changes

fedekunze enabled auto-merge (squash) August 24, 2021 12:45

thomas-nguy added 18 commits August 25, 2021 10:04

support for eip1898

90dfca3

update changelog

ab860be

fix linter

18cb29e

fix linter

9392a39

refactor code

e466500

add test

013bbd2

cleanup code

6b7d069

add comment

4c47c0d

support for eip1898

35fb189

fix linter

4d4c3f8

fix linter

426d392

refactor code

9717e4e

add test

fcf6ef3

create const for block param

de8e8fc

change default to be earliest to be consistant with geth

f5e8882

correct comment

4b6dc97

update doc

99164d9

update doc

56d3828

thomas-nguy force-pushed the thomas/460-eip1898-support branch from 0c02e32 to 56d3828 Compare August 25, 2021 01:13

fedekunze merged commit 4ea3cc1 into evmos:main Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eip1898 support #462

Add eip1898 support #462

thomas-nguy commented Aug 20, 2021

fedekunze left a comment

fedekunze Aug 20, 2021

leejw51crypto Aug 20, 2021

thomas-nguy Aug 23, 2021

fedekunze Aug 20, 2021

thomas-nguy Aug 23, 2021

fedekunze Aug 20, 2021

thomas-nguy Aug 23, 2021

fedekunze Aug 20, 2021

thomas-nguy Aug 23, 2021

leejw51crypto Aug 20, 2021

thomas-nguy Aug 23, 2021

leejw51crypto Aug 20, 2021

thomas-nguy Aug 23, 2021

leejw51crypto Aug 20, 2021

thomas-nguy Aug 23, 2021

codecov bot commented Aug 23, 2021 •

edited

Loading

fedekunze Aug 23, 2021

thomas-nguy Aug 24, 2021

fedekunze Aug 23, 2021

thomas-nguy Aug 24, 2021

fedekunze left a comment

Add eip1898 support #462

Add eip1898 support #462

Conversation

thomas-nguy commented Aug 20, 2021

Description

fedekunze left a 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

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

codecov bot commented Aug 23, 2021 • edited Loading

Codecov Report

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

fedekunze left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2021 •

edited

Loading