-
Notifications
You must be signed in to change notification settings - Fork 112
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
dot/rpc: Implement RPC system_name, system_version, system_chain #849
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this exclude it from the config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
import "github.com/ChainSafe/gossamer/dot/types" | ||
|
||
// Service struct to hold rpc service data | ||
type Service struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could be be part of the rpc or core service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// RPC Service | ||
|
||
// check if rpc service is enabled | ||
if enabled := RPCServiceEnabled(cfg); enabled { | ||
|
||
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -21,12 +21,11 @@ import ( | |||
"strconv" | |||
"strings" | |||
|
|||
database "github.com/ChainSafe/chaindb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this intentionally moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Changes
Added code to implement RPC calls:
Tests:
Checklist:
Issues: