-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat(rpc): internal F3 RPC methods to power go-f3 sidecar #4646
Conversation
Let's have a tracking issue for that. |
} | ||
|
||
pub enum GetPowerTable {} | ||
impl RpcMethod<1> for GetPowerTable { |
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.
That's a pretty massive method. Can it be refactored?
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.
There's a lot of duplicate code handling different variant of power state, v12-v14 has already been simplified with a macro. v8-v11 are all different. I guess I can update fil-actor-states
to unify the API for v8-v11 to further simplify the code with a single macro, how about doing that in a subsequent PR since it requires a release of fil-actor-states
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.
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 it require a release or can we move that logic entirely to Forest's shim? In any case, I'm good with that as long as it's indeed a follow-up and not an abandonware issue. :)
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.
Yeah I can move the entire thing to forest as well
Co-authored-by: Hubert <[email protected]>
|
let ts = ctx.chain_index().load_required_tipset(&tsk)?; | ||
let state: power::State = ctx.state_manager.get_actor_state(&ts)?; | ||
let mut id_power_worker_mappings = vec![]; | ||
match &state { |
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.
nit: since this match is so long - perhaps it would be more readable to separate this into helper methods?
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 tracked by ChainSafe/fil-actor-states#324
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 to me apart from a massive match block, but I don't consider it a blocker.
I would introduce a CI job that runs the EC tests too, perhaps a different PR is in order, not sure how much of a priority that is though. |
@ruseinov Will do that in a subsequent PR since running the Go tests is a bit more complicated than normal as they depend on a running forest node. |
Summary of changes
Part of #4644
To construct a
go-f3
node, an EC(expected consensus) backend is required. When we run a f3 node as sidecar, we cannot directly pass an EC backend, instead, we expose a set of RPC methods on forest side, and implement a go EC backend with RPC client.Some design considerations:
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist