Enable strict json check in api compare#6970
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughReplace flattened message wrapper types with canonical ChangesSchema & Runtime shape → wiring → tests
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC
participant StateManager
participant IPLDStore
Client->>RPC: Call ChainGetMessage / GasEstimateMessageGas
RPC->>StateManager: lookup message & run execution trace
StateManager->>IPLDStore: perform IPLD Get/Put operations
IPLDStore-->>StateManager: return IPLD results
StateManager-->>RPC: return Message + ExecutionTrace (Logs, IpldOps)
RPC-->>Client: JSON response (Message schema)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
sudo-shashank
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/state.rs (1)
3234-3284:⚠️ Potential issue | 🟠 Major
upgrade_xx_heightshould be config-driven, not hardcoded.Line 3284 hardcodes a sentinel epoch, so
StateGetNetworkParamscan diverge from the actual network config once that upgrade is scheduled (or on custom networks). Please source this value fromChainConfig(with an explicit fallback policy if needed) rather than embedding a literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/state.rs` around lines 3234 - 3284, The code currently hardcodes upgrade_xx_height to 999_999_999_999_999 in TryFrom<&ChainConfig> for ForkUpgradeParams; instead, read the value from ChainConfig (use the existing height lookup helper get_height or an explicit ChainConfig field for the XX upgrade) and assign that to upgrade_xx_height, with a clear fallback policy (e.g., use get_height(XX)? or fallback to config.some_optional_xx_height.unwrap_or(default_epoch)) so StateGetNetworkParams stays in sync with the network config. Locate this change in the try_from implementation that constructs ForkUpgradeParams and replace the literal with the config-driven lookup and fallback.
🧹 Nitpick comments (2)
src/lotus_json/message.rs (1)
120-120: Optional readability tweak for intentionally ignored CID.Consider using an explicit ignored binding name and short comment for parity with
signed_message.rs.Suggested tiny cleanup
- cid: _, + cid: _ignored_cid, // CID is derived from message fields; input CID is not used🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lotus_json/message.rs` at line 120, The ignored CID field in message.rs is currently written as cid: _, which is less explicit; update the pattern to use an explicit ignored binding like cid: _cid and add a short comment (e.g., // intentionally ignored) to match the style used in signed_message.rs so the intent is clear when reading the Message destructuring.src/rpc/methods/state/types.rs (1)
204-213: Consider adding a short struct-level note explaining whylogs/ipld_opsare excluded from equality.The code is fine as-is; this would just make intent easier to retain for future changes around API-compare behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/state/types.rs` around lines 204 - 213, Add a short doc comment on the ExecutionTrace struct explaining why logs and ipld_ops are intentionally excluded from the PartialEq implementation: they are implementation-dependent and not part of API-level equality comparison. Update the struct-level comment for ExecutionTrace (referencing fields logs and ipld_ops and the PartialEq impl) to state this intent so future maintainers understand why eq ignores those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/rpc/methods/state.rs`:
- Around line 3234-3284: The code currently hardcodes upgrade_xx_height to
999_999_999_999_999 in TryFrom<&ChainConfig> for ForkUpgradeParams; instead,
read the value from ChainConfig (use the existing height lookup helper
get_height or an explicit ChainConfig field for the XX upgrade) and assign that
to upgrade_xx_height, with a clear fallback policy (e.g., use get_height(XX)? or
fallback to config.some_optional_xx_height.unwrap_or(default_epoch)) so
StateGetNetworkParams stays in sync with the network config. Locate this change
in the try_from implementation that constructs ForkUpgradeParams and replace the
literal with the config-driven lookup and fallback.
---
Nitpick comments:
In `@src/lotus_json/message.rs`:
- Line 120: The ignored CID field in message.rs is currently written as cid: _,
which is less explicit; update the pattern to use an explicit ignored binding
like cid: _cid and add a short comment (e.g., // intentionally ignored) to match
the style used in signed_message.rs so the intent is clear when reading the
Message destructuring.
In `@src/rpc/methods/state/types.rs`:
- Around line 204-213: Add a short doc comment on the ExecutionTrace struct
explaining why logs and ipld_ops are intentionally excluded from the PartialEq
implementation: they are implementation-dependent and not part of API-level
equality comparison. Update the struct-level comment for ExecutionTrace
(referencing fields logs and ipld_ops and the PartialEq impl) to state this
intent so future maintainers understand why eq ignores those fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d5006cd-50f9-4bf8-9882-73c9293697ea
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
docs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsonscripts/tests/api_compare/docker-compose.ymlsrc/lotus_json/message.rssrc/lotus_json/signed_message.rssrc/rpc/methods/common.rssrc/rpc/methods/eth.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.rssrc/state_manager/utils.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
sudo-shashank
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
sudo-shashank
force-pushed
the
shashank/enable-strict-json-api
branch
from
f4f800e to
7f3fcee
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/openrpc-specs/v0.json (1)
10151-10189:⚠️ Potential issue | 🟠 MajorRemove or make
UpgradeXxHeightoptional in the OpenRPC schema.Lotus's
StateGetNetworkParamsreturnsUpgradeFireHorseHeightinForkUpgradeParams, notUpgradeXxHeight. Forest's implementation hardcodesupgrade_xx_heightto999_999_999_999_999, but marking it as required in the OpenRPC schema will cause strict validation failures against Lotus responses, which omit this field entirely. Either removeUpgradeXxHeightfrom the schema or mark it as non-required to maintain compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openrpc-specs/v0.json` around lines 10151 - 10189, The OpenRPC schema currently lists UpgradeXxHeight as a required property in the ForkUpgradeParams object which conflicts with Lotus's StateGetNetworkParams (which returns UpgradeFireHorseHeight and omits UpgradeXxHeight) and forces strict validation failures; update the schema by either removing UpgradeXxHeight from the ForkUpgradeParams required array or moving UpgradeXxHeight out of the "required" list (making it optional) so responses that omit upgrade_xx_height (Forest hardcodes it but Lotus omits it) validate correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/openrpc-specs/v0.json`:
- Around line 10151-10189: The OpenRPC schema currently lists UpgradeXxHeight as
a required property in the ForkUpgradeParams object which conflicts with Lotus's
StateGetNetworkParams (which returns UpgradeFireHorseHeight and omits
UpgradeXxHeight) and forces strict validation failures; update the schema by
either removing UpgradeXxHeight from the ForkUpgradeParams required array or
moving UpgradeXxHeight out of the "required" list (making it optional) so
responses that omit upgrade_xx_height (Forest hardcodes it but Lotus omits it)
validate correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17b75584-731e-4e12-b601-7db4e794ecda
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
docs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsonsrc/lotus_json/message.rssrc/rpc/methods/chain.rssrc/rpc/methods/gas.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/stateful_tests.rssrc/wallet/subcommands/wallet_cmd.rs
✅ Files skipped from review due to trivial changes (1)
- src/tool/subcommands/api_cmd/api_compare_tests.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Why the do the snap files change?
sudo-shashank
force-pushed
the
shashank/enable-strict-json-api
branch
from
dafcc4b to
0869652
Compare
akaladarshi
left a comment
akaladarshi
left a comment
There was a problem hiding this comment.
@sudo-shashank Can you please add the ref link using the latest version of lotus instead of using the master branch, because master is subjected to change.
@sudo-shashank What I mean by latest version was something like this : |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/openrpc-specs/v0.json (1)
10342-10399: 🛠️ Refactor suggestion | 🟠 MajorSplit request and response message schemas.
Adding
CIDto the sharedMessageschema fixes response-side strict JSON, but this same schema is also referenced by request params likeFilecoin.StateCall(Lines 3126-3131),Filecoin.GasEstimateMessageGas(Lines 2244-2248),Filecoin.MpoolPushMessage(Lines 2570-2575), andFilecoin.WalletSignMessage(Lines 5703-5708). That silently documentsCIDas valid input even though this PR only needs it on responses. Please keepMessagefor request bodies and introduce a response-only variant for theCID-carrying shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openrpc-specs/v0.json` around lines 10342 - 10399, The shared "Message" schema currently includes "CID" which incorrectly documents CID as valid for request params; revert the request shape by keeping "Message" without "CID" and introduce a new response-only schema (e.g., "MessageWithCID" or "MessageResponse") that adds the "CID" property. Update all response references that must carry CID to point to the new schema and leave the request references (such as Filecoin.StateCall, Filecoin.GasEstimateMessageGas, Filecoin.MpoolPushMessage, Filecoin.WalletSignMessage) referencing the original "Message" schema so CID is no longer allowed in request bodies. Ensure the new schema reuses the same properties as "Message" plus the "CID" $ref to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/openrpc-specs/v0.json`:
- Around line 10342-10399: The shared "Message" schema currently includes "CID"
which incorrectly documents CID as valid for request params; revert the request
shape by keeping "Message" without "CID" and introduce a new response-only
schema (e.g., "MessageWithCID" or "MessageResponse") that adds the "CID"
property. Update all response references that must carry CID to point to the new
schema and leave the request references (such as Filecoin.StateCall,
Filecoin.GasEstimateMessageGas, Filecoin.MpoolPushMessage,
Filecoin.WalletSignMessage) referencing the original "Message" schema so CID is
no longer allowed in request bodies. Ensure the new schema reuses the same
properties as "Message" plus the "CID" $ref to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68cbbdff-9b1c-4b70-88e7-62aa8ea43a5f
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsonsrc/rpc/methods/common.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rpc/methods/state/types.rs
@akaladarshi latest commit or |
3 participants
Summary of changes
Changes introduced in this pull request:
Set
FOREST_STRICT_JSON=1for all api compare check.Fix the unknown field error found in the below methods:
Updated
Filecoin.StateCall,Filecoin.StateReplayandFilecoin.StateComputetest snapshots becauseExecutionTracenow includesLogsandIpldOps, which earlier snapshots did not capture.Updated
Filecoin.StateGetNetworkParamstest snapshot becauseNetworkParamsnow includesGenesisTimestamp.Reference issue to close (if applicable)
Closes #5635
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Refactor
Tests / Other