Enable strict json check in api compare#6969
Closed
sudo-shashank wants to merge 3 commits intomainfrom
Closed
Conversation
Contributor
|
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 (1)
WalkthroughThis PR extends the RPC API with additional operational tracking and configuration fields. It adds IPLD operation details and execution logs to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Comment |
![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/lotus_json/message.rs (1)
108-120:⚠️ Potential issue | 🟠 MajorDon’t discard
CIDif these responses still need semantic parity checks.
from_lotus_jsonnow drops the parsedCID, soRpcTest::identity/RpcTest::validateonly compare the message body fields forMessageresponses. That meansChainGetMessageandGasEstimateMessageGascan serialize a wrong embeddedCIDand still pass the new strict-json compare as long as the field is syntactically valid. Please keep theCIDin a compare-only type, or add an explicit raw-JSONCIDassertion for these endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lotus_json/message.rs` around lines 108 - 120, The from_lotus_json implementation is dropping the parsed CID (cid: _) which lets responses with incorrect embedded CIDs pass semantic parity checks; update from_lotus_json in message.rs to preserve the parsed CID (e.g., bind cid and store it on the returned compare-type or on Message) instead of discarding it, or alternatively add an explicit raw-JSON CID assertion in the RpcTest identity/validate code paths for the Message responses (notably ChainGetMessage and GasEstimateMessageGas) so the CID is included in comparisons; locate the from_lotus_json function and the LotusJson/Message compare-type and either propagate the cid into that type or add a CID check in RpcTest::identity / RpcTest::validate.
🧹 Nitpick comments (1)
src/rpc/methods/state.rs (1)
3234-3234: Replace the raw upgrade sentinel with a named constant.Line 3284 uses a magic value for
upgrade_xx_height. Please extract it to a named constant with a short rationale to reduce accidental drift.♻️ Suggested refactor
+const UPGRADE_XX_HEIGHT_PLACEHOLDER: ChainEpoch = 999_999_999_999_999; + impl TryFrom<&ChainConfig> for ForkUpgradeParams { @@ - upgrade_xx_height: 999_999_999_999_999, + upgrade_xx_height: UPGRADE_XX_HEIGHT_PLACEHOLDER,Also applies to: 3284-3284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/state.rs` at line 3234, Introduce a named constant (e.g. UPGRADE_XX_HEIGHT: ChainEpoch = <magic_value>) and replace the raw magic literal used for upgrade_xx_height with that constant; update the declaration/assignment involving upgrade_xx_height and any other occurrences to reference UPGRADE_XX_HEIGHT, and add a brief doc comment explaining the rationale (prevents accidental drift and centralizes the sentinel for upgrade XX). Ensure references to the symbol upgrade_xx_height in functions/structs in this module now use UPGRADE_XX_HEIGHT so the sentinel is defined once and clearly documented.
🤖 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/lotus_json/message.rs`:
- Around line 108-120: The from_lotus_json implementation is dropping the parsed
CID (cid: _) which lets responses with incorrect embedded CIDs pass semantic
parity checks; update from_lotus_json in message.rs to preserve the parsed CID
(e.g., bind cid and store it on the returned compare-type or on Message) instead
of discarding it, or alternatively add an explicit raw-JSON CID assertion in the
RpcTest identity/validate code paths for the Message responses (notably
ChainGetMessage and GasEstimateMessageGas) so the CID is included in
comparisons; locate the from_lotus_json function and the LotusJson/Message
compare-type and either propagate the cid into that type or add a CID check in
RpcTest::identity / RpcTest::validate.
---
Nitpick comments:
In `@src/rpc/methods/state.rs`:
- Line 3234: Introduce a named constant (e.g. UPGRADE_XX_HEIGHT: ChainEpoch =
<magic_value>) and replace the raw magic literal used for upgrade_xx_height with
that constant; update the declaration/assignment involving upgrade_xx_height and
any other occurrences to reference UPGRADE_XX_HEIGHT, and add a brief doc
comment explaining the rationale (prevents accidental drift and centralizes the
sentinel for upgrade XX). Ensure references to the symbol upgrade_xx_height in
functions/structs in this module now use UPGRADE_XX_HEIGHT so the sentinel is
defined once and clearly documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5026415e-3cba-4e3d-b814-81fb089f7e5a
⛔ 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 (15)
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/chain.rssrc/rpc/methods/common.rssrc/rpc/methods/eth.rssrc/rpc/methods/gas.rssrc/rpc/methods/state.rssrc/rpc/methods/state/types.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
sudo-shashank
force-pushed
the
shashank/enable-strict-json
branch
from
068e96a to
0b44d82
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/state.rs (1)
3234-3284: Replace the hard-coded upgrade sentinel with a named constant.Line 3284 uses a raw sentinel epoch. A named constant will make intent clearer and reduce drift risk in future edits.
♻️ Suggested small cleanup
+const UPGRADE_XX_SENTINEL_HEIGHT: ChainEpoch = 999_999_999_999_999; ... - upgrade_xx_height: 999_999_999_999_999, + upgrade_xx_height: UPGRADE_XX_SENTINEL_HEIGHT,🤖 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 hard-coded sentinel 999_999_999_999_999 assigned to ForkUpgradeParams::upgrade_xx_height in the TryFrom<&ChainConfig> impl should be replaced with a named constant (e.g., UPGRADE_SENTINEL or MAX_SENTINEL_EPOCH) to clarify intent and avoid future drift; define the constant near the top of the module (or in the crate's constants module) and use that constant in the try_from conversion for upgrade_xx_height so all references are explicit and easy to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rpc/methods/state.rs`:
- Around line 3234-3284: The hard-coded sentinel 999_999_999_999_999 assigned to
ForkUpgradeParams::upgrade_xx_height in the TryFrom<&ChainConfig> impl should be
replaced with a named constant (e.g., UPGRADE_SENTINEL or MAX_SENTINEL_EPOCH) to
clarify intent and avoid future drift; define the constant near the top of the
module (or in the crate's constants module) and use that constant in the
try_from conversion for upgrade_xx_height so all references are explicit and
easy to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eff58c4a-d341-4268-9bb4-ee3402754126
⛔ 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.jsonscripts/tests/api_compare/docker-compose.ymlsrc/rpc/methods/common.rssrc/rpc/methods/eth.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 (4)
- src/rpc/methods/eth.rs
- scripts/tests/api_compare/docker-compose.yml
- src/rpc/methods/common.rs
- src/state_manager/utils.rs
sudo-shashank
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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
1 participant
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:
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
Tests