dot/rpc: Implement RPC system_name, system_version, system_chain#849
dot/rpc: Implement RPC system_name, system_version, system_chain#849edwardmack merged 9 commits intodevelopmentfrom
Conversation
noot
left a comment
noot
left a comment
There was a problem hiding this comment.
it would be preferred not to pass in cli.Context to new RPC service
instead of go struct so that it can be build dynamically later based on genesis definition.
|
I've refactored this, it's ready for review. |
| Core CoreConfig `toml:"core"` | ||
| Network NetworkConfig `toml:"network"` | ||
| RPC RPCConfig `toml:"rpc"` | ||
| System types.SystemInfo `toml:"-"` |
There was a problem hiding this comment.
does this exclude it from the config file?
| import "github.com/ChainSafe/gossamer/dot/types" | ||
|
|
||
| // Service struct to hold rpc service data | ||
| type Service struct { |
There was a problem hiding this comment.
this looks good! however, it doesn't look like it's started when the node is started in dot NewNode?
There was a problem hiding this comment.
I don't have this started because there is no go routine to start, so I guess it's not really a service. It's currently a place to hold static chain and app values. Should I implement the Service interface to add the Start and Stop functions and add call to start so it's treated like other services (for consistency, and in case we add a go routine later). OR should I treat this differently since it's not really a service, and if so, how?
There was a problem hiding this comment.
Maybe it could be be part of the rpc or core service?
There was a problem hiding this comment.
maybe just rename it from Service to Info or something like that, since there isn't a need to make it into an actual service
There was a problem hiding this comment.
I made it implement the Service interface, so it's handled like the others. I was running into issues with cyclic references when I tried making it a part of rpc service. This seems ok since it's following the pattern (now) that we're using for the others.
noot
left a comment
noot
left a comment
There was a problem hiding this comment.
looks good! doesn't look like the system service is started anywhere though, maybe you wanted to add that here?
There was a problem hiding this comment.
This looks great! You mentioned wanting to make changes on the call. Are you planning on initializing the system service in the dot node initialization process as part of this pull request?
If you think it is going to be a service (adhere to the service interface) and should be its own service, I think it works as is. Approved as is if you were planning or would prefer to save those changes.
ryanchristo
left a comment
ryanchristo
left a comment
There was a problem hiding this comment.
Nice work! Changes look good to me. Looks like integration tests are failing but unrelated and might just need to be rerun.
|
|
||
| // create system service and append to node services | ||
| sysSrvc := createSystemService(&cfg.System) | ||
| nodeSrvcs = append(nodeSrvcs, sysSrvc) |
|
|
||
| // create rpc service and append rpc service to node services | ||
| rpcSrvc := createRPCService(cfg, stateSrvc, coreSrvc, networkSrvc, rt) | ||
| rpcSrvc := createRPCService(cfg, stateSrvc, coreSrvc, networkSrvc, rt, sysSrvc) |
| "strconv" | ||
| "strings" | ||
|
|
||
| database "github.com/ChainSafe/chaindb" |
There was a problem hiding this comment.
was this intentionally moved?
There was a problem hiding this comment.
no, maybe my IDE did it when I did goimports? not sure.
* implement RPC call system_name, system_version * implement RPC call system_chain * added tests * save commit, refactoring rpc module * move system rpc api to it's own package * treat system_properties response as map instead of go struct so that it can be build dynamically later based on genesis definition. * make system service implement Service interface
3 participants
Changes
Added code to implement RPC calls:
Tests:
Checklist:
Issues: