[Feature] StateMinerFaults RPC. #3726
Conversation
ruseinov
requested review from
LesnyRumcajs and
elmattic
and removed request for
a team
|
I don't understand.
How do we know it's Lotus-compatible? |
tested locally, it's compatible. |
| data.state_manager | ||
| .miner_faults(&address, &ts) | ||
| .map_err(|e| e.into()) | ||
| .map(|r| r.into()) |
There was a problem hiding this comment.
nit: maybe make it more explicit what types we're converting into.
| data.state_manager | |
| .miner_faults(&address, &ts) | |
| .map_err(|e| e.into()) | |
| .map(|r| r.into()) | |
| Ok(LotusJson(data.state_manager.miner_faults(&address, &ts)?)) |
There was a problem hiding this comment.
It lines up with the other endpoints, I see no reason to be explicit here, while being implicit in other methods.
There was a problem hiding this comment.
Also, we'll still need to do map_err.
There was a problem hiding this comment.
Ah, we don't, if we use the ?. I've basically used the same approach I did for the StateMinerPower, which was used in state_market_balance.
I'd say that it would be nice to go with the same approach for all the methods. I've just taken a look and we're pretty much 50/50. In my opinion the best course of action here is to let everybody go with their own flow and once we've finished the RPC compatibility routine - just do a small refactoring to make them look the same.
Cargo.toml
Outdated
| fil_actor_system_state = "7.0.0-rc.5" | ||
| fil_actor_verifreg_state = "7.0.0-rc.5" | ||
| fil_actors_shared = "7.0.0-rc.5" | ||
| fil_actor_account_state = "7.0.0" |
There was a problem hiding this comment.
This will probably clash with #3739
There was a problem hiding this comment.
It might, we'll have several PRs depeding on this version bump.
src/state_manager/mod.rs
Outdated
|
|
||
| let mut faults = Vec::new(); | ||
|
|
||
| state.for_each_deadline(&self.chain_config.policy, self.blockstore(), |_, part| { |
There was a problem hiding this comment.
I prefer a more explicit name here, like partition.
There was a problem hiding this comment.
It's not partition. It's State.
There was a problem hiding this comment.
Ah, you are referring to part?
There was a problem hiding this comment.
Yes I'm referring to part.
There was a problem hiding this comment.
Looks like part should be deadline and dl should be partition.
src/state_manager/mod.rs
Outdated
|
|
||
| let mut faults = Vec::new(); | ||
|
|
||
| state.for_each_deadline(&self.chain_config.policy, self.blockstore(), |_, part| { |
There was a problem hiding this comment.
Looks like part should be deadline and dl should be partition.
Good catch! Fixed. |
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Work on #3639
Other information and links
Tested with:
forest-tool api compare --filter MinerFaults --lotus /dns/127.0.0.1/tcp/1234/http snapshot-latestChange checklist