refactor(jsonrpc): centralize block selector parsing into JsonRpcApiUtil#11
refactor(jsonrpc): centralize block selector parsing into JsonRpcApiUtil#110xbigapple wants to merge 8 commits into
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.
![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".
![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
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.
…ution, also use longValueExact in eth_getProof block number parsing and clarify the getBlockByNumOrTag comment
…use ASCII arrows in comments, also add safe tag coverage in state-read endpoint tests
…StrategyTest comments, also collapse double blank line in LogFilterWrapper and drop unused local in testStrategy4_LatestGreaterThanSmallBlock_Throws
0xbigapple
force-pushed
the
refactor/jsonrpc-block-selector
branch
from
8e9f5c8 to
5a38512
Compare
# Conflicts: # framework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.java
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
Centralizes JSON‑RPC block selector parsing in
JsonRpcApiUtil, adds strict tag/hex handling (incl. unsupportedsafe), and aligns “latest” behavior across block and state reads. Simplifies log filter ranges and removesWalletJSON‑RPC helpers.JsonRpcApiUtiltag constants/errors and helpers:isBlockTag,parseBlockTag,parseBlockNumber(strict0xhex; overflow rejected vialongValueExact). “latest” resolves to head viawallet.getHeadBlockNum(), while actual block reads usewallet.getNowBlock().TronJsonRpcImpl#getBlockByNumOrTag; updatedeth_getBlockTransactionCountByNumber,eth_getBlockByNumber,getTransactionByBlockNumberAndIndex, andgetBlockReceiptsto use it; unified errors, includingsafe(“TAG [earliest | pending | finalized | safe] not supported”).requireLatestBlockTagfor state reads (getTrxBalance,getStorageAt,getABIOfSmartContract,eth_call): reject numeric quantities (“QUANTITY not supported…”) and malformed hex (“invalid block number”); only “latest” accepted.WalletJSON‑RPC helpers and the “-1 means latest” sentinel; addedgetHeadBlockNum().LogFilterWrapper: four strategies;fromBlock="latest"uses a snapshot (current head);toBlock="latest"is open‑ended (tracks future blocks); consistent range validation.eth_callobject form),safetag rejections, and long overflow cases; addedLogFilterWrapperStrategyTest.Written for commit 0004cea. Summary will update on new commits. Review in cubic
Summary by CodeRabbit