Skip to content

fix(api): deprecate events field in shielded TRC20 scan APIs#14

Closed
Federico2014 wants to merge 1 commit intodevelopfrom
fix/deprecate-shielded-trc20-events
Closed

fix(api): deprecate events field in shielded TRC20 scan APIs#14
Federico2014 wants to merge 1 commit intodevelopfrom
fix/deprecate-shielded-trc20-events

Conversation

@Federico2014
Copy link
Copy Markdown
Owner

@Federico2014 Federico2014 commented Apr 15, 2026

What does this PR do?

Deprecates the events field in shielded TRC20 scan API requests and removes server-side log-type filtering.

  1. Marked the events field as [deprecated = true] in IvkDecryptTRC20Parameters and OvkDecryptTRC20Parameters request messages in api.proto
  2. Added runtime rejection: gRPC returns INVALID_ARGUMENT, HTTP returns 400 when the deprecated events field is populated
  3. Removed topicsList parameter from internal method signatures in Wallet, RpcApiService, and HTTP servlet handlers (ScanShieldedTRC20NotesByIvkServlet, ScanShieldedTRC20NotesByOvkServlet)
  4. All log types (Mint/Transfer/Burn) are now always inspected during a scan
  5. Added gRPC test coverage for the deprecation-rejection path across Full, Solidity, and PBFT stubs

Why are these changes required?

The events field allowed callers to pass custom log topic strings that were hashed and matched server-side via Wallet.getShieldedTRC20LogType(). This mechanism has an ArrayIndexOutOfBoundsException bug:

  1. Array out-of-bounds on log data parsing: Downstream methods getNoteTxFromLogListByIvk and getNoteTxFromLogListByOvk parse logData at fixed offsets based on the returned logType (e.g., logType 1–3 expect ≥ 788 bytes, logType 4 expects ≥ 144 bytes). If a user-crafted topicsList causes a non-shielded transaction log to be misidentified as a shielded log, its logData will be too short and ByteArray.subArray() throws ArrayIndexOutOfBoundsException.

  2. 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.

  3. 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.

