Skip to content

feat: add support for eam actor in state decode params API#5994

Merged
akaladarshi merged 13 commits intomainfrom
akaladarshi/add-eam-actor-state-decode-param
Sep 3, 2025
Merged

feat: add support for eam actor in state decode params API#5994
akaladarshi merged 13 commits intomainfrom
akaladarshi/add-eam-actor-state-decode-param

Conversation

@akaladarshi
Copy link
Copy Markdown
Collaborator

@akaladarshi akaladarshi commented Aug 25, 2025

Summary of changes

Changes introduced in this pull request:

  • Adds support for the eam actor in state decode params API

Reference issue to close (if applicable)

Closes #5991

Other information and links

File Name Method Name Method Number
filecoin_eam_statedecodeparams_1756139121347152.rpcsnap.json.zst Constructor 1
filecoin_eam_statedecodeparams_1756139121347218.rpcsnap.json.zst Create 2
filecoin_eam_statedecodeparams_1756139121347288.rpcsnap.json.zst Create2 3
filecoin_eam_statedecodeparams_1756139121347364.rpcsnap.json.zst CreateExternal 4

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • New Features

    • Added Ethereum Account Manager (EAM) actor support across versions 10–16 with Constructor, Create, Create2, and CreateExternal methods.
    • Enabled state parameter decoding for EAM methods in the API with consistent JSON (de)serialization for EAM parameters.
  • Tests

    • Added API compare tests and four new snapshots covering EAM state parameter decoding.

@akaladarshi akaladarshi requested a review from a team as a code owner August 25, 2025 16:48
@akaladarshi akaladarshi requested review from LesnyRumcajs and elmattic and removed request for a team August 25, 2025 16:48
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Aug 25, 2025

Walkthrough

Adds LotusJson representations and HasLotusJson mappings for EAM actor params (v10–v16), registers EAM methods in the RPC method registry, exposes an EAM registry module, and adds StateDecodeParams tests and snapshots for EAM methods.

Changes

Cohort / File(s) Summary
LotusJson EAM params & mappings
src/lotus_json/actor_states/methods/eam_params.rs
Adds EAMCreateParamsLotusJson, EAMCreate2ParamsLotusJson, EAMCreateExternalParamsLotusJson and macro-driven HasLotusJson impls for fil_actor_eam_state v10–v16 Create/Create2/CreateExternal (serde/schemars annotations, tests).
Methods module wiring
src/lotus_json/actor_states/methods/mod.rs
Adds mod eam_params; to include the new LotusJson mappings.
RPC registry: EAM module
src/rpc/registry/actors/eam.rs
New file registering Constructor, Create, Create2, CreateExternal for EAM across v10–v16 into MethodRegistry.
RPC registry: actors mod
src/rpc/registry/actors/mod.rs
Adds pub(crate) mod eam; to actors registry modules.
RPC registry: method dispatch
src/rpc/registry/methods_reg.rs
Integrates EAM into actor registration flow; calls eam::register_actor_methods when BuiltinActor::EAM encountered.
API compare tests
src/tool/subcommands/api_cmd/api_compare_tests.rs
Adds eam_actor_state_decode_params_tests and invokes it from state decode params tests for EAM v16 Constructor/Create/Create2/CreateExternal; constructs serialized payloads.
Test snapshots list
src/tool/subcommands/api_cmd/test_snapshots.txt
Adds four new EAM StateDecodeParams snapshot entries.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant RPC as RPC Server
  participant Registry as MethodRegistry
  participant EAM as EAM Registrar
  participant LJ as LotusJson Mappings

  Client->>RPC: Filecoin.StateDecodeParams(actor=EAM, method, payload, tipset)
  RPC->>Registry: Resolve actor CID & version
  Registry->>EAM: register_actor_methods(registry, cid, version)
  Note right of EAM: Registers Constructor, Create, Create2, CreateExternal (v10–v16)
  RPC->>LJ: Use HasLotusJson for vX to decode payload
  LJ-->>RPC: Native params (Create/Create2/CreateExternal)
  RPC-->>Client: JSON result (LotusJson)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective Addressed Explanation
