test: PR12 unit-test coverage on external account#24
Open
hzj-edu-nju wants to merge 14 commits intobladehan1:developfrom
Open
test: PR12 unit-test coverage on external account#24hzj-edu-nju wants to merge 14 commits intobladehan1:developfrom
hzj-edu-nju wants to merge 14 commits intobladehan1:developfrom
Conversation
…er assignment Fork PRs lack write access under `pull_request` trigger, causing reviewer assignment to fail silently. Switch to `pull_request_target` and explicitly grant `pull-requests: write` permission. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(ci): fix reviewer assignment permissions for fork PRs
📝 WalkthroughWalkthroughThis PR enhances contract event parsing with stricter bounds and UTF-8 validation, centralizes revert-reason decoding in JSON RPC responses, and updates the CI workflow trigger to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
framework/src/main/java/org/tron/common/logsfilter/ContractEventParser.java (1)
80-91: Consider small consistency tweaks insubBytesguards.Two minor points on the strengthened validation:
- The empty-source branch at line 82 produces a message without
start/length, while the second branch includes all three values. Unifying the format makes failures easier to grep in logs.start >= src.lengthat line 84 also rejects a legitimate zero-length slice at end-of-buffer (start == src.length && length == 0). This isn't exercised byparseDataBytestoday (the ternary at line 44 avoids it whenlength == 0), but ifsubBytesis ever reused in a new caller the condition will surprise. Allowinglength == 0atstart == src.lengthwould be strictly more permissive without weakening the guard.♻️ Suggested tweak
protected static byte[] subBytes(byte[] src, int start, int length) { if (ArrayUtils.isEmpty(src)) { - throw new OutputLengthException("source data is empty"); + throw new OutputLengthException( + "source data is empty, start:" + start + ", length:" + length); } - if (start < 0 || start >= src.length || length < 0 || length > src.length - start) { + if (start < 0 || start > src.length || length < 0 || length > src.length - start) { throw new OutputLengthException( "data start:" + start + ", length:" + length + ", src.length:" + src.length); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/common/logsfilter/ContractEventParser.java` around lines 80 - 91, Update subBytes to (1) use a consistent error message format that always includes start, length and src.length when throwing OutputLengthException, and (2) relax the bounds check to permit start == src.length when length == 0 (i.e., allow a zero-length slice at end-of-buffer). Adjust the guard that currently checks "start >= src.length" so it only fails when start > src.length or when start == src.length and length != 0, and ensure the thrown OutputLengthException message includes the three values for both the empty-src and bounds-failure branches; keep parseDataBytes behavior unchanged except for relying on the now-permissive subBytes behavior.framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java (1)
474-502: Helper centralizes revert decoding cleanly; boundary semantics are correct and well-tested.A few observations:
- Early rejection of
null/ length<= 4/ wrong selector is correct and avoids callingHex.toHexStringon short buffers (theoff+lenform would otherwise AIOOBE).- The size guard uses strict
>, so exactlyMAX_REVERT_REASON_PAYLOAD_BYTES(4096) is parsed — this matchestestTryDecodeRevertReasonAtPayloadLimitand is intentional.- The catch-all around
ContractEventParser.parseDataBytescorrectly swallowsNegativeArraySizeException/IndexOutOfBoundsException/ format issues (exercised by the "malformed"/"negative length" tests).Minor nit: you could drop the BouncyCastle dependency here by using
java.util.Arrays.copyOfRange, which would keep the helper self-contained with JDK APIs.♻️ Optional diff
- String reason = ContractEventParser.parseDataBytes( - org.bouncycastle.util.Arrays.copyOfRange(resData, REVERT_REASON_SELECTOR_LENGTH, - resData.length), - "string", 0); + String reason = ContractEventParser.parseDataBytes( + java.util.Arrays.copyOfRange(resData, REVERT_REASON_SELECTOR_LENGTH, resData.length), + "string", 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java` around lines 474 - 502, The helper tryDecodeRevertReason currently uses org.bouncycastle.util.Arrays.copyOfRange to slice the payload before calling ContractEventParser.parseDataBytes; replace that call with java.util.Arrays.copyOfRange so the method removes the BouncyCastle dependency while keeping the same semantics (use REVERT_REASON_SELECTOR_LENGTH as the start index and resData.length as the end), keeping the existing null/length/selector checks and the MAX_REVERT_REASON_PAYLOAD_BYTES guard intact.framework/src/test/java/org/tron/core/jsonrpc/JsonRpcCallAndEstimateGasTest.java (1)
42-53: State restoration in tearDown is correct; a small hardening suggestion.Capturing
originalEstimateEnergyin a field initializer works because JUnit 4 creates a fresh test-class instance per@Test, so there's no cross-test leakage. One minor concern: if a test fails between mutation andtearDownin a way that throws duringmockRpc.close(), thesetEstimateEnergyrestore is still reached only because it follows the null-check block — that's fine as written, just worth being aware of.Mockito.clearAllCaches()is harmless but typically unnecessary with stateless local mocks; consider dropping it to speed up the suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/jsonrpc/JsonRpcCallAndEstimateGasTest.java` around lines 42 - 53, The tearDown currently restores CommonParameter using the field originalEstimateEnergy after attempting mockRpc.close(), but if mockRpc.close() throws the restore may be skipped; update tearDown (method tearDown and field originalEstimateEnergy) to always restore CommonParameter.getInstance().setEstimateEnergy(originalEstimateEnergy) even if mockRpc.close() throws (e.g., use try/finally around mockRpc.close() or null-check then try/catch/finally), and optionally remove the Mockito.clearAllCaches() call if you want to speed the suite since local stateless mocks do not need it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/src/main/java/org/tron/common/logsfilter/ContractEventParser.java`:
- Around line 80-91: Update subBytes to (1) use a consistent error message
format that always includes start, length and src.length when throwing
OutputLengthException, and (2) relax the bounds check to permit start ==
src.length when length == 0 (i.e., allow a zero-length slice at end-of-buffer).
Adjust the guard that currently checks "start >= src.length" so it only fails
when start > src.length or when start == src.length and length != 0, and ensure
the thrown OutputLengthException message includes the three values for both the
empty-src and bounds-failure branches; keep parseDataBytes behavior unchanged
except for relying on the now-permissive subBytes behavior.
In `@framework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.java`:
- Around line 474-502: The helper tryDecodeRevertReason currently uses
org.bouncycastle.util.Arrays.copyOfRange to slice the payload before calling
ContractEventParser.parseDataBytes; replace that call with
java.util.Arrays.copyOfRange so the method removes the BouncyCastle dependency
while keeping the same semantics (use REVERT_REASON_SELECTOR_LENGTH as the start
index and resData.length as the end), keeping the existing null/length/selector
checks and the MAX_REVERT_REASON_PAYLOAD_BYTES guard intact.
In
`@framework/src/test/java/org/tron/core/jsonrpc/JsonRpcCallAndEstimateGasTest.java`:
- Around line 42-53: The tearDown currently restores CommonParameter using the
field originalEstimateEnergy after attempting mockRpc.close(), but if
mockRpc.close() throws the restore may be skipped; update tearDown (method
tearDown and field originalEstimateEnergy) to always restore
CommonParameter.getInstance().setEstimateEnergy(originalEstimateEnergy) even if
mockRpc.close() throws (e.g., use try/finally around mockRpc.close() or
null-check then try/catch/finally), and optionally remove the
Mockito.clearAllCaches() call if you want to speed the suite since local
stateless mocks do not need it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b992f2c-f9b3-4711-9b06-6d9d9d4fee5f
📒 Files selected for processing (6)
.github/workflows/pr-reviewer.ymlframework/src/main/java/org/tron/common/logsfilter/ContractEventParser.javaframework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.javaframework/src/test/java/org/tron/common/logsfilter/EventParserTest.javaframework/src/test/java/org/tron/core/jsonrpc/JsonRpcCallAndEstimateGasTest.javaframework/src/test/java/org/tron/core/services/jsonrpc/TronJsonRpcRevertReasonTest.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/pr-reviewer.yml">
<violation number="1" location=".github/workflows/pr-reviewer.yml:4">
P2: Workflow now runs in privileged `pull_request_target` context with write permissions while using an unpinned action tag (`@v8`), increasing supply-chain risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| on: | ||
| pull_request: | ||
| pull_request_target: |
There was a problem hiding this comment.
P2: Workflow now runs in privileged pull_request_target context with write permissions while using an unpinned action tag (@v8), increasing supply-chain risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/pr-reviewer.yml, line 4:
<comment>Workflow now runs in privileged `pull_request_target` context with write permissions while using an unpinned action tag (`@v8`), increasing supply-chain risk.</comment>
<file context>
@@ -1,15 +1,17 @@
on:
- pull_request:
+ pull_request_target:
branches: [ 'develop', 'release_**' ]
types: [ opened, edited, reopened ]
</file context>
4 tasks
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.
External-account test PR for validating unit-test coverage results on origin.\n\nSource code copied from 0xbigapple#12 and pushed via test account branch .\n\nExpected checks:\n- PR Build runs from develop workflow\n- Coverage Gate evaluates coverage for the PR code changes\n- No workflow override from feature branch
Summary by cubic
Hardened ABI parsing and revert-reason handling, added focused unit tests, and fixed reviewer assignment for forked PRs. Improves error clarity in
eth_call/eth_estimateGasand increases test coverage.Error(string)and skip malformed/oversized data; applied to call and estimateGas paths.pull_request_targetand grantingpull-requests: write.Written for commit fee9f83. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes & Improvements
Chores