⚠️ Breaking Changes:

  • scanShieldedTRC20NotesByIvk/Ovk — the events request field is now rejected outright (INVALID_ARGUMENT on 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:

  • Unit Tests — RpcApiServicesTest, WalletMockTest, ShieldedTRC20BuilderTest
  • Build passes (./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

    • Requests including the deprecated "events" field for shielded TRC20 note scans are now rejected with HTTP 400 / gRPC INVALID_ARGUMENT and a clear deprecation message.
  • Updates

    • Scan endpoints no longer accept or forward the deprecated "events" input.
    • TRC20 log-type detection now uses a deterministic topic-byte mapping (unrecognized topics return neutral result).
    • Protobuf scan parameters marked deprecated for "events".
  • Tests

    • Added unit and servlet tests for rejection and guard cases.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 15, 2026

📝 Walkthrough

Walkthrough

Deprecates the events field for shielded TRC20 scans: proto marks it deprecated; gRPC and HTTP endpoints reject non-empty events; internal Wallet APIs remove the topics parameter and simplify log-type detection; tests added/updated to assert rejection and signature changes.

Changes

Cohort / File(s) Summary
Protocol
protocol/src/main/protos/api/api.proto
Marked IvkDecryptTRC20Parameters.events and OvkDecryptTRC20Parameters.events as [deprecated = true].
Core Wallet Logic
framework/src/main/java/org/tron/core/Wallet.java
Removed ProtocolStringList topicsList parameter from shielded TRC20 scan helpers; simplified getShieldedTRC20LogType(...) to classify based only on log.getTopicsList() and adjusted downstream invocations.
gRPC API Validation
framework/src/main/java/org/tron/core/services/RpcApiService.java
Added rejectIfEventsPresent(...) usage in scan handlers to return INVALID_ARGUMENT when non-empty events are provided; stopped forwarding events into Wallet scan calls.
HTTP Servlet Validation
framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java, framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.java
Added calls to Util.rejectIfEventsPresent(...) in doPost/doGet, return HTTP 400 on non-empty events, remove events argument when calling wallet scan; removed unused imports.
Utilities
framework/src/main/java/org/tron/core/services/http/Util.java
Added EVENTS_DEPRECATED_MSG constant and two rejectIfEventsPresent(...) helpers for ProtocolStringList and String[] that throw IllegalArgumentException when any non-empty event is present.
Tests — RPC & Servlets
framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java, framework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.java
Added RPC tests asserting INVALID_ARGUMENT for non-empty events and guard tests for empty events; added servlet tests asserting HTTP 400 and deprecation message; new servlet test class added.
Tests — Call Sites & Mocks
framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java, framework/src/test/java/org/tron/core/WalletMockTest.java
Removed trailing null events args from wallet scan call sites and reflective test signatures; removed unused protobuf imports and construction of topicsList.
Misc
various whitespace/import tidy-ups
Minor formatting and import removals around updated files (non-functional).

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nosed the fields and found a nest,
The old "events" tucked in its rest.
Servers now say "please don't send"—
Hop home, filter on your end.
Thump-thump, the tunnels are blessed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly and concisely summarizes the main change: deprecating the events field in shielded TRC20 scan APIs. It is specific and directly related to the primary objective.
Linked Issues check ✅ Passed All coding requirements from issue #16 are met: events field marked deprecated in api.proto, runtime rejection added in gRPC/HTTP handlers, topicsList parameters removed from Wallet/RpcApiService/servlets, getShieldedTRC20LogType simplified, and comprehensive test coverage added.
Out of Scope Changes check ✅ Passed All changes are directly aligned with deprecating the events field and removing server-side topic filtering. Minor whitespace adjustments in witness pagination are incidental and do not introduce unrelated functionality.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/deprecate-shielded-trc20-events

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 8 files

@Federico2014 Federico2014 force-pushed the fix/deprecate-shielded-trc20-events branch from adee49c to 3335e0b Compare April 17, 2026 06:37
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
framework/src/main/java/org/tron/core/Wallet.java (1)

3947-3955: Keep the IVK index counter aligned with note-bearing logs.

getNoteTxFromLogListByIvk() only produces a NoteTx for log types 1..3, but this loop now increments index for any logType > 0, including TokenBurn (logType == 4). After removing client-side topic filtering, a burn transaction that emits both leaf and token-burn logs can shift or gap the returned index, which no longer matches the proto’s “index of note in txid” contract. Please verify this with a burn + change-note case; if index is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 208807d and 3335e0b.

📒 Files selected for processing (8)
  • framework/src/main/java/org/tron/core/Wallet.java
  • framework/src/main/java/org/tron/core/services/RpcApiService.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/ShieldedTRC20BuilderTest.java
  • framework/src/test/java/org/tron/core/WalletMockTest.java
  • framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
  • protocol/src/main/protos/api/api.proto

Comment on lines +47 to +50
static void rejectIfEventsPresent(String eventsParam) {
if (eventsParam != null && !eventsParam.isEmpty()) {
throw new IllegalArgumentException(EVENTS_DEPRECATED_MSG);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.

@Federico2014 Federico2014 force-pushed the fix/deprecate-shielded-trc20-events branch 2 times, most recently from 53beedf to 63fe9fd Compare April 17, 2026 15:54
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

rejectIfEventsPresent correctly rejects with INVALID_ARGUMENT when the deprecated events field is populated, and ProtocolStringList.getEventsList() from protobuf is guaranteed non-null so the isEmpty() call is safe.

Minor: since this is a breaking change for existing clients, consider a logger.info when rejecting so operators can detect and help migrate clients still sending events. 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_MSG is copy-pasted from ScanShieldedTRC20NotesByIvkServlet. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3335e0b and 63fe9fd.

📒 Files selected for processing (9)
  • framework/src/main/java/org/tron/core/Wallet.java
  • framework/src/main/java/org/tron/core/services/RpcApiService.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/ShieldedTRC20BuilderTest.java
  • framework/src/test/java/org/tron/core/WalletMockTest.java
  • framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
  • framework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.java
  • protocol/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

@Federico2014 Federico2014 force-pushed the fix/deprecate-shielded-trc20-events branch from 87f9d57 to 0fce723 Compare April 20, 2026 06:51
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
framework/src/main/java/org/tron/core/services/RpcApiService.java (1)

296-306: Optional: delegate to Util.rejectIfEventsPresent to 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 translating IllegalArgumentException into Status.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 two rejectIfEventsPresent overloads.

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 the ProtocolStringList variant does not. Protobuf-generated getEventsList() 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

📥 Commits

Reviewing files that changed from the base of the PR and between 87f9d57 and 0fce723.

📒 Files selected for processing (10)
  • framework/src/main/java/org/tron/core/Wallet.java
  • framework/src/main/java/org/tron/core/services/RpcApiService.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/main/java/org/tron/core/services/http/Util.java
  • framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java
  • framework/src/test/java/org/tron/core/WalletMockTest.java
  • framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
  • framework/src/test/java/org/tron/core/services/http/ScanShieldedTRC20NotesServletTest.java
  • protocol/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Deprecate events field in shielded TRC20 scan APIs

1 participant