Add EAM support in Filecoin.StateDecodeParams API [#5991]
Version coverage v10–v16 for EAM params [#5991]

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
Duplicate helper function eam_actor_state_decode_params_tests(...) (src/tool/subcommands/api_cmd/api_compare_tests.rs) Duplicate definition appears unrelated to the objective and likely accidental; may affect test compilation or cause duplicate symbol errors.

Possibly related issues

Possibly related PRs

Suggested reviewers

  • elmattic
  • hanabi1224
  • LesnyRumcajs

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a083967 and b08b744.

📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd/test_snapshots.txt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tool/subcommands/api_cmd/test_snapshots.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: tests
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: All lint checks
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (rust)
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch akaladarshi/add-eam-actor-state-decode-param

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Aug 25, 2025
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
src/rpc/registry/methods_reg.rs (2)

100-101: Register EAM methods in the global registry + add unit test coverage

The new match arm correctly dispatches BuiltinActor::EAM to eam::register_actor_methods(self, cid, version). To keep tests aligned with supported actors, extend the test_supported_actor_methods_registered list to include EAM so we catch regressions in registration.

Apply the diff:

@@ fn test_supported_actor_methods_registered() {
-        let supported_actors = vec![
+        let supported_actors = vec![
             BuiltinActor::Account,
             BuiltinActor::Miner,
             BuiltinActor::EVM,
             BuiltinActor::Cron,
             BuiltinActor::DataCap,
+            BuiltinActor::EAM,
         ];

115-150: Minor nit: avoid unnecessary .as_str() when emitting base64 JSON for empty-params

In the register_actor_methods! macro’s empty branch, json!(BASE64_STANDARD.encode(bytes).as_str()) can drop the .as_str()json! accepts String directly. This avoids creating a temporary &str.

- Ok(serde_json::json!(BASE64_STANDARD.encode(bytes).as_str()))
+ Ok(serde_json::json!(BASE64_STANDARD.encode(bytes)))
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

1884-1925: EAM decode-params tests: coverage and minor polish

The four tests cover Constructor (empty), Create, Create2, and CreateExternal with v16 types. Two suggestions:

  • Add a negative test with non-empty bytes for Constructor to ensure the “empty param” branch in the registry encodes raw bytes as base64 (matching Lotus behavior).
  • Optionally add at least one v1X (e.g., v14) param serialization to exercise the version-dispatch in actors/eam.rs, even if the tipset is v16; you can still feed params of other versions for decode parity checks where valid.

Here’s a small add-on test for the Constructor non-empty-bytes case:

@@ fn eam_actor_state_decode_params_tests(tipset: &Tipset) -> anyhow::Result<Vec<RpcTest>> {
-    Ok(vec![
+    Ok(vec![
         RpcTest::identity(StateDecodeParams::request((
             Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR,
             fil_actor_eam_state::v16::Method::Constructor as u64,
             vec![],
             tipset.key().into(),
         ))?),
+        // Constructor with non-empty bytes: should return base64-encoded string
+        RpcTest::identity(StateDecodeParams::request((
+            Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR,
+            fil_actor_eam_state::v16::Method::Constructor as u64,
+            vec![0x82, 0x18, 0x2a, 0x44, 0x12, 0x34, 0x56, 0x78],
+            tipset.key().into(),
+        ))?),
         RpcTest::identity(StateDecodeParams::request((
             Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR,
             fil_actor_eam_state::v16::Method::Create as u64,
             to_vec(&create_params)?,
             tipset.key().into(),
         ))?),
src/rpc/registry/actors/eam.rs (3)

5-5: Remove unused import to avoid warnings.

MethodNum is not used in this module.

-use crate::shim::message::MethodNum;

8-25: Rename macro for clarity and consistency.

The current name register_eam_reg_version! is a bit stuttery. Consider register_eam_for_version!, which matches intent and mirrors similar modules.

-macro_rules! register_eam_reg_version {
+macro_rules! register_eam_for_version {
@@
-        10 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v10),
-        11 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v11),
-        12 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v12),
-        13 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v13),
-        14 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v14),
-        15 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v15),
-        16 => register_eam_reg_version!(registry, cid, fil_actor_eam_state::v16),
+        10 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v10),
+        11 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v11),
+        12 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v12),
+        13 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v13),
+        14 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v14),
+        15 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v15),
+        16 => register_eam_for_version!(registry, cid, fil_actor_eam_state::v16),

Also applies to: 29-36


27-38: Optional: log on unsupported versions to aid diagnostics.

A tiny debug log helps during network upgrades and mismatched actor tables.

-        _ => {}
+        _ => {
+            tracing::debug!("EAM: no registrations for unsupported state version {}", version);
+        }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 81115f8 and a32e2dd.

