Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ETCM-110] Add eth_chainId RPC method #683

Merged
merged 2 commits into from
Sep 23, 2020
Merged

Conversation

kapke
Copy link
Contributor

@kapke kapke commented Sep 21, 2020

Description

According to Ethereum RPC Spec, Mantis lacks eth_chainId method, which makes it much harder to issue transactions from Luna.

PS. This PR got so big due to auto-formatting. Changes crucial for this task are really minimal (test for eth service, test for rpc controller, insomnia entry, controller entry, json codec, eth service method).

Testing

  1. All tests should pass
  2. eth_chainId in Insomnia should return configured value in hex format according to spec (https://playground.open-rpc.org/?schemaUrl=https://raw.githubusercontent.com/etclabscore/ethereum-json-rpc-specification/master/openrpc.json&uiSchema%5BappBar%5D%5Bui:input%5D=false)

@kapke kapke added the bug Something isn't working label Sep 21, 2020
@kapke kapke requested review from pslaski and mmrozek September 21, 2020 12:34
@kapke kapke self-assigned this Sep 21, 2020
@kapke kapke force-pushed the etcm-110-eth_chainid_rpc-method branch from da5d362 to 749f5fe Compare September 21, 2020 12:40
Copy link
Contributor

@mmrozek mmrozek left a comment

Choose a reason for hiding this comment

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

Minor comments only. LGTM!

implicit val eth_chainId = new JsonDecoder[ChainIdRequest] with JsonEncoder[ChainIdResponse] {
def decodeJson(params: Option[JArray]) = params match {
case None | Some(JArray(Nil)) => Right(ChainIdRequest())
case other => Left(InvalidParams("eth_chainId method does not take params"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized that other parameterless methods don't check if params is empty. Maybe we could improve that in this PR. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If only @pslaski agrees (this will result in quite huge PR due to formatting)

Copy link
Contributor

Choose a reason for hiding this comment

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

this PR is already huge so go for it ;)

@kapke kapke force-pushed the etcm-110-eth_chainid_rpc-method branch 2 times, most recently from 1d59a1f to d7fa62c Compare September 22, 2020 10:40
Copy link
Contributor

@pslaski pslaski left a comment

Choose a reason for hiding this comment

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

LGTM, minor: I'm not sure if there are tests in JRC specs for the non-params case

@@ -23,11 +25,32 @@ object JsonRpcController {
trait JsonDecoder[T] {
def decodeJson(params: Option[JArray]): Either[JsonRpcError, T]
}
object JsonDecoder {
abstract class NoParamsDecoder[T](request: => T) extends JsonDecoder[T] {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@kapke kapke force-pushed the etcm-110-eth_chainid_rpc-method branch from d7fa62c to 8f34a23 Compare September 23, 2020 12:25
@kapke
Copy link
Contributor Author

kapke commented Sep 23, 2020

@pslaski From what I've checked there usually are such tests for happy path and I think it's enough since the behavior is uniform now across whole RPC API

@kapke kapke merged commit 1668edf into develop Sep 23, 2020
@kapke kapke deleted the etcm-110-eth_chainid_rpc-method branch September 23, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants