Issue #17: Implemented eth_getBlockByNumber() #30

yurenju · 2018-06-13T00:17:32Z

No description provided.

coveralls · 2018-06-13T00:23:24Z

Pull Request Test Coverage Report for Build 41

133 of 141 (94.33%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 90.558%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tests/rpc/rpc.js	18	20	90.0%
tests/rpc/eth/getBlockByNumber.js	53	59	89.83%

Totals
Change from base Build 40:	0.4%
Covered Lines:	182
Relevant Lines:	192

💛 - Coveralls

holgerd77

Hi @yurenju, thanks so much for this PR, this could have come straight from a textbook, really like this clean structure you have already set up and the completeness with the tests and everything.

Have made some comments directly in the code.

P.S.: Croatia was great, we already visited Rovinj, still have all these beautiful sights swirling around in my head. 🏖 ⛪️ 🐚

holgerd77 · 2018-06-18T12:08:25Z

lib/rpc/rpc-manager.js

@@ -0,0 +1,21 @@
+const modules = require('./modules')
+
+class RPCServer {


This is probably RPCManager?

I forgot to rename this one, nice catch!

holgerd77 · 2018-06-18T12:15:18Z

lib/index.js

+
+  if (config.rpc) {
+    const rpc = new RPCServer(cm.getBlockchain())
+    rpc.listen(config.rpcport)


Can you add a info log msg either here or in the server constructor (not sure which one is more clean?) that the server has been started, also mentioning the port?

I would like to follow convention from other modules to have an argument config into this module and save _config and _logger into RPCServer and RPCManager

holgerd77 · 2018-06-18T12:43:30Z

lib/rpc/modules/eth.js

+      cb(err, block.toJSON(true))
+    })
+  }
+}


Since you already mentioned Ganache in the issue description: I just looked into the ganache-core code a bit and I am generally wondering if we are replicating a lot of of the code here (or will do in the future) if one is looking at e.g. this.

I tried to extract the structure from there a bit but have not yet a clear view on the similarities and differences, they seem to have there own blockchain implementation and statemanager. Do you have already formed an opinion on this?

Might be worth to analyze this a bit deeper before we start implementing all the RPC methods.

Which part do you want to replicate from ganache-core?

took a look I feel we should expand argument like function(block_number, include_full_transactions, callback) instead of (params, cb) to make it more clear.

I'm not familiar with ganache-core codebase, so I don't have any opinion for that yet, but I agree we should analyze ganache-core deeper to get a better idea to implement rpc.

holgerd77 · 2018-06-18T13:05:58Z

tests/rpc.js

+    t.equal(blockchain.getBlock.firstCall.args[0], 1)
+    t.end()
+  })
+})


So great that you already added some framing for test execution, really like this sinon library, didn't know that before.

I think our general reference for RPC execution should be Geth behavior, this is also really practical to test running the JS client and Geth with RPC and different port side by side and then have a look at the results.

I've done this with JS 8545 and Geth 8546 and get divergent behavior e.g. for:

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x1", true],"id":1}' http://127.0.0.1:8545 {...JSON Result...} curl -X POST --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x1", true],"id":1}' http://127.0.0.1:8546 invalid content type, only application/json is supported

So Geth requires a content header -H "Content-Type: application/json" which I think we should then require as well.

Also things like leading zeros (0x01) differ:

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x01", true],"id":1}' -H "Content-Type: application/json" http://127.0.0.1:8545|jq {...JSON Result...} curl -X POST --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x01", true],"id":1}' -H "Content-Type: application/json" http://127.0.0.1:8546|jq % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 197 100 117 100 80 73079 49968 --:--:-- --:--:-- --:--:-- 114k { "jsonrpc": "2.0", "id": 1, "error": { "code": -32602, "message": "invalid argument 0: hex number with leading zero digits" } }

Can you test some basic edge cases (e.g. not-existing/downloaded block) and see how things behave and then adopt the code base to make sure that the output is the same?

Hi @yurenju, I would probably merge #29 before this PR and then ask you to rebase on top, if you see any problems here let me know.

@holgerd77 sure go ahead! I may update my pr in two days when I get more free time :D, will rebase after #29 is landed.

and thanks for testing with js client and geth side by side, I will add some basic edge cases!

I feel the way I handled rpc is too simple since we need to fit JSON-RPC spec, e.g., batch query of JSON-RPC. there are few Node.js packages for JSON-RPC and seems jayson is a well-maintain solution.

so jayson module will be introduced on next commit.

https://github.com/tedeh/jayson

To be honest, I didn't even know until now that JSON-RPC is a dedicated standard, lol. Yes, that makes definitely sense then, library seems to be a good choice on first look.

yurenju · 2018-06-24T15:51:24Z

Just heads up I didn't finish my commit this weekend but already added some edge test case for invalid parameters, only accept content-type as application/json, etc.

I pushed my WIP commit to the previous PR to show the progress. hopefully I can finish this commit this week.

holgerd77 · 2018-06-25T10:41:54Z

Ah, already running this with jayson, cool! 😄 Thanks for re-thinking this so constantly, this will be really really valuable if we have a solid structure here.

I now merged #29, can you rebase your PR on top?

yurenju · 2018-06-25T15:40:42Z

just rebased it. the last commit introduces some changes:

