fix(api): deprecate events field in shielded TRC20 scan APIs#14
fix(api): deprecate events field in shielded TRC20 scan APIs#14Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughDeprecates the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RpcApiService
participant Wallet
participant Database
Client->>RpcApiService: scanShieldedTRC20NotesByIvk(params, events?)
alt events present and non-empty
RpcApiService-->>Client: INVALID_ARGUMENT (rejectIfEventsPresent)
else events empty or missing
RpcApiService->>Wallet: scanShieldedTRC20NotesByIvk(params)
Wallet->>Database: fetchLogs(startBlock, endBlock, contract)
Database-->>Wallet: logs
Wallet->>Wallet: getShieldedTRC20LogType(log.getTopicsList(), contract)
Wallet-->>RpcApiService: decrypted notes/results
RpcApiService-->>Client: stream results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)framework/src/main/java/org/tron/core/Wallet.java[] 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 |
adee49c to
3335e0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
framework/src/main/java/org/tron/core/Wallet.java (1)
3947-3955: Keep the IVKindexcounter aligned with note-bearing logs.
getNoteTxFromLogListByIvk()only produces aNoteTxfor log types 1..3, but this loop now incrementsindexfor anylogType > 0, includingTokenBurn(logType == 4). After removing client-side topic filtering, a burn transaction that emits both leaf and token-burn logs can shift or gap the returnedindex, which no longer matches the proto’s “index of note in txid” contract. Please verify this with a burn + change-note case; ifindexis supposed to stay note-based, only advance it for the IVK-emitting log types.Possible fix
- if (logType > 0) { + if (logType > 0 && logType < 4) { noteBuilder = DecryptNotesTRC20.NoteTx.newBuilder(); noteBuilder.setTxid(ByteString.copyFrom(txId)); noteBuilder.setIndex(index); index += 1; noteTx = getNoteTxFromLogListByIvk(noteBuilder, log, ivk, ak, nk, shieldedTRC20ContractAddress, logType); noteTx.ifPresent(builder::addNoteTxs); }🤖 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/Wallet.java` around lines 3947 - 3955, The loop increments the IVK note `index` for any logType > 0 (including TokenBurn == 4) causing gaps when no NoteTx is produced; only advance the note index for IVK-emitting note types. Change the logic around getShieldedTRC20LogType/getNoteTxFromLogListByIvk so that `index += 1` happens only for the note-bearing log types (e.g., logType 1..3) or after confirming noteTx.isPresent(), leaving `index` unchanged for TokenBurn (logType == 4) and other non-note logs.
🤖 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/http/ScanShieldedTRC20NotesByIvkServlet.java`:
- Around line 47-50: The helper rejectIfEventsPresent currently takes a single
String and only checks the first events parameter; change its signature to
rejectIfEventsPresent(String[] eventsParams) and update the implementation to
iterate over eventsParams (or handle null) and throw new
IllegalArgumentException(EVENTS_DEPRECATED_MSG) if any element is non-null and
not empty; update callers to pass request.getParameterValues("events") instead
of getParameter("events") and apply the identical signature/usage change in
ScanShieldedTRC20NotesByOvkServlet so both classes use the new
rejectIfEventsPresent(String[]) helper.
In
`@framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.java`:
- Around line 26-27: The wrapped member access in
ScanShieldedTRC20NotesByOvkServlet violates the SeparatorWrapDot rule: move the
dot to the continuation line so chained calls place the '.' at the start of the
next line; specifically change the call like
ovkDecryptTRC20Parameters.getEventsList() split across lines so the '.' begins
the continuation (referencing ovkDecryptTRC20Parameters and
rejectIfEventsPresent), and apply the same fix to the other similar wrapped
member accesses in this file.
In `@framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java`:
- Around line 561-607: The tests only assert the error message but not the gRPC
status code; update both
testScanShieldedTRC20NotesByIvkRejectsDeprecatedEvents() and
testScanShieldedTRC20NotesByOvkRejectsDeprecatedEvents() to also assert that
each caught StatusRuntimeException (fullException, solidityException,
pbftException) has status code Status.INVALID_ARGUMENT (e.g. use
Assert.assertEquals(io.grpc.Status.INVALID_ARGUMENT.getCode(),
fullException.getStatus().getCode()) and the same for solidityException and
pbftException) for the calls on blockingStubFull, blockingStubSolidity and
blockingStubPBFT so the API contract is enforced.
---
Nitpick comments:
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Around line 3947-3955: The loop increments the IVK note `index` for any
logType > 0 (including TokenBurn == 4) causing gaps when no NoteTx is produced;
only advance the note index for IVK-emitting note types. Change the logic around
getShieldedTRC20LogType/getNoteTxFromLogListByIvk so that `index += 1` happens
only for the note-bearing log types (e.g., logType 1..3) or after confirming
noteTx.isPresent(), leaving `index` unchanged for TokenBurn (logType == 4) and
other non-note logs.
🪄 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: 4c8fd412-3390-4bda-970b-a10dc74f4c0e
📒 Files selected for processing (8)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.javaframework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.javaframework/src/test/java/org/tron/core/WalletMockTest.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.javaprotocol/src/main/protos/api/api.proto
| static void rejectIfEventsPresent(String eventsParam) { | ||
| if (eventsParam != null && !eventsParam.isEmpty()) { | ||
| throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java | sed -n '40,90p'Repository: Federico2014/java-tron
Length of output: 2632
🏁 Script executed:
cat -n framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.java | sed -n '40,90p'Repository: Federico2014/java-tron
Length of output: 1514
Reject all events query values, not just the first one.
getParameter("events") only checks the first value in the query string, so a request like ?events=&events=mint bypasses the new 400 error path even though the deprecated field is populated. Change the method signature to accept String[] eventsParams and use getParameterValues("events") to iterate all values; apply the same fix to the ScanShieldedTRC20NotesByOvkServlet which reuses this helper.
Suggested fix
- static void rejectIfEventsPresent(String eventsParam) {
- if (eventsParam != null && !eventsParam.isEmpty()) {
- throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG);
- }
- }
+ static void rejectIfEventsPresent(String[] eventsParams) {
+ if (eventsParams == null) {
+ return;
+ }
+ for (String eventsParam : eventsParams) {
+ if (eventsParam != null && !eventsParam.isEmpty()) {
+ throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG);
+ }
+ }
+ }
@@
- rejectIfEventsPresent(request.getParameter("events"));
+ rejectIfEventsPresent(request.getParameterValues("events"));Apply the same change in ScanShieldedTRC20NotesByOvkServlet line 46–47.
🤖 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/http/ScanShieldedTRC20NotesByIvkServlet.java`
around lines 47 - 50, The helper rejectIfEventsPresent currently takes a single
String and only checks the first events parameter; change its signature to
rejectIfEventsPresent(String[] eventsParams) and update the implementation to
iterate over eventsParams (or handle null) and throw new
IllegalArgumentException(EVENTS_DEPRECATED_MSG) if any element is non-null and
not empty; update callers to pass request.getParameterValues("events") instead
of getParameter("events") and apply the identical signature/usage change in
ScanShieldedTRC20NotesByOvkServlet so both classes use the new
rejectIfEventsPresent(String[]) helper.
53beedf to
63fe9fd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/src/main/java/org/tron/core/services/RpcApiService.java (1)
295-304: Helper is clean; consider logging rejected requests for observability.
rejectIfEventsPresentcorrectly rejects withINVALID_ARGUMENTwhen the deprecatedeventsfield is populated, andProtocolStringList.getEventsList()from protobuf is guaranteed non-null so theisEmpty()call is safe.Minor: since this is a breaking change for existing clients, consider a
logger.infowhen rejecting so operators can detect and help migrate clients still sendingevents. Not blocking.🤖 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/RpcApiService.java` around lines 295 - 304, The helper rejectIfEventsPresent should also log when it rejects a request so operators can observe clients still sending the deprecated field; update the method (rejectIfEventsPresent) to call the class logger (e.g., logger.info or logger.warn) immediately before responseObserver.onError, include a brief message that the 'events' field was received and rejected plus minimal context (e.g., size of ProtocolStringList or eventsList.toString()) to avoid verbose payloads; keep the existing INVALID_ARGUMENT response behavior unchanged.framework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.java (1)
21-22: Nit: duplicated error message literal.
EVENTS_DEPRECATED_MSGis copy-pasted fromScanShieldedTRC20NotesByIvkServlet. If the production message is ever updated, this test will silently drift. Consider promoting the servlet constant to package-private (or moving it to a small shared constants class) so the test can reference the same source of truth.🤖 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/services/http/ScanShieldedTRC20NotesServletTest.java` around lines 21 - 22, The test defines a duplicate literal EVENTS_DEPRECATED_MSG that duplicates the message in ScanShieldedTRC20NotesByIvkServlet; change the servlet to expose that constant (make the servlet's constant package-private or move it into a shared constants class) and update ScanShieldedTRC20NotesServletTest to reference the servlet/shared constant instead of its own copy (look for EVENTS_DEPRECATED_MSG in the test and the constant in ScanShieldedTRC20NotesByIvkServlet).
🤖 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/core/services/RpcApiService.java`:
- Around line 295-304: The helper rejectIfEventsPresent should also log when it
rejects a request so operators can observe clients still sending the deprecated
field; update the method (rejectIfEventsPresent) to call the class logger (e.g.,
logger.info or logger.warn) immediately before responseObserver.onError, include
a brief message that the 'events' field was received and rejected plus minimal
context (e.g., size of ProtocolStringList or eventsList.toString()) to avoid
verbose payloads; keep the existing INVALID_ARGUMENT response behavior
unchanged.
In
`@framework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.java`:
- Around line 21-22: The test defines a duplicate literal EVENTS_DEPRECATED_MSG
that duplicates the message in ScanShieldedTRC20NotesByIvkServlet; change the
servlet to expose that constant (make the servlet's constant package-private or
move it into a shared constants class) and update
ScanShieldedTRC20NotesServletTest to reference the servlet/shared constant
instead of its own copy (look for EVENTS_DEPRECATED_MSG in the test and the
constant in ScanShieldedTRC20NotesByIvkServlet).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82b3425f-c58f-4a9f-8ba1-e9293e325f65
📒 Files selected for processing (9)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.javaframework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.javaframework/src/test/java/org/tron/core/WalletMockTest.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.javaframework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.javaprotocol/src/main/protos/api/api.proto
✅ Files skipped from review due to trivial changes (1)
- framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/src/main/protos/api/api.proto
- framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.java
- framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
87f9d57 to
0fce723
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/src/main/java/org/tron/core/services/RpcApiService.java (1)
296-306: Optional: delegate toUtil.rejectIfEventsPresentto keep one source of truth.This helper re-implements the same predicate already defined in
Util.rejectIfEventsPresent(ProtocolStringList). If the rejection rule ever changes (e.g. also reject when list is simply non-empty), you'd need to update both places. Consider calling the Util helper and translatingIllegalArgumentExceptionintoStatus.INVALID_ARGUMENT:♻️ Suggested refactor
private static boolean rejectIfEventsPresent( StreamObserver<?> responseObserver, ProtocolStringList events) { - if (events.stream().anyMatch(s -> !s.isEmpty())) { - logger.warn(Util.EVENTS_DEPRECATED_MSG); + try { + Util.rejectIfEventsPresent(events); + } catch (IllegalArgumentException e) { responseObserver.onError(Status.INVALID_ARGUMENT - .withDescription(Util.EVENTS_DEPRECATED_MSG) + .withDescription(e.getMessage()) .asRuntimeException()); return true; } return false; }🤖 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/RpcApiService.java` around lines 296 - 306, The local rejectIfEventsPresent method duplicates Util.rejectIfEventsPresent(ProtocolStringList); change it to call Util.rejectIfEventsPresent(events) and translate any thrown IllegalArgumentException into the existing gRPC error response: catch IllegalArgumentException, log/warn with Util.EVENTS_DEPRECATED_MSG, call responseObserver.onError(Status.INVALID_ARGUMENT.withDescription(Util.EVENTS_DEPRECATED_MSG).asRuntimeException()), and return true; otherwise return false when Util does not throw. Keep the method signature and use responseObserver and Status.INVALID_ARGUMENT as before.framework/src/main/java/org/tron/core/services/http/Util.java (1)
87-103: Minor: consolidate the tworejectIfEventsPresentoverloads.The two overloads duplicate the same "reject if any element is non-empty" rule but are subtly inconsistent — the
String[]variant null-guards its argument while theProtocolStringListvariant does not. Protobuf-generatedgetEventsList()never returns null so this isn't a bug today, but reducing duplication and making the behavior uniform keeps future callers safe.♻️ Suggested consolidation
public static void rejectIfEventsPresent(ProtocolStringList events) { - if (events.stream().anyMatch(s -> !s.isEmpty())) { - logger.warn(EVENTS_DEPRECATED_MSG); - throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG); - } + if (events != null && events.stream().anyMatch(s -> s != null && !s.isEmpty())) { + logger.warn(EVENTS_DEPRECATED_MSG); + throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG); + } } public static void rejectIfEventsPresent(String[] eventsParams) { - if (eventsParams != null) { - for (String v : eventsParams) { - if (v != null && !v.isEmpty()) { - logger.warn(EVENTS_DEPRECATED_MSG); - throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG); - } - } - } + if (eventsParams == null) { + return; + } + rejectIfEventsPresent( + com.google.protobuf.LazyStringArrayList.EMPTY); // or inline the same predicate }A simpler alternative: factor out a private
private static void rejectIfAnyNonEmpty(Stream<String> s)and have both overloads delegate to it.🤖 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/http/Util.java` around lines 87 - 103, Consolidate the duplicate logic in the two rejectIfEventsPresent overloads by extracting a private helper method (e.g., private static void rejectIfAnyNonEmpty(Stream<String> s)) that performs the non-empty check and logging/exception throw; update public static void rejectIfEventsPresent(ProtocolStringList events) and public static void rejectIfEventsPresent(String[] eventsParams) to null-guard their inputs as needed, convert them to a Stream<String> (e.g., events.stream() and Arrays.stream(eventsParams)) and delegate to rejectIfAnyNonEmpty so behavior is uniform and duplication is removed.
🤖 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/core/services/http/Util.java`:
- Around line 87-103: Consolidate the duplicate logic in the two
rejectIfEventsPresent overloads by extracting a private helper method (e.g.,
private static void rejectIfAnyNonEmpty(Stream<String> s)) that performs the
non-empty check and logging/exception throw; update public static void
rejectIfEventsPresent(ProtocolStringList events) and public static void
rejectIfEventsPresent(String[] eventsParams) to null-guard their inputs as
needed, convert them to a Stream<String> (e.g., events.stream() and
Arrays.stream(eventsParams)) and delegate to rejectIfAnyNonEmpty so behavior is
uniform and duplication is removed.
In `@framework/src/main/java/org/tron/core/services/RpcApiService.java`:
- Around line 296-306: The local rejectIfEventsPresent method duplicates
Util.rejectIfEventsPresent(ProtocolStringList); change it to call
Util.rejectIfEventsPresent(events) and translate any thrown
IllegalArgumentException into the existing gRPC error response: catch
IllegalArgumentException, log/warn with Util.EVENTS_DEPRECATED_MSG, call
responseObserver.onError(Status.INVALID_ARGUMENT.withDescription(Util.EVENTS_DEPRECATED_MSG).asRuntimeException()),
and return true; otherwise return false when Util does not throw. Keep the
method signature and use responseObserver and Status.INVALID_ARGUMENT as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d2c71b7-1992-41d1-ac24-21ba7fa7351c
📒 Files selected for processing (10)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.javaframework/src/main/java/org/tron/core/services/http/Util.javaframework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.javaframework/src/test/java/org/tron/core/WalletMockTest.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.javaframework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.javaprotocol/src/main/protos/api/api.proto
✅ Files skipped from review due to trivial changes (2)
- protocol/src/main/protos/api/api.proto
- framework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java
- framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java
- framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.java
- framework/src/test/java/org/tron/core/WalletMockTest.java
- framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
- framework/src/main/java/org/tron/core/Wallet.java
What does this PR do?
Deprecates the
eventsfield in shielded TRC20 scan API requests and removes server-side log-type filtering.eventsfield as[deprecated = true]inIvkDecryptTRC20ParametersandOvkDecryptTRC20Parametersrequest messages inapi.protoINVALID_ARGUMENT, HTTP returns 400 when the deprecatedeventsfield is populatedtopicsListparameter from internal method signatures inWallet,RpcApiService, and HTTP servlet handlers (ScanShieldedTRC20NotesByIvkServlet,ScanShieldedTRC20NotesByOvkServlet)Why are these changes required?
The
eventsfield allowed callers to pass custom log topic strings that were hashed and matched server-side viaWallet.getShieldedTRC20LogType(). This mechanism has anArrayIndexOutOfBoundsExceptionbug:Array out-of-bounds on log data parsing: Downstream methods
getNoteTxFromLogListByIvkandgetNoteTxFromLogListByOvkparselogDataat fixed offsets based on the returnedlogType(e.g., logType 1–3 expect ≥ 788 bytes, logType 4 expects ≥ 144 bytes). If a user-craftedtopicsListcauses a non-shielded transaction log to be misidentified as a shielded log, itslogDatawill be too short andByteArray.subArray()throwsArrayIndexOutOfBoundsException.Unreliable string matching: The logType is determined by
topic.toLowerCase().contains("mint")etc., which is too broad — a user can construct arbitrary strings containing these substrings to control the logType return value.Redundant functionality: The predefined constant matching path (path 1, when topicsList is empty) already covers all 4 valid log types (Mint/Transfer/Burn_Leaf/Burn_Token). The custom topicsList path adds no business value.
scanShieldedTRC20NotesByIvk/Ovk— theeventsrequest field is now rejected outright (INVALID_ARGUMENTon gRPC, HTTP 400). Clients still sending the field must remove it. Clients that relied on the field to limit scan scope will now receive results for all log types.This PR has been tested by:
RpcApiServicesTest,WalletMockTest,ShieldedTRC20BuilderTest./gradlew clean build)Follow up
No consensus logic or on-chain state transitions affected.
Extra details
Split from #7
Closes #16
Summary by CodeRabbit
New Features
Updates
Tests