Enable strict json check in api compare#6970
Open
sudo-shashank wants to merge 6 commits intomainfrom
Open
Conversation
Contributor
WalkthroughThe changes refactor message handling and execution trace modeling by having RPC endpoints return Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
sudo-shashank
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
| "UpgradeTockHeight", | ||
| "UpgradeGoldenWeekHeight" | ||
| "UpgradeGoldenWeekHeight", | ||
| "UpgradeXxHeight" |
sudo-shashank
force-pushed
the
shashank/enable-strict-json-api
branch
from
f4f800e to
7f3fcee
Compare
Contributor
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Reference issue to close (if applicable)
Closes #5635
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Improvements