Open
Conversation
…upport-dynamics-abi
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
17 issues found across 37 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="abi/src/test/java/org/tron/trident/abi/DefaultFunctionEncoderTest.java">
<violation number="1" location="abi/src/test/java/org/tron/trident/abi/DefaultFunctionEncoderTest.java:90">
P1: Copy-paste bug: `nazz2` is created but never used in the assertion — the test still passes `nazz` instead of `nazz2`, so the "correct handling of empty list of dynamic struct" case is not actually verified.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/TypeReference.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/TypeReference.java:45">
P2: `getInnerTypes()` can return `null` since `innerTypes` is only initialized in one constructor, but all callers in `TypeDecoder` dereference it without null checks (e.g., `innerTypes.size()`). Consider initializing the field to an empty list to avoid potential `NullPointerException`.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java:29">
P1: Inconsistency: the varargs constructor is missing the `Array.class.isAssignableFrom` check that both the List constructor (line 41) and `StaticArray`'s constructor have. Calling `new DynamicArray(nestedArray1, nestedArray2)` with Array-type elements will resolve the wrong component type via `AbiTypes.getType()` instead of using the element's actual class.</violation>
<violation number="2" location="abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java:85">
P1: `DynamicArray.class.isAssignableFrom(...)` should be `Array.class.isAssignableFrom(...)` to also handle `StaticArray` elements (e.g., `uint256[3][]`). The `StaticArray.getTypeAsString()` uses `Array.class` for this check — this should be consistent.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java:372">
P1: Unchecked cast: `Class.forName(type)` may return a class that does not extend `Type`. Use `Class.forName(type).asSubclass(Type.class)` to validate the type hierarchy at load time and fail fast with a clear error instead of causing a deferred `ClassCastException`.</violation>
<violation number="2" location="abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java:372">
P1: Security concern: `Class.forName(type)` with a raw, potentially user-controlled `type` string allows loading arbitrary classes on the classpath, triggering their static initializers. Unlike other `Class.forName` usages in this codebase that use controlled prefixed strings, this passes external input directly. Consider restricting the input — e.g., validating that `type` starts with an expected package prefix like `org.tron.trident.abi.datatypes`.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/CustomErrorEncoder.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/CustomErrorEncoder.java:40">
P1: Bug: `calculateSignatureHash` returns the full 32-byte hash (64 hex chars), but Solidity custom error selectors are only 4 bytes (8 hex chars), identical to function selectors. This was likely copied from `EventEncoder.buildEventSignature` where returning the full hash is correct (event topics are 32 bytes), but for error selectors the hash must be truncated to 4 bytes, as done in `FunctionEncoder.buildMethodId` with `.substring(2, 10)`.</violation>
</file>
<file name="abi/src/test/java/org/tron/trident/abi/CustomErrorEncoderTest.java">
<violation number="1" location="abi/src/test/java/org/tron/trident/abi/CustomErrorEncoderTest.java:75">
P1: Wrong class used: this test calls `EventEncoder.buildMethodSignature` instead of `CustomErrorEncoder.buildErrorSignature`. All other tests in this `CustomErrorEncoderTest` class use `CustomErrorEncoder` methods. This looks like a copy-paste mistake — the dynamic-array error-signature path of `CustomErrorEncoder` is left untested.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java:22">
P2: Exposing a mutable `ArrayList` as `public` breaks encapsulation. External code can modify `itemTypes` out of sync with the struct's actual values. Consider keeping the field `private` and providing a getter that returns an unmodifiable list if external access is needed.</violation>
<violation number="2" location="abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java:55">
P2: The `DynamicArray` and `Array` branches both execute identical code (`getValue().get(i).getTypeAsString()`), and `DynamicArray extends Array`, so the `DynamicArray` check is redundant. These can be merged into a single condition to reduce duplication.</violation>
</file>
<file name="abi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.java">
<violation number="1" location="abi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.java:21">
P2: Arguments to `assertEquals` are swapped: JUnit 5's signature is `assertEquals(expected, actual)`, but here the actual value is passed first. This won't break the test, but if it ever fails, the error message will be inverted and confusing.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/datatypes/Utf8String.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/datatypes/Utf8String.java:36">
P1: Potential `NullPointerException`: `value` can be `null` (the constructor doesn't prevent it and `equals`/`hashCode` explicitly handle `null`). Calling `value.isEmpty()` will throw NPE. Add a null guard.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/TypeDecoder.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/TypeDecoder.java:993">
P1: Bug: Offset calculation after decoding a nested `StaticArray` reads array data as a length prefix. `decodeUintAsInt(input, currOffset)` interprets the first element of the already-decoded static array as a byte-length (the formula `(val / MAX_BYTE_LENGTH) + 2` is for length-prefixed DynamicBytes/Utf8String). Static arrays are inline-encoded with no length prefix, so the correct advance should be based on the known `staticLength` and inner element size (e.g., `staticLength * MAX_BYTE_LENGTH_FOR_HEX_STRING` for simple types).</violation>
</file>
<file name="core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java">
<violation number="1" location="core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java:539">
P2: Missing `Thread.sleep()` (or polling for confirmation) between `broadcastTransaction` and the subsequent `triggerConstantContract` read. Without waiting for the transaction to be confirmed, `lastNestedInfo` may return stale data, making this test flaky. The `deployContract` test correctly waits 5 seconds after broadcasting.</violation>
</file>
<file name="abi/src/test/java/org/tron/trident/abi/TypeEncoderPackedTest.java">
<violation number="1" location="abi/src/test/java/org/tron/trident/abi/TypeEncoderPackedTest.java:112">
P2: Swapped `assertEquals` arguments: `assertEquals(expected, actual)` convention requires the expected literal first. This causes confusing failure messages where expected and actual labels are flipped.</violation>
</file>
<file name="abi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.java">
<violation number="1" location="abi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.java:64">
P2: Duplicate constants: `FUNC_IDNARBARFOONARFOODYNAMICARRAY` and `FUNC_IDNARBARFOONARFOOARRAYS` both have value `"idNarBarFooNarFooArrays"`. Consider removing the redundant constant and using a single one to avoid confusion.</violation>
</file>
<file name="abi/src/main/java/org/tron/trident/abi/Utils.java">
<violation number="1" location="abi/src/main/java/org/tron/trident/abi/Utils.java:315">
P1: Bug: Field ordering is broken for structs with nested structs. `canonicalFields` (non-struct) are concatenated before `nestedFields` (expanded struct fields), which destroys the original declaration order. For `struct Baz { Bar { int a, int b }, int c }`, this returns `{c, a, b}` instead of the documented `{a, b, c}`. Since this is used for ABI encoding/decoding, field order matters. The fix should iterate fields in declaration order and either include them directly or recursively expand nested structs inline.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| assertEquals( | ||
| "someFunc((((string,string)[])[],uint256))", | ||
| FunctionEncoder.buildMethodSignature("someFunc", Arrays.asList(nazz))); |
There was a problem hiding this comment.
P1: Copy-paste bug: nazz2 is created but never used in the assertion — the test still passes nazz instead of nazz2, so the "correct handling of empty list of dynamic struct" case is not actually verified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/test/java/org/tron/trident/abi/DefaultFunctionEncoderTest.java, line 90:
<comment>Copy-paste bug: `nazz2` is created but never used in the assertion — the test still passes `nazz` instead of `nazz2`, so the "correct handling of empty list of dynamic struct" case is not actually verified.</comment>
<file context>
@@ -57,6 +59,53 @@ public void testBuildEmptyMethodSignature() {
+
+ assertEquals(
+ "someFunc((((string,string)[])[],uint256))",
+ FunctionEncoder.buildMethodSignature("someFunc", Arrays.asList(nazz)));
+
+ // correct handling of empty list of dynamic struct
</file context>
Suggested change
| FunctionEncoder.buildMethodSignature("someFunc", Arrays.asList(nazz))); | |
| FunctionEncoder.buildMethodSignature("someFunc", Arrays.asList(nazz2))); |
| } else { | ||
| if (StructType.class.isAssignableFrom(value.get(0).getClass())) { | ||
| type = value.get(0).getTypeAsString(); | ||
| } else if (DynamicArray.class.isAssignableFrom(value.get(0).getClass())) { |
There was a problem hiding this comment.
P1: DynamicArray.class.isAssignableFrom(...) should be Array.class.isAssignableFrom(...) to also handle StaticArray elements (e.g., uint256[3][]). The StaticArray.getTypeAsString() uses Array.class for this check — this should be consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java, line 85:
<comment>`DynamicArray.class.isAssignableFrom(...)` should be `Array.class.isAssignableFrom(...)` to also handle `StaticArray` elements (e.g., `uint256[3][]`). The `StaticArray.getTypeAsString()` uses `Array.class` for this check — this should be consistent.</comment>
<file context>
@@ -60,6 +70,24 @@ public DynamicArray(Class<T> type, T... values) {
+ } else {
+ if (StructType.class.isAssignableFrom(value.get(0).getClass())) {
+ type = value.get(0).getTypeAsString();
+ } else if (DynamicArray.class.isAssignableFrom(value.get(0).getClass())) {
+ type = value.get(0).getTypeAsString();
+ } else {
</file context>
Suggested change
| } else if (DynamicArray.class.isAssignableFrom(value.get(0).getClass())) { | |
| } else if (Array.class.isAssignableFrom(value.get(0).getClass())) { |
| public DynamicArray(T... values) { | ||
| super((Class<T>) AbiTypes.getType(values[0].getTypeAsString()), values); | ||
| super( | ||
| StructType.class.isAssignableFrom(values[0].getClass()) |
There was a problem hiding this comment.
P1: Inconsistency: the varargs constructor is missing the Array.class.isAssignableFrom check that both the List constructor (line 41) and StaticArray's constructor have. Calling new DynamicArray(nestedArray1, nestedArray2) with Array-type elements will resolve the wrong component type via AbiTypes.getType() instead of using the element's actual class.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java, line 29:
<comment>Inconsistency: the varargs constructor is missing the `Array.class.isAssignableFrom` check that both the List constructor (line 41) and `StaticArray`'s constructor have. Calling `new DynamicArray(nestedArray1, nestedArray2)` with Array-type elements will resolve the wrong component type via `AbiTypes.getType()` instead of using the element's actual class.</comment>
<file context>
@@ -24,13 +25,22 @@ public class DynamicArray<T extends Type> extends Array<T> {
public DynamicArray(T... values) {
- super((Class<T>) AbiTypes.getType(values[0].getTypeAsString()), values);
+ super(
+ StructType.class.isAssignableFrom(values[0].getClass())
+ ? (Class<T>) values[0].getClass()
+ : (Class<T>) AbiTypes.getType(values[0].getTypeAsString()),
</file context>
Suggested change
| StructType.class.isAssignableFrom(values[0].getClass()) | |
| StructType.class.isAssignableFrom(values[0].getClass()) | |
| || Array.class.isAssignableFrom(values[0].getClass()) |
| throw new UnsupportedOperationException("Unsupported type encountered: " + type); | ||
| default: { | ||
| try { | ||
| return (Class<? extends Type>) Class.forName(type); |
There was a problem hiding this comment.
P1: Unchecked cast: Class.forName(type) may return a class that does not extend Type. Use Class.forName(type).asSubclass(Type.class) to validate the type hierarchy at load time and fail fast with a clear error instead of causing a deferred ClassCastException.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java, line 372:
<comment>Unchecked cast: `Class.forName(type)` may return a class that does not extend `Type`. Use `Class.forName(type).asSubclass(Type.class)` to validate the type hierarchy at load time and fail fast with a clear error instead of causing a deferred `ClassCastException`.</comment>
<file context>
@@ -367,11 +367,22 @@ public static Class<? extends Type> getType(String type, boolean primitives) {
- throw new UnsupportedOperationException("Unsupported type encountered: " + type);
+ default: {
+ try {
+ return (Class<? extends Type>) Class.forName(type);
+ } catch (ClassNotFoundException e) {
+ throw new UnsupportedOperationException(
</file context>
Suggested change
| return (Class<? extends Type>) Class.forName(type); | |
| return Class.forName(type).asSubclass(Type.class); |
| throw new UnsupportedOperationException("Unsupported type encountered: " + type); | ||
| default: { | ||
| try { | ||
| return (Class<? extends Type>) Class.forName(type); |
There was a problem hiding this comment.
P1: Security concern: Class.forName(type) with a raw, potentially user-controlled type string allows loading arbitrary classes on the classpath, triggering their static initializers. Unlike other Class.forName usages in this codebase that use controlled prefixed strings, this passes external input directly. Consider restricting the input — e.g., validating that type starts with an expected package prefix like org.tron.trident.abi.datatypes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java, line 372:
<comment>Security concern: `Class.forName(type)` with a raw, potentially user-controlled `type` string allows loading arbitrary classes on the classpath, triggering their static initializers. Unlike other `Class.forName` usages in this codebase that use controlled prefixed strings, this passes external input directly. Consider restricting the input — e.g., validating that `type` starts with an expected package prefix like `org.tron.trident.abi.datatypes`.</comment>
<file context>
@@ -367,11 +367,22 @@ public static Class<? extends Type> getType(String type, boolean primitives) {
- throw new UnsupportedOperationException("Unsupported type encountered: " + type);
+ default: {
+ try {
+ return (Class<? extends Type>) Class.forName(type);
+ } catch (ClassNotFoundException e) {
+ throw new UnsupportedOperationException(
</file context>
| public class DynamicStruct extends DynamicArray<Type> implements StructType { | ||
|
|
||
| private final List<Class<Type>> itemTypes = new ArrayList<>(); | ||
| public final List<Class<Type>> itemTypes = new ArrayList<>(); |
There was a problem hiding this comment.
P2: Exposing a mutable ArrayList as public breaks encapsulation. External code can modify itemTypes out of sync with the struct's actual values. Consider keeping the field private and providing a getter that returns an unmodifiable list if external access is needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java, line 22:
<comment>Exposing a mutable `ArrayList` as `public` breaks encapsulation. External code can modify `itemTypes` out of sync with the struct's actual values. Consider keeping the field `private` and providing a getter that returns an unmodifiable list if external access is needed.</comment>
<file context>
@@ -19,12 +19,13 @@
public class DynamicStruct extends DynamicArray<Type> implements StructType {
- private final List<Class<Type>> itemTypes = new ArrayList<>();
+ public final List<Class<Type>> itemTypes = new ArrayList<>();
public DynamicStruct(List<Type> values) {
</file context>
| new TypeReference<Address>() {}, new TypeReference<Uint256>() {}); | ||
| CustomError event = new CustomError("MyError", parameters); | ||
|
|
||
| assertEquals(event.getName(), "MyError"); |
There was a problem hiding this comment.
P2: Arguments to assertEquals are swapped: JUnit 5's signature is assertEquals(expected, actual), but here the actual value is passed first. This won't break the test, but if it ever fails, the error message will be inverted and confusing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.java, line 21:
<comment>Arguments to `assertEquals` are swapped: JUnit 5's signature is `assertEquals(expected, actual)`, but here the actual value is passed first. This won't break the test, but if it ever fails, the error message will be inverted and confusing.</comment>
<file context>
@@ -0,0 +1,29 @@
+ new TypeReference<Address>() {}, new TypeReference<Uint256>() {});
+ CustomError event = new CustomError("MyError", parameters);
+
+ assertEquals(event.getName(), "MyError");
+
+ Iterator<TypeReference<?>> expectedParameter = parameters.iterator();
</file context>
| client.deployContract("AbiV2TestContract", CONTRACT_ABI, CONTRACT_BYTECODE, null, 1000_000_000L, 100, 10_000_000L, 0, null, 0); | ||
|
|
||
| Transaction signTransaction = client.signTransaction(transactionExtention.getTransaction()); | ||
| String txId = client.broadcastTransaction(signTransaction); |
There was a problem hiding this comment.
P2: Missing Thread.sleep() (or polling for confirmation) between broadcastTransaction and the subsequent triggerConstantContract read. Without waiting for the transaction to be confirmed, lastNestedInfo may return stale data, making this test flaky. The deployContract test correctly waits 5 seconds after broadcasting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java, line 539:
<comment>Missing `Thread.sleep()` (or polling for confirmation) between `broadcastTransaction` and the subsequent `triggerConstantContract` read. Without waiting for the transaction to be confirmed, `lastNestedInfo` may return stale data, making this test flaky. The `deployContract` test correctly waits 5 seconds after broadcasting.</comment>
<file context>
@@ -0,0 +1,780 @@
+ client.deployContract("AbiV2TestContract", CONTRACT_ABI, CONTRACT_BYTECODE, null, 1000_000_000L, 100, 10_000_000L, 0, null, 0);
+
+ Transaction signTransaction = client.signTransaction(transactionExtention.getTransaction());
+ String txId = client.broadcastTransaction(signTransaction);
+ System.out.println("Deploy transaction ID: " + txId);
+
</file context>
|
|
||
| @Test | ||
| public void testBoolEncodePacked() { | ||
| assertEquals(TypeEncoder.encodePacked(new Bool(false)), ("00")); |
There was a problem hiding this comment.
P2: Swapped assertEquals arguments: assertEquals(expected, actual) convention requires the expected literal first. This causes confusing failure messages where expected and actual labels are flipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/test/java/org/tron/trident/abi/TypeEncoderPackedTest.java, line 112:
<comment>Swapped `assertEquals` arguments: `assertEquals(expected, actual)` convention requires the expected literal first. This causes confusing failure messages where expected and actual labels are flipped.</comment>
<file context>
@@ -0,0 +1,1082 @@
+
+ @Test
+ public void testBoolEncodePacked() {
+ assertEquals(TypeEncoder.encodePacked(new Bool(false)), ("00"));
+ assertEquals(TypeEncoder.encodePacked(new Bool(true)), ("01"));
+ }
</file context>
|
|
||
| public static final String FUNC_GETNARBARFOONARFOODYNAMICARRAY = "getNarBarFooNarFooArrays"; | ||
|
|
||
| public static final String FUNC_IDNARBARFOONARFOODYNAMICARRAY = "idNarBarFooNarFooArrays"; |
There was a problem hiding this comment.
P2: Duplicate constants: FUNC_IDNARBARFOONARFOODYNAMICARRAY and FUNC_IDNARBARFOONARFOOARRAYS both have value "idNarBarFooNarFooArrays". Consider removing the redundant constant and using a single one to avoid confusion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.java, line 64:
<comment>Duplicate constants: `FUNC_IDNARBARFOONARFOODYNAMICARRAY` and `FUNC_IDNARBARFOONARFOOARRAYS` both have value `"idNarBarFooNarFooArrays"`. Consider removing the redundant constant and using a single one to avoid confusion.</comment>
<file context>
@@ -31,12 +42,60 @@ public class AbiV2TestFixture {
+
+ public static final String FUNC_GETNARBARFOONARFOODYNAMICARRAY = "getNarBarFooNarFooArrays";
+
+ public static final String FUNC_IDNARBARFOONARFOODYNAMICARRAY = "idNarBarFooNarFooArrays";
+
+ public static final String FUNC_GETBARDYNAMICARRAY = "getBarDynamicArray";
</file context>
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
1 participant
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.
What does this PR do?
support abiV2 include static/dynamics arrays, struct
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Adds ABI v2 support: encode/decode structs (tuples), static/dynamic (including nested) arrays, packed encoding, and custom errors. Also fixes decoding issues and normalizes event signature hashes (no 0x).
New Features
Bug Fixes
Written for commit 514213c. Summary will update on new commits.