Conversation
…upport-dynamics-abi
📝 WalkthroughWalkthroughThis PR significantly enhances the ABI encoding/decoding library with support for custom Ethereum error encoding, packed value encoding (abi.encodePacked), nested struct and complex array handling, SPI-based encoder/decoder selection, and improved type resolution. Introduces new public APIs and extensive test coverage for complex data structures. Changes
Sequence Diagram(s)This change involves encoding/decoding enhancements across multiple heterogeneous areas (SPI patterns, padding calculations, struct handling, packed encoding) that do not form a cohesive sequential interaction flow suitable for a single diagram. The modifications are largely distributed logic enhancements in parallel paths rather than a unified multi-component interaction pattern. Sequence diagrams are not applicable. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Comment |
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.
abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java
Outdated
Show resolved
Hide resolved
|
@claude review this pr |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
abi/src/main/java/org/tron/trident/abi/TypeEncoder.java (1)
353-358:⚠️ Potential issue | 🟠 MajorUse UTF-8 byte length for string offset calculation in ABI encoding.
Line 356 uses
String.length(), which returns the character count in UTF-16, not the byte length required for ABI offset calculation. For strings with multi-byte UTF-8 characters, this will undercount the encoded size and shift all subsequent element offsets. UsegetBytes(StandardCharsets.UTF_8).lengthinstead.Suggested fix
int bytesLength = arrayOfBytes ? ((byte[]) value.getValue().get(i - 1).getValue()).length - : ((String) value.getValue().get(i - 1).getValue()).length(); + : ((String) value.getValue().get(i - 1).getValue()) + .getBytes(StandardCharsets.UTF_8).length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/TypeEncoder.java` around lines 353 - 358, In TypeEncoder (the block computing bytesLength/numberOfWords for array elements), replace use of String.length() with the UTF-8 byte length by calling ((String) value.getValue().get(i - 1).getValue()).getBytes(StandardCharsets.UTF_8).length so offsets use encoded byte size; add/import java.nio.charset.StandardCharsets if missing and keep arrayOfBytes branch unchanged (byte[] uses .length). This ensures numberOfWords and totalBytesLength are computed from UTF-8 bytes rather than UTF-16 char count.
🧹 Nitpick comments (15)
abi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.java (1)
21-26: JUnit assertion argument order and incomplete iteration check.
JUnit convention is
assertEquals(expected, actual). The current orderassertEquals(event.getName(), "MyError")has actual first, which will produce confusing failure messages.The iteration loop doesn't verify that
event.getParameters()doesn't have more elements than expected. If the error had extra parameters, the test would pass incorrectly.♻️ Proposed fix
- assertEquals(event.getName(), "MyError"); + assertEquals("MyError", event.getName()); + assertEquals(parameters.size(), event.getParameters().size()); Iterator<TypeReference<?>> expectedParameter = parameters.iterator(); for (TypeReference<?> actualParameter : event.getParameters()) { - assertEquals(expectedParameter.next(), actualParameter); + assertEquals(expectedParameter.next(), actualParameter); }Or use a more direct comparison:
assertEquals("MyError", event.getName()); assertEquals(parameters, event.getParameters());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.java` around lines 21 - 26, Swap assertion argument order and strengthen parameter equality: change assertEquals(event.getName(), "MyError") to assertEquals("MyError", event.getName()); replace the manual iterator loop over event.getParameters() with a direct equality/assertion that verifies both contents and size (for example assertEquals(parameters, event.getParameters()) or assertEquals(parameters.size(), event.getParameters().size()) followed by element-by-element comparison) so that extra parameters on event will fail; update test in CustomErrorTest to use these corrected assertions referencing event and parameters.abi/src/main/java/org/tron/trident/abi/FunctionReturnDecoder.java (3)
75-82: Same raw type issue asdecodeDynamicBytes.♻️ Proposed fix
public static String decodeAddress(String rawInput) { - List outputParameters = new ArrayList<TypeReference<Type>>(); + List<TypeReference<Type>> outputParameters = new ArrayList<>(); outputParameters.add(new TypeReference<Address>() {}); List<Type> typeList = decoder.decodeFunctionResult(rawInput, outputParameters); return typeList.isEmpty() ? null : ((Address) typeList.get(0)).getValue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/FunctionReturnDecoder.java` around lines 75 - 82, The decodeAddress method currently uses a raw List for outputParameters (same raw-type problem as decodeDynamicBytes); change the declaration to a parameterized List<TypeReference<Address>> (e.g., List<TypeReference<Address>> outputParameters = new ArrayList<>()) and keep the TypeReference<Address>(){} element, then call decoder.decodeFunctionResult(rawInput, outputParameters) as before so generics are correct for decodeAddress and related decoding logic.
60-65: Missing Javadoc fordecodeAddress.The
decodeDynamicBytesmethod has Javadoc, butdecodeAddress(lines 75-82) lacks documentation. Both are new public API methods and should be documented consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/FunctionReturnDecoder.java` around lines 60 - 65, Add Javadoc for the public method decodeAddress matching the style and detail of decodeDynamicBytes: describe that it decodes an ABI-encoded address from rawInput, document the parameters (rawInput) and return value (String or Address type and null on invalid response), and mention any exceptions or behavior for invalid input; reference the method name decodeAddress so reviewers can locate it and ensure consistency with the existing decodeDynamicBytes Javadoc format.
66-73: Use parameterized type instead of rawList.The declaration
List outputParameters = new ArrayList<TypeReference<Type>>()uses a raw type on the left side, which bypasses generic type checking and generates compiler warnings.♻️ Proposed fix
public static byte[] decodeDynamicBytes(String rawInput) { - List outputParameters = new ArrayList<TypeReference<Type>>(); + List<TypeReference<Type>> outputParameters = new ArrayList<>(); outputParameters.add(new TypeReference<DynamicBytes>() {}); List<Type> typeList = decoder.decodeFunctionResult(rawInput, outputParameters); return typeList.isEmpty() ? null : ((DynamicBytes) typeList.get(0)).getValue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/FunctionReturnDecoder.java` around lines 66 - 73, In decodeDynamicBytes, replace the raw-typed declaration "List outputParameters = new ArrayList<TypeReference<Type>>()" with a parameterized type such as "List<TypeReference<Type>> outputParameters = new ArrayList<>()" so generics are preserved; keep the existing add of new TypeReference<DynamicBytes>() {} and the subsequent call to decoder.decodeFunctionResult(rawInput, outputParameters) unchanged.abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java (1)
386-394: Rename static method for naming consistency:getTypeAString→getTypeAsString.The static utility method in AbiTypes has a naming inconsistency—it's missing the 's' in "as" compared to the instance method
Type.getTypeAsString()used throughout the codebase. Renaming would improve consistency and discoverability.This change requires updating 5 call sites:
- StaticStruct.java:45
- StaticArray.java:93
- DynamicStruct.java:61
- DynamicArray.java:81, 89
♻️ Proposed rename for consistency
- public static String getTypeAString(Class<? extends Type> type) { + public static String getTypeAsString(Class<? extends Type> type) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java` around lines 386 - 394, Rename the static method AbiTypes.getTypeAString to getTypeAsString and update all callers to use the new name; specifically change the method declaration in AbiTypes (currently getTypeAString) to getTypeAsString and update the five call sites in StaticStruct, StaticArray, DynamicStruct, and DynamicArray to call AbiTypes.getTypeAsString(...) (this aligns the static helper with the instance method Type.getTypeAsString and preserves existing behavior for Utf8String and DynamicBytes by returning "string" and "bytes" respectively).abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java (1)
74-77: Misleading comment: the code actually handles struct arrays correctly.The comment states "This will fail if the dynamic array is an array of structs," but lines 78-79 explicitly handle
StructTypeby callingUtils.getStructType(). Consider removing or correcting the comment.📝 Suggested comment fix
- // Handle dynamic array of zero length. This will fail if the dynamic array - // is an array of structs. + // Handle dynamic array of zero length if (value.isEmpty()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java` around lines 74 - 77, The comment above the empty dynamic-array handling in DynamicArray.java is misleading; update or remove the line claiming "This will fail if the dynamic array is an array of structs" because the subsequent code handles StructType via Utils.getStructType() (see value.isEmpty() check and handling of StructType). Edit the comment to accurately reflect that zero-length dynamic arrays are handled, including struct arrays, or simply remove the incorrect sentence so the code and comment are consistent.abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java (2)
55-59: Redundant conditional branches.Both
ifbranches (StructType/DynamicArray and Array) execute identical code:getValue().get(i).getTypeAsString(). These can be consolidated.♻️ Suggested simplification
- if (StructType.class.isAssignableFrom(cls) - || DynamicArray.class.isAssignableFrom(cls)) { - type.append(getValue().get(i).getTypeAsString()); - } else if (Array.class.isAssignableFrom(cls)) { + if (StructType.class.isAssignableFrom(cls) + || Array.class.isAssignableFrom(cls)) { type.append(getValue().get(i).getTypeAsString()); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java` around lines 55 - 59, The two conditional branches in DynamicStruct (the check using StructType.class.isAssignableFrom(cls) || DynamicArray.class.isAssignableFrom(cls) and the else if Array.class.isAssignableFrom(cls)) both call getValue().get(i).getTypeAsString(); simplify by removing the redundant branch and consolidating into a single branch or unconditional call: inside the method in class DynamicStruct, replace the duplicated condition logic around cls with a single call to getValue().get(i).getTypeAsString() (keeping any surrounding context intact) so only one path uses getTypeAsString().
22-22: Public mutable list exposes internal state.Exposing
itemTypesas apublic finalfield allows external code to mutate the list (add/remove elements). Consider returning an unmodifiable view or making the field private with a getter that returnsCollections.unmodifiableList(itemTypes).♻️ Suggested encapsulation
- public final List<Class<Type>> itemTypes = new ArrayList<>(); + private final List<Class<Type>> itemTypes = new ArrayList<>(); + + public List<Class<Type>> getItemTypes() { + return Collections.unmodifiableList(itemTypes); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java` at line 22, The public mutable field itemTypes in class DynamicStruct exposes internal state; make itemTypes private (e.g., private final List<Class<Type>> itemTypes = new ArrayList<>();) and add a public accessor getItemTypes() that returns Collections.unmodifiableList(itemTypes) so callers cannot mutate the internal list; update any internal usages to reference the private field and adjust external code to call getItemTypes() where necessary.abi/src/main/java/org/tron/trident/abi/datatypes/CustomError.java (1)
22-29: Make fields final for immutability.The
nameandparametersfields are not declaredfinal, which allows accidental reassignment. Since this is a value-holding class, immutability is preferred.♻️ Suggested fix
public class CustomError { - private String name; - private List<TypeReference<Type>> parameters; + private final String name; + private final List<TypeReference<Type>> parameters; public CustomError(String name, List<TypeReference<?>> parameters) { this.name = name; this.parameters = convert(parameters); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/datatypes/CustomError.java` around lines 22 - 29, Make the CustomError class immutable by declaring the fields name and parameters as final and ensuring they are only set in the constructor; update the class declaration to use private final String name and private final List<TypeReference<Type>> parameters, keep the existing constructor CustomError(String name, List<TypeReference<?>> parameters) to assign these finals (using convert(parameters) for parameters), and verify no other code reassigns these fields or requires setters.abi/src/test/java/org/tron/trident/abi/datatypes/DynamicArrayTest.java (1)
38-50: Raw type usage in multidimensional array test.Line 40 uses raw
DynamicArraywithout generic type parameter. While this works for the test, it generates compiler warnings. Consider usingDynamicArray<DynamicArray<DynamicArray<Uint256>>>for type safety.♻️ Suggested type-safe version
`@Test` public void testMultidimensionalDynamicArray() { - DynamicArray<DynamicArray> array = + `@SuppressWarnings`("unchecked") + DynamicArray<DynamicArray<DynamicArray<Uint256>>> array = new DynamicArray<>( DynamicArray.class, Collections.singletonList( new DynamicArray<>( DynamicArray.class, Collections.singletonList( new DynamicArray<>( Uint256.class, new ArrayList<>()))))); assertEquals("uint256[][][]", array.getTypeAsString()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/test/java/org/tron/trident/abi/datatypes/DynamicArrayTest.java` around lines 38 - 50, The testMultidimensionalDynamicArray uses raw types for DynamicArray which causes compiler warnings; update the declarations and constructors to use fully parameterized nested generics (e.g., DynamicArray<DynamicArray<DynamicArray<Uint256>>> and inner DynamicArray<DynamicArray<Uint256>> / DynamicArray<Uint256>) so the test remains the same (assertEquals("uint256[][][]", array.getTypeAsString())) but is type-safe; adjust the DynamicArray instantiations in the test to use these generic types rather than raw DynamicArray.abi/src/main/java/org/tron/trident/abi/TypeEncoder.java (1)
289-297: Reuse the encoded dynamic value.
encode(type)is invoked twice for every dynamic member here. ReusingencodedValueavoids double work on nested structs/arrays and keeps the offset math tied to the exact payload appended.Suggested fix
String encodedValue = encode(type); - dynamicValues.add(encode(type)); + dynamicValues.add(encodedValue); dynamicOffset += encodedValue.length() >> 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/TypeEncoder.java` around lines 289 - 297, The code calls encode(type) twice for dynamic members; capture the result once and reuse it: replace the second encode(type) call with the local encodedValue variable so dynamicValues.add(encodedValue) is used and dynamicOffset is incremented from encodedValue.length() (keeping the current shift/right-by-1 logic). Update the block in TypeEncoder where isDynamic(type) is handled (references: dynamicOffset, offsetsAndStaticValues, dynamicValues, encodedValue, encode(type)) to ensure the offset arithmetic and appended payload use the exact same encoded bytes.core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java (1)
718-723: ExerciseFunctionEncoderhere instead of hardcoded calldata.The test builds
functionabove but then bypasses it with a raw selector/payload string. That leaves the new multi-dimensional array encoder path untested, so an encoder regression can still keep this test green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java` around lines 718 - 723, The test currently constructs a Function named `function` but calls `client.triggerConstantContract(...)` with a hardcoded calldata string; replace that raw hex with the encoded calldata produced from the constructed Function by calling the encoder (e.g. FunctionEncoder.encode(function)) and pass that encoded hex string into `client.triggerConstantContract(testAddress, contractAddress, /* encoded calldata */)` so the multi-dimensional array encoding path is exercised; keep the same `testAddress` and `contractAddress` parameters and assert on the returned `transactionExtention` as before.abi/src/main/java/org/tron/trident/abi/TypeDecoder.java (2)
766-771: Consider using a more specific exception type.Throwing a generic
RuntimeExceptionat line 768 when the parameter annotation is missing makes it harder to catch and handle this specific error condition programmatically. Consider usingIllegalArgumentExceptionor a custom exception type.♻️ Proposed change
} else if (DynamicArray.class.isAssignableFrom(declaredField)) { if (parameter == null) { - throw new RuntimeException( + throw new IllegalArgumentException( "parameter can not be null, try to use annotation `@Parameterized` " + "to specify the parameter type"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/TypeDecoder.java` around lines 766 - 771, Replace the generic RuntimeException thrown in the DynamicArray handling branch of TypeDecoder (the block where DynamicArray.class.isAssignableFrom(declaredField) and parameter == null) with a more specific exception; change the throw to IllegalArgumentException (or a custom exception type) and include the same explanatory message so callers can catch this specific invalid-argument condition when the `@Parameterized` annotation is missing.
500-509: TheisAssignableFromdirection is correct but semantically confusing.Lines 501 and 503 use
classType.isAssignableFrom(DynamicStruct.class)andclassType.isAssignableFrom(StaticStruct.class), which returntrueonly whenclassTypeISDynamicStruct/StaticStructitself (not for subclasses). This correctly routes base types to their list constructors and custom struct subclasses likeBar,Qux,BytesStructto the reflection path in theelsebranch, which is the intended behavior. However, the condition reads backwards from typical pattern matching, which could confuse future maintainers—consider clarifying with a comment or renaming to reflect that this checks for base types specifically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/main/java/org/tron/trident/abi/TypeDecoder.java` around lines 500 - 509, Replace the confusing use of classType.isAssignableFrom(DynamicStruct.class) and classType.isAssignableFrom(StaticStruct.class) in TypeDecoder with explicit equality checks (e.g., classType.equals(DynamicStruct.class) and classType.equals(StaticStruct.class)) to make the intention clear that only the exact base types should match; keep the existing reflection fallback (findStructConstructor, ctor.newInstance) for custom struct subclasses and preserve casting of parameters to List<Type> for the DynamicStruct/StaticStruct branches.abi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.java (1)
928-961: Multi-dimensional array fixtures use necessary raw type casts.The raw type casts like
(Class)StaticArray2.classat lines 931, 941, etc., are unavoidable due to Java's type erasure limitations. Consider adding@SuppressWarnings("unchecked")or@SuppressWarnings("rawtypes")to suppress compiler warnings for these fixture declarations.♻️ Proposed annotation
+ `@SuppressWarnings`({"unchecked", "rawtypes"}) public static final Function setGetMultiDimStaticArrayFunction = new Function(FUNC_SETGETMULTIDIMSTATICARRAY,+ `@SuppressWarnings`({"unchecked", "rawtypes"}) public static final Function setGetMultiDimDynamicArrayFunction = new Function(FUNC_SETGETMULTIDIMDYNAMICARRAY,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.java` around lines 928 - 961, The multi-dimensional array fixture declarations setGetMultiDimStaticArrayFunction and setGetMultiDimDynamicArrayFunction use unavoidable raw type casts (e.g., (Class)StaticArray2.class and (Class)DynamicArray.class) that trigger unchecked/rawtypes compiler warnings; suppress these warnings by annotating the fixture fields with `@SuppressWarnings`("unchecked") or `@SuppressWarnings`("rawtypes") as appropriate (place the annotation immediately above the static final field declarations for setGetMultiDimStaticArrayFunction and setGetMultiDimDynamicArrayFunction) so the casts remain but the compiler warnings are silenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@abi/src/main/java/org/tron/trident/abi/CustomErrorEncoder.java`:
- Around line 37-41: The calculateSignatureHash method uses platform default
bytes; change errorSignature.getBytes() to use a deterministic UTF-8 encoding
(e.g., errorSignature.getBytes(StandardCharsets.UTF_8)) and add the required
import (java.nio.charset.StandardCharsets) so the SHA3 hash is computed
consistently across platforms; update the method in
CustomErrorEncoder.calculateSignatureHash accordingly.
In `@abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java`:
- Around line 370-377: In the default branch of AbiTypes (the code that
currently does (Class<? extends Type>) Class.forName(type)), avoid the unchecked
cast and unsafe initialization: load the class without running static
initializers using Class.forName(type, false,
Thread.currentThread().getContextClassLoader()), then validate it implements
org.tron.trident.abi.datatypes.Type by using Class#asSubclass(Type.class) (or
checking Type.class.isAssignableFrom(loaded)), throwing a clear
UnsupportedOperationException if not; return the resulting Class<? extends Type>
from asSubclass so the cast is safe and static initializers are not executed.
In `@abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java`:
- Around line 27-34: The varargs DynamicArray(T... values) constructor (and the
List-based DynamicArray(List<T> values) constructor) access
values.get(0)/values[0] without checking emptiness, which can throw
ArrayIndexOutOfBoundsException; fix by adding a defensive precondition at the
top of both constructors that validates values/values list is non-empty and
throws a clear IllegalArgumentException (e.g., "values must not be empty")
instead of letting an AIOOBE occur—update the DynamicArray(T... values) and
DynamicArray(List<T> values) constructors accordingly.
In `@abi/src/main/java/org/tron/trident/abi/datatypes/StaticArray.java`:
- Around line 51-56: The deprecated StaticArray(List<T> values) constructor
reads values.get(0) and can throw IndexOutOfBoundsException for an empty list;
update the constructor in class StaticArray to guard against empty values by
checking values == null || values.isEmpty() and selecting a sensible default
element type (e.g., using AbiTypes.getType("uint256") or another appropriate
fallback) or by throwing an IllegalArgumentException with a clear message;
ensure the logic that currently branches on
StructType.class.isAssignableFrom(...) || Array.class.isAssignableFrom(...)
still uses the chosen fallback when the list is empty and keep the existing
behavior for non-empty lists (references: constructor StaticArray(List<T>
values), values.get(0), AbiTypes.getType).
In `@abi/src/main/java/org/tron/trident/abi/datatypes/Utf8String.java`:
- Around line 30-41: The bytes32PaddedLength() override in Utf8String is
misleading and unused for dynamic strings; remove the override from class
Utf8String so the class falls back to the default Type.bytes32PaddedLength()
behavior, or if you intend a special-purpose behavior keep the method but update
its Javadoc to clearly state it is not used for ABI dynamic encoding and return
a value that correctly documents that special use; locate the method by name
(bytes32PaddedLength) in Utf8String and consider callers like
TypeEncoder.encodeString() and encodeDynamicBytes() and the base Type
implementation when making the change.
In `@abi/src/main/java/org/tron/trident/abi/DefaultFunctionReturnDecoder.java`:
- Around line 104-144: The TypeReference.StaticArrayTypeReference branch in
DefaultFunctionReturnDecoder currently advances offset as length *
MAX_BYTE_LENGTH_FOR_HEX_STRING unconditionally; change it to compute the
top-level head size consistently with the StaticArray handling: extract the
parameterized element type from the TypeReference.StaticArrayTypeReference (via
the same logic used by getParameterizedTypeFromArray), then if the parameterized
type is DynamicStruct or Utf8String advance by one slot
(MAX_BYTE_LENGTH_FOR_HEX_STRING), if it is StaticStruct advance by
staticStructNestedPublicFieldsFlatList(elementClass).size() * length *
MAX_BYTE_LENGTH_FOR_HEX_STRING, otherwise advance by length *
MAX_BYTE_LENGTH_FOR_HEX_STRING; keep using TypeDecoder.decodeStaticArray for
decoding. Ensure you update the offset calculation in the
TypeReference.StaticArrayTypeReference branch to mirror the StaticArray branch's
branches (DynamicStruct, StaticStruct, Utf8String, fallback).
In `@abi/src/main/java/org/tron/trident/abi/TypeDecoder.java`:
- Around line 391-406: countNestedFields can NPE when
typeReference.getInnerTypes() is null for nested StaticStructs; update
countNestedFields to defensively handle a null return from
TypeReference.getInnerTypes() (for StaticStruct.class cases) by treating it as
empty (i.e. return 0) before calling stream(), so that StaticStructs with
missing inner type info do not cause an NPE; reference the countNestedFields
method, TypeReference.getInnerTypes(), StaticStruct.class and Utils.getTypeName
in your change.
- Around line 988-995: The offset after decoding a nested static array is being
computed as if the array were dynamic by reading a length prefix via
decodeUintAsInt at currOffset; instead, update the logic in TypeDecoder after
the call to TypeDecoder.decodeStaticArray to advance currOffset based on the
known staticLength and the element size (use staticLength and
Type.MAX_BYTE_LENGTH / element byte size logic) rather than reading a length
from the input with decodeUintAsInt; ensure you use
MAX_BYTE_LENGTH_FOR_HEX_STRING and Type.MAX_BYTE_LENGTH consistently to compute
the exact number of hex chars consumed for staticLength * elementSize so
currOffset points to the next ABI value.
In `@abi/src/main/java/org/tron/trident/abi/TypeEncoder.java`:
- Around line 53-60: The isDynamic(Type) method currently only treats a
StaticArray as dynamic when its component type is DynamicStruct; change it to
detect any dynamic component (DynamicBytes, Utf8String, DynamicArray, or any
type that itself returns true from isDynamic) so StaticArray with dynamic
elements returns true; then update the encode() routing so that arrays of those
StaticArray types go through encodeDynamicsTypesArraysOffsets (or the same
dynamic-array offset handling path) instead of inline encoding. Reference:
isDynamic, StaticArray.getComponentType(), DynamicBytes, Utf8String,
DynamicArray, DynamicStruct, encodeDynamicsTypesArraysOffsets, and encode().
In `@abi/src/main/java/org/tron/trident/abi/Utils.java`:
- Around line 124-136: findStructConstructor currently uses
Arrays.stream(...).findAny() which can pick an arbitrary matching constructor;
instead select the most specific constructor (prefer the one with the smallest
parameter count or the one with the fewest parameters equal to raw Type.class)
so typed constructors win over the generic bridge. Modify findStructConstructor
to stream declared constructors, filter by
allMatch(Type.class::isAssignableFrom) as before, then sort or reduce to pick
the constructor with minimal parameter length (and as a tiebreaker minimal count
of parameters exactly equal to Type.class) and return that or throw the same
RuntimeException if none found.
In `@abi/src/test/java/org/tron/trident/abi/CustomErrorEncoderTest.java`:
- Around line 68-76: The test method testBuildErrorSignatureWithDynamicArrays
currently calls EventEncoder.buildMethodSignature but lives in
CustomErrorEncoderTest; change the call to
CustomErrorEncoder.buildErrorSignature (or add a clarifying comment if you
intentionally compare both encoders) so the test asserts
CustomErrorEncoder.buildErrorSignature("DynamicArrayError", convert(parameters))
and remains consistent with the test class name; update only the invocation from
EventEncoder.buildMethodSignature to CustomErrorEncoder.buildErrorSignature in
that test (or add a one-line comment above the assertion explaining the
cross-check intent).
In `@core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java`:
- Around line 449-450: The test in AbiV2ContractTest writes lastNestedInfo to
the public contractAddress and immediately reads it back, which can race with
block finality or external writes; update the test to poll for transaction
confirmation using getTransactionInfoById(txId) (or alternatively deploy an
isolated contract per test run) and only perform the read/assert once
getTransactionInfoById returns a confirmed/valid receipt for the write
transaction; apply the same change to the similar read-after-write assertions
around lines 635-657 to ensure the test waits for the write to be finalized
before asserting shared contract state.
---
Outside diff comments:
In `@abi/src/main/java/org/tron/trident/abi/TypeEncoder.java`:
- Around line 353-358: In TypeEncoder (the block computing
bytesLength/numberOfWords for array elements), replace use of String.length()
with the UTF-8 byte length by calling ((String) value.getValue().get(i -
1).getValue()).getBytes(StandardCharsets.UTF_8).length so offsets use encoded
byte size; add/import java.nio.charset.StandardCharsets if missing and keep
arrayOfBytes branch unchanged (byte[] uses .length). This ensures numberOfWords
and totalBytesLength are computed from UTF-8 bytes rather than UTF-16 char
count.
---
Nitpick comments:
In `@abi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.java`:
- Around line 386-394: Rename the static method AbiTypes.getTypeAString to
getTypeAsString and update all callers to use the new name; specifically change
the method declaration in AbiTypes (currently getTypeAString) to getTypeAsString
and update the five call sites in StaticStruct, StaticArray, DynamicStruct, and
DynamicArray to call AbiTypes.getTypeAsString(...) (this aligns the static
helper with the instance method Type.getTypeAsString and preserves existing
behavior for Utf8String and DynamicBytes by returning "string" and "bytes"
respectively).
In `@abi/src/main/java/org/tron/trident/abi/datatypes/CustomError.java`:
- Around line 22-29: Make the CustomError class immutable by declaring the
fields name and parameters as final and ensuring they are only set in the
constructor; update the class declaration to use private final String name and
private final List<TypeReference<Type>> parameters, keep the existing
constructor CustomError(String name, List<TypeReference<?>> parameters) to
assign these finals (using convert(parameters) for parameters), and verify no
other code reassigns these fields or requires setters.
In `@abi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.java`:
- Around line 74-77: The comment above the empty dynamic-array handling in
DynamicArray.java is misleading; update or remove the line claiming "This will
fail if the dynamic array is an array of structs" because the subsequent code
handles StructType via Utils.getStructType() (see value.isEmpty() check and
handling of StructType). Edit the comment to accurately reflect that zero-length
dynamic arrays are handled, including struct arrays, or simply remove the
incorrect sentence so the code and comment are consistent.
In `@abi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.java`:
- Around line 55-59: The two conditional branches in DynamicStruct (the check
using StructType.class.isAssignableFrom(cls) ||
DynamicArray.class.isAssignableFrom(cls) and the else if
Array.class.isAssignableFrom(cls)) both call
getValue().get(i).getTypeAsString(); simplify by removing the redundant branch
and consolidating into a single branch or unconditional call: inside the method
in class DynamicStruct, replace the duplicated condition logic around cls with a
single call to getValue().get(i).getTypeAsString() (keeping any surrounding
context intact) so only one path uses getTypeAsString().
- Line 22: The public mutable field itemTypes in class DynamicStruct exposes
internal state; make itemTypes private (e.g., private final List<Class<Type>>
itemTypes = new ArrayList<>();) and add a public accessor getItemTypes() that
returns Collections.unmodifiableList(itemTypes) so callers cannot mutate the
internal list; update any internal usages to reference the private field and
adjust external code to call getItemTypes() where necessary.
In `@abi/src/main/java/org/tron/trident/abi/FunctionReturnDecoder.java`:
- Around line 75-82: The decodeAddress method currently uses a raw List for
outputParameters (same raw-type problem as decodeDynamicBytes); change the
declaration to a parameterized List<TypeReference<Address>> (e.g.,
List<TypeReference<Address>> outputParameters = new ArrayList<>()) and keep the
TypeReference<Address>(){} element, then call
decoder.decodeFunctionResult(rawInput, outputParameters) as before so generics
are correct for decodeAddress and related decoding logic.
- Around line 60-65: Add Javadoc for the public method decodeAddress matching
the style and detail of decodeDynamicBytes: describe that it decodes an
ABI-encoded address from rawInput, document the parameters (rawInput) and return
value (String or Address type and null on invalid response), and mention any
exceptions or behavior for invalid input; reference the method name
decodeAddress so reviewers can locate it and ensure consistency with the
existing decodeDynamicBytes Javadoc format.
- Around line 66-73: In decodeDynamicBytes, replace the raw-typed declaration
"List outputParameters = new ArrayList<TypeReference<Type>>()" with a
parameterized type such as "List<TypeReference<Type>> outputParameters = new
ArrayList<>()" so generics are preserved; keep the existing add of new
TypeReference<DynamicBytes>() {} and the subsequent call to
decoder.decodeFunctionResult(rawInput, outputParameters) unchanged.
In `@abi/src/main/java/org/tron/trident/abi/TypeDecoder.java`:
- Around line 766-771: Replace the generic RuntimeException thrown in the
DynamicArray handling branch of TypeDecoder (the block where
DynamicArray.class.isAssignableFrom(declaredField) and parameter == null) with a
more specific exception; change the throw to IllegalArgumentException (or a
custom exception type) and include the same explanatory message so callers can
catch this specific invalid-argument condition when the `@Parameterized`
annotation is missing.
- Around line 500-509: Replace the confusing use of
classType.isAssignableFrom(DynamicStruct.class) and
classType.isAssignableFrom(StaticStruct.class) in TypeDecoder with explicit
equality checks (e.g., classType.equals(DynamicStruct.class) and
classType.equals(StaticStruct.class)) to make the intention clear that only the
exact base types should match; keep the existing reflection fallback
(findStructConstructor, ctor.newInstance) for custom struct subclasses and
preserve casting of parameters to List<Type> for the DynamicStruct/StaticStruct
branches.
In `@abi/src/main/java/org/tron/trident/abi/TypeEncoder.java`:
- Around line 289-297: The code calls encode(type) twice for dynamic members;
capture the result once and reuse it: replace the second encode(type) call with
the local encodedValue variable so dynamicValues.add(encodedValue) is used and
dynamicOffset is incremented from encodedValue.length() (keeping the current
shift/right-by-1 logic). Update the block in TypeEncoder where isDynamic(type)
is handled (references: dynamicOffset, offsetsAndStaticValues, dynamicValues,
encodedValue, encode(type)) to ensure the offset arithmetic and appended payload
use the exact same encoded bytes.
In `@abi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.java`:
- Around line 928-961: The multi-dimensional array fixture declarations
setGetMultiDimStaticArrayFunction and setGetMultiDimDynamicArrayFunction use
unavoidable raw type casts (e.g., (Class)StaticArray2.class and
(Class)DynamicArray.class) that trigger unchecked/rawtypes compiler warnings;
suppress these warnings by annotating the fixture fields with
`@SuppressWarnings`("unchecked") or `@SuppressWarnings`("rawtypes") as appropriate
(place the annotation immediately above the static final field declarations for
setGetMultiDimStaticArrayFunction and setGetMultiDimDynamicArrayFunction) so the
casts remain but the compiler warnings are silenced.
In `@abi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.java`:
- Around line 21-26: Swap assertion argument order and strengthen parameter
equality: change assertEquals(event.getName(), "MyError") to
assertEquals("MyError", event.getName()); replace the manual iterator loop over
event.getParameters() with a direct equality/assertion that verifies both
contents and size (for example assertEquals(parameters, event.getParameters())
or assertEquals(parameters.size(), event.getParameters().size()) followed by
element-by-element comparison) so that extra parameters on event will fail;
update test in CustomErrorTest to use these corrected assertions referencing
event and parameters.
In `@abi/src/test/java/org/tron/trident/abi/datatypes/DynamicArrayTest.java`:
- Around line 38-50: The testMultidimensionalDynamicArray uses raw types for
DynamicArray which causes compiler warnings; update the declarations and
constructors to use fully parameterized nested generics (e.g.,
DynamicArray<DynamicArray<DynamicArray<Uint256>>> and inner
DynamicArray<DynamicArray<Uint256>> / DynamicArray<Uint256>) so the test remains
the same (assertEquals("uint256[][][]", array.getTypeAsString())) but is
type-safe; adjust the DynamicArray instantiations in the test to use these
generic types rather than raw DynamicArray.
In `@core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java`:
- Around line 718-723: The test currently constructs a Function named `function`
but calls `client.triggerConstantContract(...)` with a hardcoded calldata
string; replace that raw hex with the encoded calldata produced from the
constructed Function by calling the encoder (e.g.
FunctionEncoder.encode(function)) and pass that encoded hex string into
`client.triggerConstantContract(testAddress, contractAddress, /* encoded
calldata */)` so the multi-dimensional array encoding path is exercised; keep
the same `testAddress` and `contractAddress` parameters and assert on the
returned `transactionExtention` as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ac664c-4ae0-44c1-a1cd-4072a00bbd04
⛔ Files ignored due to path filters (1)
abi/src/main/java/org/tron/trident/abi/datatypes/generated/StaticArray0.javais excluded by!**/generated/**
📒 Files selected for processing (36)
abi/src/main/java/org/tron/trident/abi/CustomErrorEncoder.javaabi/src/main/java/org/tron/trident/abi/DefaultFunctionEncoder.javaabi/src/main/java/org/tron/trident/abi/DefaultFunctionReturnDecoder.javaabi/src/main/java/org/tron/trident/abi/EventEncoder.javaabi/src/main/java/org/tron/trident/abi/FunctionEncoder.javaabi/src/main/java/org/tron/trident/abi/FunctionReturnDecoder.javaabi/src/main/java/org/tron/trident/abi/TypeDecoder.javaabi/src/main/java/org/tron/trident/abi/TypeEncoder.javaabi/src/main/java/org/tron/trident/abi/TypeReference.javaabi/src/main/java/org/tron/trident/abi/Utils.javaabi/src/main/java/org/tron/trident/abi/datatypes/AbiTypes.javaabi/src/main/java/org/tron/trident/abi/datatypes/Array.javaabi/src/main/java/org/tron/trident/abi/datatypes/BytesType.javaabi/src/main/java/org/tron/trident/abi/datatypes/CustomError.javaabi/src/main/java/org/tron/trident/abi/datatypes/DynamicArray.javaabi/src/main/java/org/tron/trident/abi/datatypes/DynamicStruct.javaabi/src/main/java/org/tron/trident/abi/datatypes/Event.javaabi/src/main/java/org/tron/trident/abi/datatypes/Function.javaabi/src/main/java/org/tron/trident/abi/datatypes/StaticArray.javaabi/src/main/java/org/tron/trident/abi/datatypes/StaticStruct.javaabi/src/main/java/org/tron/trident/abi/datatypes/Utf8String.javaabi/src/main/java/org/tron/trident/abi/datatypes/primitive/Int.javaabi/src/main/java/org/tron/trident/abi/datatypes/reflection/Parameterized.javaabi/src/test/java/org/tron/trident/abi/AbiV2TestFixture.javaabi/src/test/java/org/tron/trident/abi/CustomErrorEncoderTest.javaabi/src/test/java/org/tron/trident/abi/DefaultFunctionEncoderTest.javaabi/src/test/java/org/tron/trident/abi/EventEncoderTest.javaabi/src/test/java/org/tron/trident/abi/FunctionReturnDecoderTest.javaabi/src/test/java/org/tron/trident/abi/TypeDecoderTest.javaabi/src/test/java/org/tron/trident/abi/TypeEncoderPackedTest.javaabi/src/test/java/org/tron/trident/abi/TypeEncoderTest.javaabi/src/test/java/org/tron/trident/abi/datatypes/CustomErrorTest.javaabi/src/test/java/org/tron/trident/abi/datatypes/DynamicArrayTest.javaabi/src/test/java/org/tron/trident/abi/datatypes/StaticArrayTest.javaabi/src/test/java/org/tron/trident/abi/datatypes/StaticStructTest.javacore/src/test/java/org/tron/trident/core/AbiV2ContractTest.java
| /** | ||
| * Returns the Bytes32 Padded length. If the string is empty, we only encode its length. Else, | ||
| * we concatenate its length along of its value | ||
| */ | ||
| @Override | ||
| public int bytes32PaddedLength() { | ||
| return 2 * MAX_BYTE_LENGTH; | ||
| if (value.isEmpty()) { | ||
| return MAX_BYTE_LENGTH; | ||
| } else { | ||
| return 2 * MAX_BYTE_LENGTH; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how bytes32PaddedLength is used to understand its contract
echo "=== Searching for bytes32PaddedLength usage ==="
rg -n "bytes32PaddedLength" --type=java -C3
echo "=== Check Type interface definition ==="
ast-grep --pattern 'interface Type {
$$$
bytes32PaddedLength($$$)
$$$
}'Repository: 0xbigapple/trident
Length of output: 6270
🏁 Script executed:
# Get the full Utf8String class
cat abi/src/main/java/org/tron/trident/abi/datatypes/Utf8String.java
# Also check BytesType for comparison
echo "=== BytesType implementation ==="
cat abi/src/main/java/org/tron/trident/abi/datatypes/BytesType.javaRepository: 0xbigapple/trident
Length of output: 3713
🏁 Script executed:
# Check how dynamic types are identified
rg -n "isDynamic" --type=java -B2 -A2 | head -100Repository: 0xbigapple/trident
Length of output: 7202
🏁 Script executed:
# Look for how Utf8String is encoded/decoded to understand expected behavior
rg -n "class Utf8String" --type=java -A30Repository: 0xbigapple/trident
Length of output: 3637
🏁 Script executed:
# Search for string encoding logic
rg -n "encodeString|encode.*Utf8|Utf8String" --type=java -B2 -A5 | head -80
# Check TypeEncoder.encode method to see how it handles Utf8String
rg -n "public static.*encode" abi/src/main/java/org/tron/trident/abi/TypeEncoder.java -A15Repository: 0xbigapple/trident
Length of output: 9802
🏁 Script executed:
# Get the encodeString method implementation
rg -n "private static.*encodeString|private static String encodeString" abi/src/main/java/org/tron/trident/abi/TypeEncoder.java -A30Repository: 0xbigapple/trident
Length of output: 44
🏁 Script executed:
# Also check if there's a test for string encoding
rg -n "testString|String.*encode|encodeString" abi/src/test/java/org/tron/trident/abi/TypeEncoderTest.java -B2 -A10 | head -100Repository: 0xbigapple/trident
Length of output: 609
🏁 Script executed:
# Search for the actual encodeString method definition in TypeEncoder
rg -n "encodeString" abi/src/main/java/org/tron/trident/abi/TypeEncoder.java -A25
# Let's also check the line numbers mentioned in the test
sed -n '1180,1200p' abi/src/main/java/org/tron/trident/abi/TypeEncoderTest.javaRepository: 0xbigapple/trident
Length of output: 2377
🏁 Script executed:
# Get the encodeDynamicBytes method
rg -n "static.*encodeDynamicBytes|private.*encodeDynamicBytes" abi/src/main/java/org/tron/trident/abi/TypeEncoder.java -A30Repository: 0xbigapple/trident
Length of output: 1240
🏁 Script executed:
# Find where bytes32PaddedLength is actually called in TypeEncoder
rg -n "\.bytes32PaddedLength\(\)" abi/src/main/java/org/tron/trident/abi/TypeEncoder.java -B5 -A5Repository: 0xbigapple/trident
Length of output: 505
Remove or correct bytes32PaddedLength() in Utf8String—it does not reflect actual encoded size and is unused.
The method returns a fixed 64 bytes for all non-empty strings regardless of content length. However, this is misleading because:
Utf8Stringis a dynamic type, sobytes32PaddedLength()is never called during actual encoding (seeTypeEncoder.encodeString()andencodeDynamicBytes())- For static types in structs, only non-dynamic types use
bytes32PaddedLength()(line 281 inTypeEncoder) - The method's Javadoc incorrectly describes the encoding contract for strings
Either remove this override (falling back to the default Type implementation returning MAX_BYTE_LENGTH), or clarify its purpose in the Javadoc if it's meant for a specific use case beyond encoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/src/main/java/org/tron/trident/abi/datatypes/Utf8String.java` around
lines 30 - 41, The bytes32PaddedLength() override in Utf8String is misleading
and unused for dynamic strings; remove the override from class Utf8String so the
class falls back to the default Type.bytes32PaddedLength() behavior, or if you
intend a special-purpose behavior keep the method but update its Javadoc to
clearly state it is not used for ABI dynamic encoding and return a value that
correctly documents that special use; locate the method by name
(bytes32PaddedLength) in Utf8String and consider callers like
TypeEncoder.encodeString() and encodeDynamicBytes() and the base Type
implementation when making the change.
| value = | ||
| (T) | ||
| TypeDecoder.decodeStaticArray( | ||
| input, currOffset, staticReference, staticLength); | ||
| currOffset += | ||
| ((decodeUintAsInt(input, currOffset) / Type.MAX_BYTE_LENGTH) + 2) | ||
| * MAX_BYTE_LENGTH_FOR_HEX_STRING; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tests that exercise nested static array decoding
rg -n "StaticArray.*StaticArray" --type=java -A5 -B5Repository: 0xbigapple/trident
Length of output: 50374
🏁 Script executed:
sed -n '858,1026p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 7888
🏁 Script executed:
rg -n "StaticArray.*Uint" abi/src/test/java/org/tron/trident/abi/TypeDecoderTest.java -A3 | grep -E "(new Uint|StaticArray)" | head -30Repository: 0xbigapple/trident
Length of output: 843
🏁 Script executed:
rg -n "bytes3d\|bytes2d" abi/src/test/java/org/tron/trident/abi/TypeDecoderTest.java -B5 -A10 | head -80Repository: 0xbigapple/trident
Length of output: 44
🏁 Script executed:
sed -n '1200,1250p' abi/src/test/java/org/tron/trident/abi/TypeDecoderTest.javaRepository: 0xbigapple/trident
Length of output: 2552
🏁 Script executed:
sed -n '1646,1665p' abi/src/test/java/org/tron/trident/abi/FunctionReturnDecoderTest.javaRepository: 0xbigapple/trident
Length of output: 1004
🏁 Script executed:
sed -n '85,140p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 2776
🏁 Script executed:
sed -n '916,960p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 2166
🏁 Script executed:
sed -n '927,975p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 2396
🏁 Script executed:
sed -n '960,975p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 720
🏁 Script executed:
sed -n '988,998p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 575
🏁 Script executed:
rg -n "public static.*decodeStaticArray" abi/src/main/java/org/tron/trident/abi/TypeDecoder.java -A5Repository: 0xbigapple/trident
Length of output: 44
🏁 Script executed:
rg -n "decodeStaticArray" abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | head -5Repository: 0xbigapple/trident
Length of output: 236
🏁 Script executed:
sed -n '356,430p' abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | cat -nRepository: 0xbigapple/trident
Length of output: 3483
🏁 Script executed:
rg -n "MAX_BYTE_LENGTH\s*=" abi/src/main/java/org/tron/trident/abi/TypeDecoder.javaRepository: 0xbigapple/trident
Length of output: 44
🏁 Script executed:
rg -n "MAX_BYTE_LENGTH" abi/src/main/java/org/tron/trident/abi/datatypes/Type.javaRepository: 0xbigapple/trident
Length of output: 139
🏁 Script executed:
sed -n '15,30p' abi/src/main/java/org/tron/trident/abi/datatypes/Type.javaRepository: 0xbigapple/trident
Length of output: 301
Incorrect offset calculation for nested static arrays treats them as dynamic types.
At lines 993-995, the offset calculation ((decodeUintAsInt(input, currOffset) / Type.MAX_BYTE_LENGTH) + 2) attempts to read and decode a length prefix from the input data. However, static arrays in ABI encoding do not have a length prefix—their size is known at compile time (already extracted as staticLength on lines 931-932). This logic is correct for dynamic arrays but incorrect for static arrays.
The calculation will produce correct results when test data contains small values (as in existing tests with values 1-6), but will fail with larger values coincidentally placed at those encoding positions. The offset should instead be calculated from staticLength and the actual element size.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/src/main/java/org/tron/trident/abi/TypeDecoder.java` around lines 988 -
995, The offset after decoding a nested static array is being computed as if the
array were dynamic by reading a length prefix via decodeUintAsInt at currOffset;
instead, update the logic in TypeDecoder after the call to
TypeDecoder.decodeStaticArray to advance currOffset based on the known
staticLength and the element size (use staticLength and Type.MAX_BYTE_LENGTH /
element byte size logic) rather than reading a length from the input with
decodeUintAsInt; ensure you use MAX_BYTE_LENGTH_FOR_HEX_STRING and
Type.MAX_BYTE_LENGTH consistently to compute the exact number of hex chars
consumed for staticLength * elementSize so currOffset points to the next ABI
value.
| static boolean isDynamic(Type parameter) { | ||
| return parameter instanceof DynamicBytes | ||
| || parameter instanceof Utf8String | ||
| || parameter instanceof DynamicArray; | ||
| || parameter instanceof DynamicArray | ||
| || (parameter instanceof StaticArray | ||
| && DynamicStruct.class.isAssignableFrom( | ||
| ((StaticArray) parameter).getComponentType())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "TypeEncoder.java" -type fRepository: 0xbigapple/trident
Length of output: 119
🏁 Script executed:
# Get the file and examine the structure
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs wc -lRepository: 0xbigapple/trident
Length of output: 123
🏁 Script executed:
# Read the isDynamic method (lines 53-60)
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '50,65p'Repository: 0xbigapple/trident
Length of output: 617
🏁 Script executed:
# Read lines 76-82
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '70,90p'Repository: 0xbigapple/trident
Length of output: 1088
🏁 Script executed:
# Read lines 337-368 (encodeArrayValuesOffsets method context)
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '330,380p'Repository: 0xbigapple/trident
Length of output: 2545
🏁 Script executed:
# Also get broader context - look at encode method
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '1,100p'Repository: 0xbigapple/trident
Length of output: 4215
🏁 Script executed:
# Read encodeArrayValues method
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '100,150p'Repository: 0xbigapple/trident
Length of output: 2341
🏁 Script executed:
# Search for encodeArrayValues method definition
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs grep -n "encodeArrayValues" | head -10Repository: 0xbigapple/trident
Length of output: 469
🏁 Script executed:
# Get encodeStaticArrayWithDynamicStruct method
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '280,330p'Repository: 0xbigapple/trident
Length of output: 2102
🏁 Script executed:
# Read more context around line 289 mentioned in original comment
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '285,310p'Repository: 0xbigapple/trident
Length of output: 1127
🏁 Script executed:
# Read the full encodeArrayValues method
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '258,275p'Repository: 0xbigapple/trident
Length of output: 642
🏁 Script executed:
# Check if there are any tests that cover these cases
find . -path "*test*" -name "*TypeEncoder*" -type f | head -5Repository: 0xbigapple/trident
Length of output: 191
🏁 Script executed:
# Check the test file for coverage of StaticArray with dynamic elements
find . -path "*test*" -name "TypeEncoderTest.java" -type f | head -1 | xargs grep -n "StaticArray\|Utf8String\|DynamicBytes" | head -20Repository: 0xbigapple/trident
Length of output: 1106
🏁 Script executed:
# Look for any test of string arrays or bytes arrays
find . -path "*test*" -name "TypeEncoderTest.java" -type f | head -1 | xargs grep -A 5 -B 2 "string\[" | head -30Repository: 0xbigapple/trident
Length of output: 44
🏁 Script executed:
# Look for tests of StaticArray with specific types
find . -path "*test*" -name "TypeEncoderTest.java" -type f | head -1 | xargs grep -n "StaticArray2\|StaticArray3" | head -20Repository: 0xbigapple/trident
Length of output: 360
🏁 Script executed:
# Get more context - check what the test does with StaticArray2 and Utf8String
find . -path "*test*" -name "TypeEncoderTest.java" -type f | head -1 | xargs sed -n '1190,1230p'Repository: 0xbigapple/trident
Length of output: 1524
🏁 Script executed:
# Search for any test combining StaticArray with dynamic types
find . -path "*test*" -name "TypeEncoderTest.java" -type f | head -1 | xargs grep -B 5 -A 15 "StaticArray.*Utf8String\|StaticArray.*DynamicBytes"Repository: 0xbigapple/trident
Length of output: 46
🏁 Script executed:
# Search more carefully for StaticArray tests with dynamic content
find . -path "*test*" -name "TypeEncoderTest.java" -type f | head -1 | xargs sed -n '1226,1350p'Repository: 0xbigapple/trident
Length of output: 6806
🏁 Script executed:
# Let's understand the actual behavior - create a test case to verify the issue
# First, let's check what methods are available on StaticArray
find . -path "*datatypes*" -name "StaticArray.java" -type fRepository: 0xbigapple/trident
Length of output: 129
🏁 Script executed:
# Let's verify the issue by examining what happens with a StaticArray of Utf8String
# Check the encode() method again to see how it routes StaticArray
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '76,82p'Repository: 0xbigapple/trident
Length of output: 381
🏁 Script executed:
# Now let's look at encodeStaticArrayWithDynamicStruct to understand the correct pattern
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs grep -A 30 "encodeStaticArrayWithDynamicStruct"Repository: 0xbigapple/trident
Length of output: 2433
🏁 Script executed:
# Let's check the Solidity ABI specification requirements by looking at comments in the code
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '320,340p'Repository: 0xbigapple/trident
Length of output: 1199
🏁 Script executed:
# Check if isDynamic is used to determine encoding behavior elsewhere
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs grep -n "isDynamic"Repository: 0xbigapple/trident
Length of output: 175
🏁 Script executed:
# Now let's test the actual issue - create a simple verification with what we know
# Search for any handling of StaticArray with Utf8String or DynamicBytes
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs grep -B 5 -A 10 "StaticArray\|encodeArrayValues\|isDynamic" | head -50Repository: 0xbigapple/trident
Length of output: 2089
🏁 Script executed:
# Look at the full encodeDynamicsTypesArraysOffsets method
find . -path "*abi*" -name "TypeEncoder.java" -type f | head -1 | xargs sed -n '369,410p'Repository: 0xbigapple/trident
Length of output: 1841
Generalize dynamic-array detection beyond DynamicStruct.
StaticArray is treated as dynamic only when its component type is DynamicStruct, causing string[2], bytes[2], uint256[][2], and similar arrays with dynamic element types to be encoded inline without offsets. This produces invalid ABI encoding. The offset-handling infrastructure (encodeDynamicsTypesArraysOffsets) is designed to handle any dynamic element type but is only applied to DynamicStruct arrays. Extend isDynamic() to return true for StaticArray with any dynamic component type (Utf8String, DynamicBytes, DynamicArray), and update the encode() routing logic accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/src/main/java/org/tron/trident/abi/TypeEncoder.java` around lines 53 -
60, The isDynamic(Type) method currently only treats a StaticArray as dynamic
when its component type is DynamicStruct; change it to detect any dynamic
component (DynamicBytes, Utf8String, DynamicArray, or any type that itself
returns true from isDynamic) so StaticArray with dynamic elements returns true;
then update the encode() routing so that arrays of those StaticArray types go
through encodeDynamicsTypesArraysOffsets (or the same dynamic-array offset
handling path) instead of inline encoding. Reference: isDynamic,
StaticArray.getComponentType(), DynamicBytes, Utf8String, DynamicArray,
DynamicStruct, encodeDynamicsTypesArraysOffsets, and encode().
| public static Constructor findStructConstructor(Class classType) { | ||
| return Arrays.stream(classType.getDeclaredConstructors()) | ||
| .filter( | ||
| declaredConstructor -> | ||
| Arrays.stream(declaredConstructor.getParameterTypes()) | ||
| .allMatch(Type.class::isAssignableFrom)) | ||
| .findAny() | ||
| .orElseThrow( | ||
| () -> | ||
| new RuntimeException( | ||
| "TypeReferenced struct must contain " | ||
| + "a constructor with types that extend Type")); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the Utils.java file
find . -type f -name "Utils.java" | grep -E "abi.*src.*main"Repository: 0xbigapple/trident
Length of output: 113
🏁 Script executed:
# Also search for the test file mentioned
find . -type f -name "AbiV2ContractTest.java"Repository: 0xbigapple/trident
Length of output: 127
🏁 Script executed:
# Search for getStructType method to understand the dependency
rg "getStructType" --type java -A 5Repository: 0xbigapple/trident
Length of output: 2845
🏁 Script executed:
# Check the actual findStructConstructor implementation and its usage
rg "findStructConstructor" --type java -B 2 -A 10Repository: 0xbigapple/trident
Length of output: 7701
🏁 Script executed:
# Examine the test file to see the constructor patterns mentioned
sed -n '458,525p' ./core/src/test/java/org/tron/trident/core/AbiV2ContractTest.javaRepository: 0xbigapple/trident
Length of output: 2275
🏁 Script executed:
# Get more context around the test classes if needed
sed -n '400,600p' ./core/src/test/java/org/tron/trident/core/AbiV2ContractTest.java | head -200Repository: 0xbigapple/trident
Length of output: 22194
🏁 Script executed:
# Check if there are any sorting or ordering mechanisms in findStructConstructor
cat -n ./abi/src/main/java/org/tron/trident/abi/Utils.java | sed -n '110,150p'Repository: 0xbigapple/trident
Length of output: 1679
🏁 Script executed:
# Check the entire getStructType method
cat -n ./abi/src/main/java/org/tron/trident/abi/Utils.java | sed -n '100,180p'Repository: 0xbigapple/trident
Length of output: 3577
🏁 Script executed:
# Check if there are any other usages of findStructConstructor that might shed light on the issue
rg "getDeclaredConstructors" --type java -B 3 -A 3Repository: 0xbigapple/trident
Length of output: 678
🏁 Script executed:
# Look at TypeDecoder usage of findStructConstructor to confirm the impact
sed -n '1,100p' ./abi/src/main/java/org/tron/trident/abi/TypeDecoder.java | head -50Repository: 0xbigapple/trident
Length of output: 2232
Prefer the most specific constructor matching the filter.
findAny() returns whichever matching constructor Java reflection provides first, which is undefined. The test classes (StaticInfo, DynamicInfo, NestedInfo) each define both a typed constructor (e.g., (Uint256, Address)) and a generic bridge ((Type, Type, ...)). If the bridge wins, getStructType() extracts Type parameters and generates malformed ABI tuples like (type,...) instead of canonical signatures. Select the constructor with the smallest parameter count or fewest raw Type.class parameters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/src/main/java/org/tron/trident/abi/Utils.java` around lines 124 - 136,
findStructConstructor currently uses Arrays.stream(...).findAny() which can pick
an arbitrary matching constructor; instead select the most specific constructor
(prefer the one with the smallest parameter count or the one with the fewest
parameters equal to raw Type.class) so typed constructors win over the generic
bridge. Modify findStructConstructor to stream declared constructors, filter by
allMatch(Type.class::isAssignableFrom) as before, then sort or reduce to pick
the constructor with minimal parameter length (and as a tiebreaker minimal count
of parameters exactly equal to Type.class) and return that or throw the same
RuntimeException if none found.
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 arrays (including nested), packed encoding, and custom errors. Also fixes decoding/padding bugs and normalizes event/error signature hashes (no 0x).
New Features
StaticStruct/DynamicStructand static/dynamic arrays, including nested/multi-dimensional and arrays of structs.encodePackedsupport andFunctionEncoder.encodeWithSelector.CustomErrorandCustomErrorEncoder(signature hashing).TypeReferencesupports inner types; added@datatypes.reflection.Parameterizedfor nested generics.AbiTypes/Utils:getTypeAStringfor structs/arrays; load custom types viaClass.forName; support empty static arrays (StaticArray0) and improved type strings for arrays/structs.Bug Fixes
BytesandUtf8Stringpadded length.DynamicStruct.getTypeAsStringfor arrays/nested types.EventEncoder.buildEventSignaturenow returns a hash without the0xprefix.Written for commit 1997f15. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements