Enable strict json check in api compare#6949
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 (2)
WalkthroughThis PR implements strict JSON deserialization for RPC responses to detect unknown fields, adds CID serialization to message types, simplifies RPC response structures by removing wrapper types, extends execution traces and network parameters with new fields, and updates test infrastructure to validate against unknown fields during API parity testing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/rpc/methods/state.rs (1)
3283-3285: Clarify the placeholder value and naming.The hardcoded value
999_999_999_999_999serves as a far-future placeholder, which is reasonable for an unscheduled upgrade. However:
- The field name
upgrade_xx_heightis non-descriptive - consider using the actual upgrade name (e.g.,upgrade_firehorse_heightas suggested by the comment) for clarity.- Consider adding a brief comment explaining why this placeholder exists, such as
// Placeholder for FireHorse upgrade - not yet scheduled.This helps maintainability when the actual upgrade height becomes known.
🤖 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 3283 - 3285, Rename the non-descriptive field upgrade_xx_height to upgrade_firehorse_height (matching the commented get_height(FireHorse)? hint) and replace the magic literal 999_999_999_999_999 with a named constant or at least retain the literal but add a clear inline comment like "// Placeholder for FireHorse upgrade - not yet scheduled" next to upgrade_firehorse_height; update any references to upgrade_xx_height elsewhere (structs, serializers, tests) to the new name so the code remains consistent and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 34: Update the CHANGELOG.md entry that currently references PR `#6926` to
instead reference issue `#5635`: replace the PR link and label
"[`#6926`](https://github.com/ChainSafe/forest/pull/6926)" with the issue-style
link "[`#5635`](https://github.com/ChainSafe/forest/issues/5635)" in the line
describing strict JSON validation so the entry follows the repo convention of
referencing the issue rather than the PR.
In `@src/rpc/methods/state/types.rs`:
- Around line 152-156: Add a new TraceIpld struct and use it for the ipld_ops
field: define a pub struct TraceIpld with serde-backed fields matching Lotus
(pub op: String for "Get"/"Put"/"Unknown", pub cid: Option<String>, pub size:
Option<u64>) and derive Serialize/Deserialize/Debug/Clone; then change the
existing ipld_ops field from Vec<serde_json::Value> to Vec<TraceIpld> while
keeping #[serde(default, skip_serializing_if = "Vec::is_empty")] on the ipld_ops
declaration in the same type (the ExecutionTrace/struct containing ipld_ops) so
deserialization is type-safe and compatible when FVM trace IpldOps are present.
---
Nitpick comments:
In `@src/rpc/methods/state.rs`:
- Around line 3283-3285: Rename the non-descriptive field upgrade_xx_height to
upgrade_firehorse_height (matching the commented get_height(FireHorse)? hint)
and replace the magic literal 999_999_999_999_999 with a named constant or at
least retain the literal but add a clear inline comment like "// Placeholder for
FireHorse upgrade - not yet scheduled" next to upgrade_firehorse_height; update
any references to upgrade_xx_height elsewhere (structs, serializers, tests) to
the new name so the code remains consistent and maintainable.
🪄 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: 1dda2f00-e919-46da-b950-f2a7cd41b259
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locksrc/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 (19)
CHANGELOG.mdCargo.tomldocs/docs/users/reference/env_variables.mdscripts/tests/api_compare/docker-compose.ymlsrc/lotus_json/message.rssrc/lotus_json/signed_message.rssrc/rpc/json_validator.rssrc/rpc/methods/chain.rssrc/rpc/methods/common.rssrc/rpc/methods/eth.rssrc/rpc/methods/gas.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.rssrc/rpc/reflect/mod.rssrc/rpc/reflect/parser.rssrc/state_manager/utils.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/stateful_tests.rssrc/wallet/subcommands/wallet_cmd.rs
f76d689 to
f6ceac2
Compare
Summary of changes
Changes introduced in this pull request:
FOREST_STRICT_JSON=1for all api compare check and fix the unknown field error found in some RPC response.Reference issue to close (if applicable)
Closes #5635
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Chores