-
Notifications
You must be signed in to change notification settings - Fork 3
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.
Great work! A few suggestions and potential bugs. It would also be great if we can actually test it against the IPC agent to be sure that it works (at least the call, we can plan the end-to-end tests in a follow-up PR).
chain/consensus/mir/rpc/rpc.go
Outdated
} | ||
|
||
func (c *JSONRPCClient) SendRequest(method string, params interface{}, reply interface{}) error { | ||
paramBytes, err := rpc.EncodeClientRequest(method, params) |
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.
Are we sure that the method an parameters generated from this package are compatible with our implementation in the IPC agent?
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, I asked @cryptoAtwill to create a string-based test in the JSON RPC server repo. Then I can use the same strings (serialized requests) to test them here.
Do you have other ideas?
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 works for now, just to double-check before we get integration tests ready.
chain/consensus/mir/rpc/rpc.go
Outdated
return fmt.Errorf("received status code: %d", resp.StatusCode) | ||
} | ||
|
||
if err := rpc.DecodeClientResponse(resp.Body, reply); err != nil { |
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.
Same here, we should ensure that is compatible with our implementation.
// GetValidatorSet gets the membership config from the actor state. | ||
func (c *ActorMembership) GetValidatorSet() (*Set, error) { | ||
var set Set | ||
err := c.client.SendRequest("Filecoin.GetValidatorSet", nil, &set) |
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 is wrong, the method name is the following: https://github.com/consensus-shipyard/ipc-agent/blob/5815f5955c1eaaf4bf07637e460cfbcee1f2bdf4/src/config/server.rs#L20
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.
Fixed
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.
Thank you for the changes. LGTM.
This PR adds a JSON RPC client to request IPC Agent, adds support for onchain reconfiguration, adds a basic test to check that onchain reconfiguration can be received via a stub JSOn RPC client, adapts CLI.