📒 Files selected for processing (7)
  • src/lotus_json/actor_states/methods/eam_params.rs (1 hunks)
  • src/lotus_json/actor_states/methods/mod.rs (1 hunks)
  • src/rpc/registry/actors/eam.rs (1 hunks)
  • src/rpc/registry/actors/mod.rs (1 hunks)
  • src/rpc/registry/methods_reg.rs (2 hunks)
  • src/tool/subcommands/api_cmd/api_compare_tests.rs (1 hunks)
  • src/tool/subcommands/api_cmd/test_snapshots.txt (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/rpc/reflect/mod.rs (1)
  • request (250-260)
src/blocks/tipset.rs (2)
  • key (336-339)
  • key (530-533)
src/rpc/registry/methods_reg.rs (1)
src/rpc/registry/actors/eam.rs (1)
  • register_actor_methods (27-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
🔇 Additional comments (10)
src/rpc/registry/actors/mod.rs (1)

7-7: Wiring EAM actor module is correct

The new pub(crate) mod eam; is in line with how other actor modules are exposed. No issues.

src/lotus_json/actor_states/methods/mod.rs (1)

9-9: EAMParams HasLotusJson impl coverage confirmed

The bottom of src/lotus_json/actor_states/methods/eam_params.rs invokes:

  • impl_eam_create_params!(16, 15, 14, 13, 12, 11, 10);
  • impl_eam_create2_params!(16, 15, 14, 13, 12, 11, 10);
  • impl_eam_create_external_params!(16, 15, 14, 13, 12, 11, 10);

These macro calls generate HasLotusJson implementations for CreateParams, Create2Params, and CreateExternalParams in versions v10 through v16. No missing impls were found—coverage across all required versions is complete.

src/rpc/registry/methods_reg.rs (3)

76-79: Imported eam module into registry — good

Including eam alongside the other actor modules is necessary for dispatch below. Looks good.


65-73: Error message includes actor type and method — consistent

The bail! message remains helpful and now covers EAM via the registry path above. No change requested.


75-104: Verify EAM CID→actor mapping for all supported versions

The new register_known_methods dispatches BuiltinActor::EAM, but I didn’t find any entries in the actor-registry mapping for EAM code CIDs. Without those, EAM methods will never be registered at runtime. Please double-check that:

  • In src/shim/machine/manifest.rs, all supported EAM versions (v10–v16) map from their code CIDs to BuiltinActor::EAM.
  • In the module where ACTOR_REGISTRY is defined (e.g. src/rpc/registry/actors_reg.rs or its neighbor), the registry includes an entry for each of those CIDs, pairing it with (BuiltinActor::EAM, version).

If any are missing, add lines such as:

 // in ACTOR_REGISTRY
+  Cid("bafy…<v10-cid>") => (BuiltinActor::EAM, 10),
+  Cid("bafy…<v11-cid>") => (BuiltinActor::EAM, 11),+  Cid("bafy…<v16-cid>") => (BuiltinActor::EAM, 16),

so that register_known_methods will invoke eam::register_actor_methods for every EAM version.

src/tool/subcommands/api_cmd/test_snapshots.txt (1)

204-207: Snapshots for EAM decode-params validated

  • Verified exactly four StateDecodeParams::request tests targeting Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR in src/tool/subcommands/api_cmd/api_compare_tests.rs.
  • Confirmed each of the four new snapshot files appears exactly once in src/tool/subcommands/api_cmd/test_snapshots.txt:
    • filecoin_eam_statedecodeparams_1756139121347152.rpcsnap.json.zst
    • filecoin_eam_statedecodeparams_1756139121347218.rpcsnap.json.zst
    • filecoin_eam_statedecodeparams_1756139121347288.rpcsnap.json.zst
    • filecoin_eam_statedecodeparams_1756139121347364.rpcsnap.json.zst
  • Ensured the list is sorted in ascending order and contains no duplicates, guaranteeing deterministic CI behavior.

All checks pass; no further action required.

src/tool/subcommands/api_cmd/api_compare_tests.rs (2)

1879-1882: Include EAM in state-decode-params test suite — good

Adding eam_actor_state_decode_params_tests(tipset)? into the existing matrix ensures we validate both schema and Lotus parity for EAM params alongside other actors.


1884-1925: Params encoding choices look correct

  • Create/Create2 use CBOR via to_vec, matching how other actors’ params are handled.
  • CreateExternal uses the CreateExternalParams newtype — correct for v16.

No functional issues spotted.

src/lotus_json/actor_states/methods/eam_params.rs (2)

10-17: CreateParams mapping LGTM.

RawBytes + base64 via crate::lotus_json is consistent with other actors, and the snapshot looks correct.


112-119: CreateExternal mapping LGTM.

Tuple newtype for RawBytes is straightforward and matches the string snapshot form.

Also applies to: 137-143

Comment on lines +63 to +68
pub struct EAMCreate2ParamsLotusJson {
#[schemars(with = "LotusJson<Vec<u8>>")]
#[serde(with = "crate::lotus_json")]
pub initcode: Vec<u8>,
pub salt: [u8; 32],
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix Create2 LotusJson: base64 encoding for Salt, snapshot mismatch, and redundant/incorrect try_into.

  • Salt is presented as a base64 string in the snapshots, but the struct field is [u8; 32] without serde(with = "crate::lotus_json"), so deserialization would expect an array of numbers, not a string.
  • The snapshot’s expected Self uses salt: [0; 32] while the provided base64 decodes to 0..31, causing the snapshot to fail.
  • try_into() on [u8; 32] to [u8; 32] is unnecessary and, as written, can be incorrect depending on trait impl expectations.

Recommend representing Salt in the LotusJson type as Vec<u8> (base64 via crate::lotus_json) and convert to [u8; 32] in from_lotus_json. Also fix the snapshot to reflect the actual bytes (0..31).

 #[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)]
 #[serde(rename_all = "PascalCase")]
 pub struct EAMCreate2ParamsLotusJson {
     #[schemars(with = "LotusJson<Vec<u8>>")]
     #[serde(with = "crate::lotus_json")]
     pub initcode: Vec<u8>,
-    pub salt: [u8; 32],
+    #[schemars(with = "LotusJson<Vec<u8>>")]
+    #[serde(with = "crate::lotus_json")]
+    pub salt: Vec<u8>,
 }
@@
                     fn snapshots() -> Vec<(serde_json::Value, Self)> {
                         vec![
                             (
                                 json!({
                                     "Initcode": "ESIzRFU=",
                                     "Salt": "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8="
                                 }),
                                 Self {
                                     initcode: hex::decode("1122334455").unwrap(),
-                                    salt: [0; 32],
+                                    salt: [
+                                        0, 1, 2, 3, 4, 5, 6, 7,
+                                        8, 9, 10, 11, 12, 13, 14, 15,
+                                        16, 17, 18, 19, 20, 21, 22, 23,
+                                        24, 25, 26, 27, 28, 29, 30, 31
+                                    ],
                                 },
                             ),
                         ]
                     }
@@
                     fn into_lotus_json(self) -> Self::LotusJson {
                         EAMCreate2ParamsLotusJson {
                             initcode: self.initcode,
-                            salt: self.salt,
+                            salt: self.salt.to_vec(),
                         }
                     }
 
                     fn from_lotus_json(lotus_json: Self::LotusJson) -> Self {
                         Self {
-                            initcode: lotus_json.initcode.into(),
-                            salt: lotus_json.salt.try_into().unwrap_or_else(|_| [0; 32]),
+                            initcode: lotus_json.initcode,
+                            salt: <[u8; 32]>::try_from(lotus_json.salt).unwrap_or_else(|v: Vec<u8>| {
+                                debug_assert_eq!(v.len(), 32, "EAM Create2 salt must be 32 bytes");
+                                [0; 32]
+                            }),
                         }
                     }

Also applies to: 77-91, 93-105

🤖 Prompt for AI Agents
In src/lotus_json/actor_states/methods/eam_params.rs around lines 63-68 (and
similarly at 77-91, 93-105): the salt field is currently [u8;32] but snapshots
provide base64, and there are redundant/incorrect try_into usages; change the
struct to represent salt as a LotusJson base64-backed Vec<u8> (add
#[schemars(with = "LotusJson<Vec<u8>>")] and #[serde(with =
"crate::lotus_json")] on salt), then in from_lotus_json convert the Vec<u8> into
a [u8;32] by verifying length == 32 and copying into an array (return an error
if length mismatches) instead of using unnecessary try_into on an array; update
the snapshot expected salt bytes to the actual decoded sequence (0..31) so tests
align.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akaladarshi What do we do with this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the snapshot test

LesnyRumcajs
LesnyRumcajs previously approved these changes Aug 26, 2025
elmattic
elmattic previously approved these changes Aug 26, 2025
@akaladarshi akaladarshi enabled auto-merge August 26, 2025 13:16
@akaladarshi akaladarshi added this pull request to the merge queue Aug 26, 2025
@akaladarshi akaladarshi removed this pull request from the merge queue due to a manual request Aug 26, 2025
@akaladarshi akaladarshi dismissed stale reviews from LesnyRumcajs and elmattic via f3d4d01 August 26, 2025 13:57
@akaladarshi akaladarshi enabled auto-merge August 26, 2025 13:58
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/lotus_json/actor_states/methods/eam_params.rs (2)

61-68: Create2 Salt must be base64 LotusJson, not a JSON number array; fix representation.

Lotus RPC encodes byte arrays as base64 strings. The EAM Create2 Salt in snapshots is base64 (e.g., AAECAwQF…Hh8=), but here Salt is declared as [u8; 32] without lotus_json, which expects a JSON array of numbers and will not deserialize the RPC payload. This also diverges from the provided RPC snapshots. Represent Salt as a base64-backed Vec in the LotusJson type, and convert to [u8; 32] at the boundary.

Apply this diff to the LotusJson wrapper struct:

 #[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)]
 #[serde(rename_all = "PascalCase")]
 pub struct EAMCreate2ParamsLotusJson {
     #[schemars(with = "LotusJson<Vec<u8>>")]
     #[serde(with = "crate::lotus_json")]
     pub initcode: Vec<u8>,
-    pub salt: [u8; 32],
+    #[schemars(with = "LotusJson<Vec<u8>>")]
+    #[serde(with = "crate::lotus_json")]
+    pub salt: Vec<u8>,
 }

70-109: Create2: update snapshots and conversions; enforce 32-byte Salt.

  • Snapshot should use base64 Salt to match Lotus RPC.
  • into_lotus_json should emit Vec for Salt.
  • from_lotus_json must validate length == 32 and convert Vec → [u8; 32] (fallback or assert on mismatch).
@@
                     #[cfg(test)]
                     fn snapshots() -> Vec<(serde_json::Value, Self)> {
                         vec![
                             (
                                 json!({
                                     "Initcode": "ESIzRFU=",
-                                    "Salt": [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]
+                                    "Salt": "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8="
                                 }),
                                 Self {
                                     initcode: hex::decode("1122334455").unwrap(),
-                                    salt: [0; 32],
+                                    salt: [
+                                        0, 1, 2, 3, 4, 5, 6, 7,
+                                        8, 9, 10, 11, 12, 13, 14, 15,
+                                        16, 17, 18, 19, 20, 21, 22, 23,
+                                        24, 25, 26, 27, 28, 29, 30, 31
+                                    ],
                                 },
                             ),
                         ]
                     }
@@
                     fn into_lotus_json(self) -> Self::LotusJson {
                         EAMCreate2ParamsLotusJson {
                             initcode: self.initcode,
-                            salt: self.salt,
+                            salt: self.salt.to_vec(),
                         }
                     }
 
                     fn from_lotus_json(lotus_json: Self::LotusJson) -> Self {
-                        Self {
-                            initcode: lotus_json.initcode,
-                            salt: lotus_json.salt,
-                        }
+                        let salt_arr = <[u8; 32]>::try_from(lotus_json.salt).unwrap_or_else(|v: Vec<u8>| {
+                            debug_assert_eq!(v.len(), 32, "EAM Create2 salt must be 32 bytes");
+                            [0; 32]
+                        });
+                        Self {
+                            initcode: lotus_json.initcode,
+                            salt: salt_arr,
+                        }
                     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a32e2dd and f3d4d01.

📒 Files selected for processing (1)
  • src/lotus_json/actor_states/methods/eam_params.rs (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests
  • GitHub Check: All lint checks
  • GitHub Check: tests-release
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (4)
src/lotus_json/actor_states/methods/eam_params.rs (4)

10-17: CreateParams JSON mapping looks correct and aligned with Lotus JSON.

Using RawBytes with crate::lotus_json for Initcode preserves base64 semantics; Nonce stays as a u64. Snapshot matches the expected decoded bytes.


19-59: CreateParams impl across v10–v16: conversions and snapshot LGTM.

into_lotus_json/from_lotus_json are correct round-trips; test snapshot is representative.


112-119: CreateExternal newtype wrapper is correct.

Wrapping RawBytes with crate::lotus_json produces the expected base64 string JSON. Snapshot matches.


120-148: CreateExternal impl across v10–v16: looks good.

Round-trips are correct; snapshot is concise and representative.

Comment on lines +150 to +152
impl_eam_create_params!(16, 15, 14, 13, 12, 11, 10);
impl_eam_create2_params!(16, 15, 14, 13, 12, 11, 10);
impl_eam_create_external_params!(16, 15, 14, 13, 12, 11, 10);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Confirm actor versions and broaden test coverage across versions.

EAM is implemented for v10–v16 here. Please confirm the fil_actor_eam_state crate actually exposes Create/Create2/CreateExternal for all these versions and that StateDecodeParams exercises at least one pre-v16 version to catch regressions.

Run the following to sanity-check snapshot presence and test references:

If you want, I can draft additional API compare tests parametrized over versions V10–V16 to ensure the decode path stays compatible.


🏁 Script executed:

#!/usr/bin/env bash
# 1) List EAM StateDecodeParams snapshots added by this PR
fd -a 'filecoin_eam_statedecodeparams_*.rpcsnap.json.zst'

# 2) Inspect API compare tests mention of EAM across versions
rg -n -C2 'EAM|BuiltinActor::EAM|StateDecodeParams' src/tool/subcommands/api_cmd

Length of output: 40337


Add API tests for EAM StateDecodeParams on pre-v16 and include missing snapshots

The macro in src/lotus_json/actor_states/methods/eam_params.rs correctly generates handlers for v10 through v16, but:

  • No new snapshot files matching filecoin_eam_statedecodeparams_*.rpcsnap.json.zst were added by this PR.
  • The existing API compare tests in
    src/tool/subcommands/api_cmd/api_compare_tests.rs only exercise
    fil_actor_eam_state::v16::Method::{Constructor, Create, Create2, CreateExternal}.

To catch regressions in earlier versions (v10–v15), please:

  • Add at least one API compare test for a representative pre-v16 version (e.g. v10) of the EAM actor. For example:
    RpcTest::identity(StateDecodeParams::request((
        Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR,
        fil_actor_eam_state::v10::Method::Create as u64,
        tipset.key().into(),
    )))?,
  • Generate and commit the corresponding snapshot (e.g.
    filecoin_eam_statedecodeparams_v10.rpcsnap.json.zst) under the same fixtures directory.
  • (Optional) Parameterize the API compare tests over versions v10–v16 to streamline full coverage.

Let me know if you’d like help drafting the parametrized tests or snapshot commands.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It would be great to always keep versions in ascending order, rather than sometimes descending. Keeping this uniform makes reading code more easy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elmattic Good catch! You're absolutely right - keeping version numbers in ascending order (10, 11, 12, 13, 14, 15, 16) would be much more readable and consistent.

The macro calls on lines 149-151 should be changed from:

impl_eam_create_params!(16, 15, 14, 13, 12, 11, 10);
impl_eam_create2_params!(16, 15, 14, 13, 12, 11, 10);
impl_eam_create_external_params!(16, 15, 14, 13, 12, 11, 10);

To:

impl_eam_create_params!(10, 11, 12, 13, 14, 15, 16);
impl_eam_create2_params!(10, 11, 12, 13, 14, 15, 16);
impl_eam_create_external_params!(10, 11, 12, 13, 14, 15, 16);

This makes it easier to scan and understand the version progression at a glance.

🐰✨


✏️ Learnings added
Learnt from: elmattic
PR: ChainSafe/forest#5994
File: src/lotus_json/actor_states/methods/eam_params.rs:150-152
Timestamp: 2025-09-02T14:01:57.720Z
Learning: In Forest codebase, version numbers should be listed in ascending order (10, 11, 12, 13, 14, 15, 16) rather than descending order for better readability and consistency.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

elmattic
elmattic previously approved these changes Sep 2, 2025
@akaladarshi akaladarshi requested a review from elmattic September 2, 2025 15:36
LesnyRumcajs
LesnyRumcajs previously approved these changes Sep 2, 2025
@akaladarshi akaladarshi added this pull request to the merge queue Sep 3, 2025
Merged via the queue into main with commit 5acc181 Sep 3, 2025
43 of 44 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/add-eam-actor-state-decode-param branch September 3, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for the EAM actor state in Filecoin.StateDecodeParams API

3 participants