Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Add support for eth_chainId #4975

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Conversation

halfalicious
Copy link
Contributor

@halfalicious halfalicious commented Apr 24, 2018

Fixes #4810
Implements https://github.com/ethereum/EIPs/blob/master/EIPS/eip-695.md

I added a chainId() function to the libethereum\Interface class which returns 0 by default (since chainId values start @ 0x1 with MainNet), along with the corresponding function to libethereum\ClientBase which retrieves the current chain id from BlockChain::chainParams. I then surfaced the function in the JSON-RPC API by adding it to libweb3jsonrpc\eth.json and regenerating EthFace.h via jsonrpcstub, and removing unnecessary changes from the generated file.

I tested my changes on Ubuntu with eth connected to Ropsten and a JSON-RPC server setup via the jsonrpcproxy.py script:

localadminuser@ELLIOTT:~$ curl -X POST --data '{"jsonrpc":"2.0","method":"eth_chainId","params":[],"id":1}' http://localhost:8545
{"id":1,"jsonrpc":"2.0","result":"0x3"}
localadminuser@ELLIOTT:~$ 

…I added a chainId() function to the libethereum\Interface class which returns 0 by default (since chainId values start @ 0x1 with MainNet), along with the corresponding function to libethereum\ClientBase which retrieves the current chain ID from BlockChain::chainParams. I then surfaced the function in the JSON-RPC API by adding it to libweb3jsonrpc\eth.json and regenerating EthFace.h via jsonrpcstub, and removing unnecessary changes from the generated file.
@codecov-io
Copy link

codecov-io commented Apr 24, 2018

Codecov Report

Merging #4975 into develop will decrease coverage by 0.07%.
The diff coverage is 7.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4975      +/-   ##
===========================================
- Coverage    61.08%   61.01%   -0.08%     
===========================================
  Files          350      350              
  Lines        28338    28319      -19     
  Branches      2884     2884              
===========================================
- Hits         17310    17278      -32     
- Misses       10069    10073       +4     
- Partials       959      968       +9

Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides single nit, it looks good to me. @gumb0 would you double check it?

@@ -174,7 +174,9 @@ class ClientBase: public Interface
}

Block block(BlockNumber _h) const;


virtual int chainId() const override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not repeat virtual keyword if override is used.

Copy link
Contributor Author

@halfalicious halfalicious Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @chfast! I followed the approach used by other functions in the class e.g.:

https://github.com/ethereum/cpp-ethereum/blob/3f2ac7c4f9e1543eb4f31cbdcdf8028eb9bfe0b6/libethereum/ClientBase.h#L130

After reading more about the virtual and override keywords, it looks like neither of them are required:

  • Virtual: This keyword is redundant because ClientBase::chainId() is implicitly virtual because it overrides a virtual function (Interface::chainId())
  • Override: This keyword isn't required, but it can be help one avoid runtime bugs since it tells the compiler to verify that there's a function in the parent class with the same signature and to raise an error if that's not the case

As such, I think it makes sense to remove the virtual keyword. However, I can also see the value in keeping both keywords in the interest of code consistency. What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, override is helpful and having both virtual and override is redundant. I'm fine with making it less consistent and leaving only override.
You could also remove all other virtual from other overridden methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gumb0, I've removed virtual from all of the ClientBase functions which override other functions.

@gumb0 gumb0 self-requested a review April 24, 2018 15:43
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@gumb0
Copy link
Member

gumb0 commented Apr 25, 2018

It would be great if you could also document how you generate stub files with jsonrpcstub, because usually no one can't make it work.

…er functions since the keyword is redundant - these functions are implicitly virtual because they override virtual functions.
@chfast chfast merged commit b387b57 into ethereum:develop Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants