-
Notifications
You must be signed in to change notification settings - Fork 166
fix: filecoin read state API #5620
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
fix: filecoin read state API #5620
Conversation
3adf408
to
122015d
Compare
Updated the |
Could you add API compare tests and RPC snapshot tests for each type of actor state? |
… rpc snapshot tests
filecoin_statereadstate_1747857537534929.rpcsnap.json.zst | ||
filecoin_statereadstate_1747857537535135.rpcsnap.json.zst | ||
filecoin_statereadstate_1747857537535290.rpcsnap.json.zst | ||
filecoin_statereadstate_1747857537534606.rpcsnap.json.zst | ||
filecoin_statereadstate_1747857537535026.rpcsnap.json.zst | ||
filecoin_statereadstate_1747857537535209.rpcsnap.json.zst |
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.
We really should have a way of commenting on what those snapshots are. Obviously not something to be done in this PR, but what we could do here is to add something to the name, e.g., statereadstate_account_1234.rpcsnap.json.zst
, or something like this.
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, but we would have to manually change it after generating the files, also we'd need to fix the format for the file name as well.
tipset.key().into(), | ||
))?), | ||
RpcTest::identity(StateReadState::request(( | ||
Address::new_id(78216), // miner actor address `f078216` |
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.
Address::new_id(78216), // miner actor address `f078216` | |
Address::new_id(78216), // miner actor address `t078216` |
Also, might be worth linking to a relevant page on a block explorer.
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.
Done.
src/rpc/actor_registry.rs
Outdated
|
||
match actor_type { | ||
BuiltinActor::Account => { | ||
let state = account::State::load(store, *code_cid, *state_cid).map_err(|e| { |
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.
Instead of map_err
, we could probably use anyhow
's .context
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'd end up being less code.
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.
Refactored it.
src/rpc/actor_registry.rs
Outdated
// Build a map from CIDs to actor types and versions once at startup | ||
static CID_TO_ACTOR_TYPE: Lazy<HashMap<Cid, (BuiltinActor, u64)>> = Lazy::new(|| { | ||
let mut map = HashMap::new(); | ||
|
||
// Populate the map using data from ACTOR_BUNDLES_METADATA | ||
for ((_, _), metadata) in ACTOR_BUNDLES_METADATA.iter() { | ||
if let Ok(version) = metadata.actor_major_version() { | ||
for (actor_type, cid) in metadata.manifest.builtin_actors() { | ||
map.insert(cid, (actor_type, version)); | ||
} | ||
} | ||
} | ||
|
||
map | ||
}); | ||
|
||
pub fn get_actor_type_from_code(code_cid: &Cid) -> anyhow::Result<(BuiltinActor, u64)> { | ||
CID_TO_ACTOR_TYPE | ||
.get(code_cid) | ||
.copied() | ||
.ok_or_else(|| anyhow!("Unknown actor code CID: {}", code_cid)) | ||
} |
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: it'd be nice to have it as a separate struct so we can abstract away the underlying data structure. It'd only have read_actor_type(cid: &Cid)
as a member method.
A unit test would also be nice to have.
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.
Done.
}; | ||
} | ||
|
||
impl_tombstone_lotus_json!(V16, V15, V14, V13, V12, V11, V10); |
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.
Can we ensure, at compilation time, that all versions are covered? If it's too complex, let's have it as a follow-up.
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.
You mean through tests?.
Because I think this is already ensured as we are implementing HasLotusJson
for the TombstoneState, which itself holds all the versions of the tombstone. and match
on self will ensure that at compile time all the versions are covered.
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.
Ah cool, didn't notice there was a match there. Excellent!
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.
Excellent work @akaladarshi !
10bf489
to
12aac4d
Compare
12aac4d
to
0874d91
Compare
@LesnyRumcajs I will raise a subsequent PR for adding more actor type support for Then we can update the |
Either works fine for me. |
Will do it in a Subsequent PR |
Summary of changes
Changes introduced in this pull request:
actor_states
module, which basically holds all the actor state implementing theHasLotusJson
traitactor_registgry
reverse lookup map forCID -> BuiltinActor
StateReadState
API to work with new changesReference issue to close (if applicable)
Partially Closes: #5596
Other information and links
We need to add remaining actor support as well (we could do this in separate PR's for the sake of reviewing smaller diff)
Change checklist