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 selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplace FlattenedApiMessage with Message across OpenRPC and Rust surfaces; add CID to Message and make Params Base64String default to null. Extend ExecutionTrace with IpldOps and Logs; add NetworkParams.GenesisTimestamp, PublicVersion.Agent, ForkUpgradeParams.UpgradeFireHorseHeight; enable FOREST_STRICT_JSON for api-compare tests. ChangesSchema & Runtime shape → wiring → tests
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 5 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
sudo-shashank
force-pushed
the
shashank/enable-strict-json-api
branch
from
af04f22 to
228cd72
Compare
sudo-shashank
force-pushed
the
shashank/enable-strict-json-api
branch
from
228cd72 to
cce0f0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/v1.json (1)
10758-10779:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark
Paramsas required in the Message schema, or clarify that it is always emitted.The serializer in
src/lotus_json/message.rsalways emits theParamsfield (asnullwhen empty), confirmed by the test snapshot showing"Params": null. The#[serde(default)]attribute enables deserialization whenParamsis missing, but does not suppress serialization. The current OpenRPC schema marksParamsas optional (not in therequiredlist) withdefault: null, which may mislead generated clients into thinking the field can be omitted from responses. Either markParamsas required in the schema to match the serialization contract, or add explicit documentation that this field is always present in responses despite being optional on input.🤖 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/v1.json` around lines 10758 - 10779, The Message schema currently omits "Params" from the required list while the serializer in src/lotus_json/message.rs always emits Params (null when empty); update the OpenRPC spec to reflect runtime behavior by adding "Params" to the Message schema's required array (so "required": ["To","From","Params"]) or alternatively add a clear note to the Message schema description explaining that Params is always serialized (may be null) despite being optional for input; reference the Message schema and the Params property and ensure the change aligns with serde behavior (#[serde(default)]) described in the comment.
🤖 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.
Inline comments:
In `@docs/openrpc-specs/v0.json`:
- Line 435: The spec reuses the generic "Message" schema for both requests and
responses but the response needs an extra CID field, which unintentionally
documents CID as a valid request property; create a separate response schema
(e.g., "MessageResponse" or "MessageWithCID") that extends or copies "Message"
and adds the CID property, update all response $ref occurrences that currently
point to "#/components/schemas/Message" (including places tied to
Filecoin.StateCall and Filecoin.MpoolPushMessage) to reference the new response
schema, and leave input/request references using the original "Message" schema
unchanged.
---
Outside diff comments:
In `@docs/openrpc-specs/v1.json`:
- Around line 10758-10779: The Message schema currently omits "Params" from the
required list while the serializer in src/lotus_json/message.rs always emits
Params (null when empty); update the OpenRPC spec to reflect runtime behavior by
adding "Params" to the Message schema's required array (so "required":
["To","From","Params"]) or alternatively add a clear note to the Message schema
description explaining that Params is always serialized (may be null) despite
being optional for input; reference the Message schema and the Params property
and ensure the change aligns with serde behavior (#[serde(default)]) described
in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e12933e1-96e7-4fb1-baa8-d46cf53baf31
⛔ 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 (6)
docs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsonsrc/rpc/methods/common.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.rssrc/state_manager/utils.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rpc/methods/state.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/openrpc-specs/v1.json (1)
9773-9785: ⚡ Quick winPin the Lotus reference links to a tag or commit.
These new descriptions point at Lotus
master, so the references will drift away from the exact API shape this spec is documenting. Please pin them to the Lotus tag/commit used by api-compare instead. (github.com)Also applies to: 11217-11221, 11795-11797
🤖 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/v1.json` around lines 9773 - 9785, Update the Lotus links in the OpenRPC JSON schema descriptions so they point to the exact tag or commit used by api-compare instead of master: replace the master URLs referenced in the "IpldOps" schema (description pointing to execresult.go) and the "Logs" schema with the pinned tag/commit URL (and do the same for the other occurrences noted in the file). Locate the description strings for the "IpldOps" and "Logs" components in docs/openrpc-specs/v1.json and substitute the GitHub master links with the corresponding repo URL that includes the specific tag or commit SHA used by api-compare; ensure the text and angle-bracket format remain unchanged except for the URL.
🤖 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.
Nitpick comments:
In `@docs/openrpc-specs/v1.json`:
- Around line 9773-9785: Update the Lotus links in the OpenRPC JSON schema
descriptions so they point to the exact tag or commit used by api-compare
instead of master: replace the master URLs referenced in the "IpldOps" schema
(description pointing to execresult.go) and the "Logs" schema with the pinned
tag/commit URL (and do the same for the other occurrences noted in the file).
Locate the description strings for the "IpldOps" and "Logs" components in
docs/openrpc-specs/v1.json and substitute the GitHub master links with the
corresponding repo URL that includes the specific tag or commit SHA used by
api-compare; ensure the text and angle-bracket format remain unchanged except
for the URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ab9053e-be3d-4921-994b-bf25acd3f07f
⛔ 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 (6)
docs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsonsrc/rpc/methods/common.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.rssrc/state_manager/utils.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/openrpc-specs/v0.json (1)
10342-10399:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSplit request and response message schemas.
Messagenow includesCID(Line 10345), but the same schema is used for request params across methods (for example Line 2574, Line 3130). That broadens the request contract even though the immediate need is response compatibility (Line 435, Line 2283).💡 Minimal direction
"Message": { "type": "object", "properties": { - "CID": { "$ref": "#/components/schemas/Nullable_Cid" }, ... } }, +"MessageWithCid": { + "allOf": [ + { "$ref": "#/components/schemas/Message" }, + { + "type": "object", + "properties": { + "CID": { "$ref": "#/components/schemas/Nullable_Cid" } + } + } + ] +}Then use
MessageWithCidonly in response schemas that need it.Also applies to: 435-435, 2283-2283
🤖 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 current "Message" schema includes "CID", widening request parameter contracts; create a new schema "MessageWithCid" that duplicates the existing "Message" properties but includes the "CID" property (and same required fields), then update only response schemas that need the CID to reference "#/components/schemas/MessageWithCid" while leaving all request param references pointing to "#/components/schemas/Message" (e.g., replace usages used in method responses such as the ones flagged) so request contracts remain unchanged.
🤖 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 current "Message" schema includes "CID", widening
request parameter contracts; create a new schema "MessageWithCid" that
duplicates the existing "Message" properties but includes the "CID" property
(and same required fields), then update only response schemas that need the CID
to reference "#/components/schemas/MessageWithCid" while leaving all request
param references pointing to "#/components/schemas/Message" (e.g., replace
usages used in method responses such as the ones flagged) so request contracts
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2adc734-5525-4de2-b85d-54ee5f380532
⛔ 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 (7)
docs/openrpc-specs/v0.jsondocs/openrpc-specs/v1.jsonsrc/rpc/methods/common.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.rssrc/state_manager/utils.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/methods/state/types.rs
- src/tool/subcommands/api_cmd/test_snapshots.txt
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