test cases for:
- http status code should be 415 if Content-Type is not application/json
- returns error if method does not exist
- returns error if parameter is invalid, include hex and boolean checker
jayson: JSON-RPC module implementation with batch request and correct status code
middleware for validation

let me know if you found anything can be improved :D

holgerd77 · 2018-06-28T10:08:25Z

@yurenju Thanks again, looking into it right now (think I will learn a lot reviewing your work...).

holgerd77

Hi Yuren, really wanted to criticize something but didn't found anything, thanks again for the fantastic work!

Also tested this locally, will merge now.

holgerd77 · 2018-06-28T10:25:43Z

lib/cliParser.js

+  .option('rpcaddr', {
+    describe: 'HTTP-RPC server listening interface',
+    default: 'localhost'
+  })


Nice that you also added the rpcaddr now.

holgerd77 · 2018-06-28T10:27:33Z

lib/index.js

+    const manager = new RPCManager(cm.getBlockchain(), config)
+    const server = jayson.server(manager.getMethods())
+    config.logger.info(`HTTP endpoint opened: http://${rpcaddr}:${rpcport}`)
+    server.http().listen(rpcport)


Was a bit sceptical about the naming but wasn't aware that consts in an if clause remain local not trickling in the global space (tested this), so this should be ok.

holgerd77 · 2018-06-28T10:28:03Z

lib/rpc/modules/eth.js

+
+    this.getBlockByNumber = middleware(this.getBlockByNumber.bind(this), 2,
+      [validators.hex, validators.bool])
+  }


This is a really super-nice middleware structure.

holgerd77 · 2018-06-28T10:32:51Z

lib/rpc/validation.js

+   */
+  middleware (method, requiredParamsCount, validators) {
+    return function (params, cb) {
+      if (params.length < requiredParamsCount) {


Wonder if this should be a !== comparison and a bit more general error msg, but we can leave this for now. Another possibility would also to make an extra check for >, this is maybe even more user-friendly for the caller.

I did this because want to align with go-ethereum behaviors, but you are right we can handle this better than geth :D

holgerd77 · 2018-06-28T10:47:10Z

tests/rpc/eth/getBlockByNumber.js

+    id: 1
+  }
+
+  request(server)


Nice setup with this supertest library - didn't know that yet - really sleak.

holgerd77 · 2018-06-28T10:59:15Z

For testing this locally:

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x1b4", true],"id":1}' -H "Content-Type: application/json" http://127.0.0.1:8545|jq

(jq is for (optional) formatting and has to be installed manually before)

Result:

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "header": {
      "parentHash": "0xe99e022112df268087ea7eafaf4790497fd21dbeeb6bd7a1721df161a6657a54",
      "uncleHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
      "coinbase": "0xbb7b8287f3f0a933474a79eae42cbca977791171",
      "stateRoot": "0xddc8b0234c2e0cad087c8b389aa7ef01f7d79b2570bccb77ce48648aa61c904d",
      "transactionsTrie": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
      "receiptTrie": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
      "bloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
      "difficulty": "0x04ea3f27bc",
      "number": "0x01b4",
      "gasLimit": "0x1388",
      "gasUsed": "0x",
      "timestamp": "0x55ba467c",
      "extraData": "0x476574682f4c5649562f76312e302e302f6c696e75782f676f312e342e32",
      "mixHash": "0x4fffe9ae21f1c9e15207b1f472d5bbdd68c9595d461666602f2be20daf5e7843",
      "nonce": "0x689056015818adbe"
    },
    "transactions": [],
    "uncleHeaders": []
  }
}

yurenju force-pushed the issue-17 branch from 5874034 to 6e0a8b6 Compare June 13, 2018 00:22

holgerd77 suggested changes Jun 18, 2018

View reviewed changes

yurenju force-pushed the issue-17 branch from ea8a962 to af1fae1 Compare June 24, 2018 15:49

yurenju added 2 commits June 25, 2018 23:23

Issue #17: Implemented eth_getBlockByNumber()

de79b53

addressed reviewer's comments (will be squashed)

4e71755

yurenju force-pushed the issue-17 branch from af1fae1 to 4e71755 Compare June 25, 2018 15:33

holgerd77 approved these changes Jun 28, 2018

View reviewed changes

holgerd77 merged commit d42166e into ethereumjs:master Jun 28, 2018

holgerd77 mentioned this pull request Dec 4, 2020

Client JSON-RPC Support (old) ethereumjs/ethereumjs-monorepo#982

Closed

42 tasks

		@@ -0,0 +1,21 @@
		const modules = require('./modules')

		class RPCServer {

Issue #17: Implemented eth_getBlockByNumber() #30

Issue #17: Implemented eth_getBlockByNumber() #30

Uh oh!

Conversation

yurenju commented Jun 13, 2018

Uh oh!

coveralls commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 41

💛 - Coveralls

Uh oh!

holgerd77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurenju Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurenju commented Jun 24, 2018

Uh oh!

holgerd77 commented Jun 25, 2018

Uh oh!

yurenju commented Jun 25, 2018

Uh oh!

holgerd77 commented Jun 28, 2018

Uh oh!

holgerd77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holgerd77 commented Jun 28, 2018

Uh oh!

Uh oh!

coveralls commented Jun 13, 2018 •

edited

Loading

yurenju Jun 23, 2018 •

edited

Loading