[PAN-2373] admin_nodeInfo rpc-api #1012
Conversation
Implement admin_nodeInfo RPC call. This involved bringing data from the ethNetworkConfig and genesis json into the rpc call, so it had a lot of testing side effects.
| @@ -139,6 +146,8 @@ | |||
|
|
|||
| public Map<String, JsonRpcMethod> methods( | |||
| final String clientVersion, | |||
| final int networkId, | |||
| final JsonNode genesisConfigConfig, | |||
There was a problem hiding this comment.
The config element of the genesisConfig.
| "eth", | ||
| ImmutableMap.of( | ||
| "config", | ||
| genesisConfigConfig, |
There was a problem hiding this comment.
Not entirely sure what the right answer is here but I don't particularly like passing the "raw" json around so would be nice to use either the parsed GenesisConfigFile or GenesisConfigOptions here. That would then also ensure we return Pantheon's understanding of the genesis config, not just the raw text which avoids confusion when some setting isn't taking effect because it's misspelt as it wouldn't appear here at all.
I'm not sure if that justifies the extra complexity of having to reserialize the JSON or if there would be some reason to want to have unsupported options funnelled through to here.
There was a problem hiding this comment.
The main question is do we want the literal config elements in the json text or do we want Pantheon's understanding of the options? Since we are doing live nodeInfo I think we want the understanding. Wiring up json de/serialization for the relevant interfaces should do the trick.
There was a problem hiding this comment.
Another option is to ask JsonGenesisConfigOptions for its configRoot, which is already the type we need. But that is the unparsed and uninterpreted values.
There was a problem hiding this comment.
Yeah I was wondering that. The big downside is that it's not quite the raw values (we normalise all keys to lower case) and not just what pantheon understands either.
There was a problem hiding this comment.
Roger. I think I can add an interface method to PantheonController to return the GenesisConfigOptions.
| new ObjectMapper().readTree(ethNetworkConfig.getGenesisConfig()).get("config"); | ||
| } catch (final IOException ioe) { | ||
| genesisConfigConfig = null; | ||
| } |
There was a problem hiding this comment.
Definitely not keen on having to parse the JSON a second time here (as opposed to just in GenesisConfigFile.fromConfig).
There was a problem hiding this comment.
The json de/serialization will address this.
I fudged on eip150Hash since we don't use it anywhere but every config has it I go into the source and retrieve it.
| } | ||
| if (cliqueConfigRoot.containsKey("blockperiodseconds")) { | ||
| builder.put("blockPeriodSeconds", getBlockPeriodSeconds()); | ||
| } |
There was a problem hiding this comment.
Getting pedantic now but I wonder if we should always include this with the default value if none is set? Would make this a really useful API for what values are actually in effect.
There was a problem hiding this comment.
All of our existing json files have them, and a default is not called out in the spec. Consensus specs are a bad place for "this default that we have all agreed to by convention" values, so I'll always report them. Also, it makes the code cleaner. :)
PR description
Implement admin_nodeInfo RPC call. This involved bringing data from the
ethNetworkConfig and genesis json into the rpc call, so it had a lot of
testing side effects.
Fixed Issue(s)
PAN-2373