refactor(jsonrpc): centralize block selector parsing into JsonRpcApiUtil#11
refactor(jsonrpc): centralize block selector parsing into JsonRpcApiUtil#110xbigapple wants to merge 2 commits intodevelopfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors JSON-RPC block identifier parsing by centralizing validation and parsing logic in Changes
Sequence Diagram(s)sequenceDiagram
participant RPC as RPC Method
participant Util as JsonRpcApiUtil
participant Wallet as Wallet
RPC->>Util: parseBlockNumber(blockIdentifier)
alt Is Block Tag
Util->>Util: isBlockTag(identifier)?
Util->>Util: parseBlockTag(tag, wallet)
alt Valid Tag (latest/earliest/finalized)
Util->>Wallet: getHeadBlockNum() or getNowBlock()
Wallet-->>Util: blockNumber
else Invalid Tag (pending/safe)
Util-->>RPC: JsonRpcInvalidParamsException
end
Util-->>RPC: blockNumber
else Hex Number
Util->>Util: ByteArray.jsonHexToLong(identifier)
Util-->>RPC: blockNumber
else Invalid
Util-->>RPC: JsonRpcInvalidParamsException
end
RPC->>Wallet: getBlockByNum(blockNumber)
Wallet-->>RPC: Block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review this pr |
|
Tip For best results, initiate chat on the files or code changes.
Just let me know what you need! |
|
✅ Actions performedFull review triggered. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Centralizes JSON-RPC block selector (tag/number) parsing into JsonRpcApiUtil to reduce duplication and remove Wallet’s reverse dependency on the JSON-RPC layer, while simplifying LogFilterWrapper’s block-range normalization.
Changes:
- Moved block tag constants and tag/number parsing into
JsonRpcApiUtil(isBlockTag,parseBlockTag,parseBlockNumber). - Refactored
LogFilterWrapperto use explicit “strategy” handling for from/to block semantics instead of-1sentinel logic. - Updated
TronJsonRpcImpland tests to use the new centralized parsing and constants; removed JSON-RPC-specific block selector helpers fromWallet.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java | Adds canonical block tag constants and parsing helpers to centralize block selector resolution. |
| framework/src/main/java/org/tron/core/services/jsonrpc/filters/LogFilterWrapper.java | Rewrites from/to block normalization into explicit strategies, using centralized parsing helpers. |
| framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java | Switches endpoints from Wallet block-selector helpers to centralized parsing and adds requireLatestBlockTag() helper. |
| framework/src/main/java/org/tron/core/Wallet.java | Removes JSON-RPC-specific block selector APIs and adds getHeadBlockNum() accessor. |
| framework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.java | Updates block selector/tag parsing tests to exercise JsonRpcApiUtil parsing directly. |
| framework/src/test/java/org/tron/core/services/jsonrpc/LogFilterWrapperStrategyTest.java | Adds unit tests validating LogFilterWrapper strategy behavior against develop-branch semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f6f59aafc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java`:
- Around line 557-563: parseBlockNumber currently lets ByteArray.jsonHexToLong
throw raw parsing exceptions; change it so any failure to parse a non-tag block
string is normalized to JsonRpcInvalidParamsException with the BLOCK_NUM_ERROR
code. Keep the existing isBlockTag -> parseBlockTag path unchanged, but wrap the
ByteArray.jsonHexToLong(blockNumOrTag) call in a try/catch that catches
parsing/number format exceptions and rethrows new
JsonRpcInvalidParamsException(BLOCK_NUM_ERROR) (or the project’s equivalent
constant) so no raw NumberFormatException or "Incorrect hex syntax" leaks out.
- Around line 531-535: JsonRpcApiUtil currently dereferences
wallet.getNowBlock() for the LATEST_STR branch, which can be null; update the
LATEST_STR handling in JsonRpcApiUtil to null-check wallet.getNowBlock() (and
its getBlockHeader()/getRawData() chain) and, if null, return/throw a controlled
JSON-RPC error response instead of letting an NPE bubble up; specifically modify
the branch that uses LATEST_STR and wallet.getNowBlock() to detect an
empty/unready block store and return a meaningful JSON-RPC error (using the
existing rpc error/exception type your codebase uses) so callers receive a
proper error rather than a null-pointer.
In `@framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java`:
- Around line 308-320: Replace direct ByteArray.hexToBigInteger(...).longValue()
parsing in the four branches with the centralized parser: call
JsonRpcApiUtil.parseBlockNumber(blockNumOrTag, wallet) instead of
ByteArray.hexToBigInteger usage so oversized or malformed selectors are rejected
consistently; locate the occurrences in TronJsonRpcImpl where blockNumOrTag is
handled (currently using JsonRpcApiUtil.isBlockTag/parseBlockTag in some
branches and ByteArray.hexToBigInteger in others) and swap those ByteArray-based
parses to JsonRpcApiUtil.parseBlockNumber, keeping the same exception behavior
(throw JsonRpcInvalidParamsException(BLOCK_NUM_ERROR) on parse failure).
In `@framework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.java`:
- Around line 462-466: Add a regression test that simulates a skewed header by
advancing the wallet's in-memory head (use wallet.getNowBlock() or the
setter/mocking facility around that) to a number greater than the persisted
latest block (LATEST_BLOCK_NUM) and then call parseBlockTag("latest", wallet);
assert that parseBlockTag still returns the persisted head block number
(LATEST_BLOCK_NUM). Locate the existing test block around parseBlockTag and
insert the skew step before calling parseBlockTag so the test verifies
resolution uses the persisted head despite a larger in-memory/header number.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27296e3e-c2e2-4d9c-904f-edb1620f3af5
📒 Files selected for processing (6)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.javaframework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.javaframework/src/main/java/org/tron/core/services/jsonrpc/filters/LogFilterWrapper.javaframework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.javaframework/src/test/java/org/tron/core/services/jsonrpc/LogFilterWrapperStrategyTest.java
framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java
Show resolved
Hide resolved
framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What does this PR do?
Centralize block tag and number resolution into
JsonRpcApiUtilto eliminate code duplication, removeWallet's reverse dependency on the JSON-RPC layer, and simplify complex logic inLogFilterWrapper.Why are these changes required?
JSON-RPC block selector parsing (tag-to-block-number resolution) is scattered across three layers (
Wallet,JsonRpcApiUtil,TronJsonRpcImpl) with duplicated, inconsistent logic. The coreWalletclass reverse-depends on the jsonrpc API layer for tag constants, and a-1sentinel value for "latest" forces every caller to re-interpret results differently. This makes the code fragile and hard to reason about.This PR has been tested by:
Follow up
Extra details
Summary by cubic
Centralized JSON‑RPC block selector parsing into
JsonRpcApiUtil, unified tag handling, and simplified log filter ranges. Resolves “latest” viawallet.getNowBlock()to avoid race conditions.JsonRpcApiUtil.isBlockTag,parseBlockTag, andparseBlockNumber; moved tag constants and errors (includingsafe) intoJsonRpcApiUtil.TronJsonRpcImplnow delegates all block parsing;eth_getBlock*use a shared resolver; state reads (getTrxBalance,getStorageAt,getABIOfSmartContract,eth_call) only accept "latest".Wallet’s JSON‑RPC helpers and the "-1 means latest" sentinel; addedgetHeadBlockNum().LogFilterWrapper: fromBlock="latest" is a snapshot; toBlock="latest" is open-ended (Long.MAX_VALUE); consistent range validation.safe), and addedLogFilterWrapperStrategyTest.Written for commit 0b375d5. Summary will update on new commits.
Summary by CodeRabbit