Skip to content

Conversation

@nomo-app
Copy link
Owner

@nomo-app nomo-app commented Jan 23, 2025

Summary by CodeRabbit

  • Improvements
    • Enhanced serialization and deserialization processes for function parameters.
    • Added JSON conversion methods for various function parameter types, including bytes, fixed, and integer types.
    • Improved type safety during data handling, ensuring correct processing of values according to their specific types.

@coderabbitai
Copy link

coderabbitai bot commented Jan 23, 2025

Walkthrough

The changes focus on enhancing the serialization and deserialization processes for function parameters in an EVM (Ethereum Virtual Machine) contract interaction library. The modifications introduce type-specific JSON conversion methods in the FunctionParamType class and update how values are processed during JSON conversion in the FunctionParamWithValue class. Additional classes related to parameter types also receive similar enhancements. These changes aim to improve type safety and correctness when handling contract function parameters.

Changes

File Change Summary
lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart Added methods for JSON serialization and deserialization: valueFromJson and valueToJson in FunctionParamType and its derived classes.
lib/src/crypto/evm/entities/contract/contract_function_param.dart Updated toJson() and fromJson() methods in FunctionParamWithValue to utilize type-specific JSON conversion methods.
lib/src/crypto/evm/entities/contract/parameter_type/bytes_parameter_type.dart Introduced valueToJson and valueFromJson methods in FunctionParamBytes and FunctionParamBytesBase for handling Uint8List.
lib/src/crypto/evm/entities/contract/parameter_type/fixed_parameter_type.dart Added valueToJson and valueFromJson methods in FunctionParamFixedBase, FunctionParamFixed, and FunctionParamUFixed for Amount objects.
lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart Implemented valueToJson and valueFromJson methods in FunctionParamInt for BigInt values.
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart Removed import statement for erc1155.dart, indicating potential refactoring of ERC1155 related functionalities.
test/ci/evm/parsing/fetch_function_signature_test.dart Enhanced tests for JSON serialization and deserialization of contract function data, ensuring data integrity.

Possibly related PRs

  • refactor: simplify TronTransaction raw transaction handling and remov… #126: The changes in the FunctionParamWithValue class in the main PR involve serialization and deserialization of the value property, which is relevant to the modifications in the TronTransaction class that also involve handling JSON input and output, indicating a connection in how transaction parameters are processed.
  • Add Hex Normalization to prevent leading Zeros in RLP #135: The changes in the main PR regarding the serialization and deserialization methods for the value property in FunctionParamWithValue relate to the modifications in RLP encoding and decoding processes, which also involve handling data representation and integrity, highlighting a shared focus on data handling improvements.

Suggested reviewers

  • ThomasFercher

Poem

🐰 Hop, hop, through the crypto maze,
JSON values now dance with grace,
Types converting left and right,
Ethereum's functions shine so bright!
A rabbit's code, precise and neat, 🌟
Serialization now complete! 🚀


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e2eb00 and c521186.

📒 Files selected for processing (2)
  • lib/src/crypto/evm/entities/contract/contract_function_param.dart (1 hunks)
  • lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart (1 hunks)
🔇 Additional comments (1)
lib/src/crypto/evm/entities/contract/contract_function_param.dart (1)

80-80: 🛠️ Refactor suggestion

Consider adding type constraints and error handling.

While using type.toJsonValue(value) improves type safety, there are a few concerns:

  1. The generic type parameter T is not constrained to match the parameter type's internal type
  2. There's no explicit error handling for invalid values

Consider adding type constraints and error handling:

-class FunctionParamWithValue<T> extends FunctionParam {
+class FunctionParamWithValue<T extends Object?> extends FunctionParam {
   final T value;

   const FunctionParamWithValue({
     required this.value,
     required super.name,
     required super.type,
     super.indexed,
-  });
+  }) {
+    if (value != null && value.runtimeType != type.internalType) {
+      throw ArgumentError('Value type ${value.runtimeType} does not match parameter type ${type.internalType}');
+    }
+  }

   Json toJson() {
     return {
       'name': name,
       'indexed': indexed,
       'type': type.name,
-      'value': type.toJsonValue(value),
+      'value': value == null ? null : type.toJsonValue(value),
     };
   }

Comment on lines 85 to 90
final type = FunctionParamType.fromString(map['type']);
return FunctionParamWithValue(
value: map['value'],
value: type.fromJsonValue(map['value']),
name: map['name'],
indexed: map['indexed'] as bool? ?? false,
type: FunctionParamType.fromString(map['type']),
type: type,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for JSON deserialization.

The current implementation assumes the JSON data is valid and correctly typed. Consider adding validation and error handling.

Consider this safer implementation:

   factory FunctionParamWithValue.fromJson(Map map) {
+    if (!map.containsKey('type')) {
+      throw FormatException('Missing required field: type');
+    }
     final type = FunctionParamType.fromString(map['type']);
+    
+    if (!map.containsKey('value')) {
+      throw FormatException('Missing required field: value');
+    }
+    
+    final dynamic jsonValue = map['value'];
+    final value = jsonValue == null ? null : type.fromJsonValue(jsonValue);
+
     return FunctionParamWithValue(
-      value: type.fromJsonValue(map['value']),
+      value: value,
       name: map['name'],
       indexed: map['indexed'] as bool? ?? false,
       type: type,
     );
   }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 165 to 172
dynamic toJsonValue<T>(T value) {
return value;
}

T fromJsonValue(dynamic value) {
return value as T;
}

Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

The review comment is correct and the suggested implementation is necessary.

The current implementation lacks proper type-specific JSON conversion, which could lead to runtime errors. This is evidenced by:

  1. The codebase uses BigInt for all integer types and Uint8List for bytes, but these types need special handling for JSON serialization.
  2. Each parameter type already has proper encoding/decoding implementations for ABI encoding, but lacks equivalent handling for JSON serialization.
  3. The suggested implementation correctly handles all parameter types and aligns with the existing type system.
🔗 Analysis chain

Implement type-specific JSON conversion for different parameter types.

The current implementation simply passes through values without proper type conversion. This could lead to runtime errors when dealing with complex types like BigInt, Uint8List, etc.

Consider implementing type-specific JSON conversion:

-  dynamic toJsonValue<T>(T value) {
-    return value;
-  }
-
-  T fromJsonValue(dynamic value) {
-    return value as T;
-  }
+  /// Converts a value to its JSON representation based on the parameter type.
+  /// 
+  /// This method ensures that complex types like BigInt, Uint8List, etc. are
+  /// properly serialized to JSON-compatible values.
+  dynamic toJsonValue<T>(T value) => switch (this) {
+    FunctionParamInt() => (value as BigInt).toString(),
+    FunctionParamBytes() => base64Encode(value as Uint8List),
+    FunctionParamAddress() => value as String,
+    FunctionParamBool() => value as bool,
+    TupleFunctionParamType type => (value as List).asMap().map(
+      (i, v) => MapEntry(i, type.types[i].toJsonValue(v))
+    ).values.toList(),
+    ArrayFunctionParamType type => (value as List)
+      .map((v) => type.itemType.toJsonValue(v))
+      .toList(),
+    _ => value,
+  };
+
+  /// Converts a JSON value back to its original type based on the parameter type.
+  /// 
+  /// This method ensures that JSON values are properly deserialized back to their
+  /// original complex types like BigInt, Uint8List, etc.
+  /// 
+  /// Throws [FormatException] if the JSON value is not in the expected format.
+  T fromJsonValue(dynamic value) => switch (this) {
+    FunctionParamInt() => BigInt.parse(value as String) as T,
+    FunctionParamBytes() => base64Decode(value as String) as T,
+    FunctionParamAddress() when value is! String => 
+      throw FormatException('Expected String for address, got ${value.runtimeType}'),
+    FunctionParamBool() when value is! bool =>
+      throw FormatException('Expected bool, got ${value.runtimeType}'),
+    TupleFunctionParamType type => type.types
+      .asMap()
+      .map((i, t) => MapEntry(i, t.fromJsonValue((value as List)[i])))
+      .values
+      .toList() as T,
+    ArrayFunctionParamType type => (value as List)
+      .map((v) => type.itemType.fromJsonValue(v))
+      .toList() as T,
+    _ => value as T,
+  };

This implementation:

  1. Adds proper type conversion for different parameter types
  2. Includes validation and error handling
  3. Adds documentation
  4. Handles complex types like tuples and arrays

Let's verify the JSON conversion for different types:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of these methods to ensure we handle all parameter types
rg -A 5 "toJsonValue|fromJsonValue"

# Search for any BigInt, Uint8List, or other complex type usage
ast-grep --pattern 'BigInt|Uint8List'

Length of output: 2171


Script:

#!/bin/bash
# Search for parameter type class definitions
ast-grep --pattern 'class $_ extends FunctionParamType'

# Search for type conversion utilities
rg -l "toString|parse|encode|decode" lib/src/crypto/evm/

# Check imports to understand available type utilities
rg "^import.*dart" lib/src/crypto/evm/entities/contract/parameter_type/

Length of output: 2426


Script:

#!/bin/bash
# Check parameter type implementations
cat lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart
cat lib/src/crypto/evm/entities/contract/parameter_type/bytes_parameter_type.dart

# Look for encoding/decoding implementations
rg -A 10 "encode|decode" lib/src/crypto/evm/entities/contract/contract_function_encoding.dart
rg -A 10 "encode|decode" lib/src/crypto/evm/entities/contract/contract_function_decoding.dart

Length of output: 16994

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (7)
lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart (1)

33-40: Enhance error handling in valueFromJson method.

While the implementation is correct, consider improving error handling for better robustness:

 @override
 BigInt valueFromJson(dynamic json) => switch (json) {
+    null => throw FormatException('JSON value cannot be null'),
     String str => BigInt.parse(str),
-    _ => throw Exception('Invalid JSON type'),
+    _ => throw FormatException('Expected String, got ${json.runtimeType}'),
   };

Additionally, consider wrapping BigInt.parse in a try-catch to provide a more descriptive error message when parsing fails:

 @override
 BigInt valueFromJson(dynamic json) => switch (json) {
     null => throw FormatException('JSON value cannot be null'),
-    String str => BigInt.parse(str),
+    String str => () {
+      try {
+        return BigInt.parse(str);
+      } on FormatException {
+        throw FormatException('Invalid BigInt format: $str');
+      }
+    }(),
     _ => throw FormatException('Expected String, got ${json.runtimeType}'),
   };
lib/src/crypto/evm/entities/contract/parameter_type/fixed_parameter_type.dart (2)

19-23: Add null safety and documentation to JSON methods.

The JSON serialization methods could benefit from:

  1. Null safety annotations
  2. Documentation explaining the expected JSON format
  @override
- valueToJson(Amount value) => value.toJson();
+ Amount? valueToJson(Amount? value) => value?.toJson();

  @override
+ /// Creates an [Amount] from its JSON representation.
+ /// 
+ /// The [json] parameter should be in the format expected by [Amount.fromJson].
+ /// Throws a [FormatException] if the JSON is invalid.
- Amount valueFromJson(dynamic json) => Amount.fromJson(json);
+ Amount? valueFromJson(dynamic json) => json == null ? null : Amount.fromJson(json);

Line range hint 11-17: Add documentation for unimplemented methods.

The encode and decode methods are marked as UnimplementedError but lack documentation explaining why they're not implemented and whether they should be implemented by subclasses.

+ /// Encodes the given [Amount] value into bytes.
+ /// 
+ /// This method must be implemented by subclasses to provide
+ /// type-specific encoding logic.
  @override
  Uint8List encode(Amount value) {
    throw UnimplementedError();
  }

+ /// Decodes the given bytes into an [Amount] value.
+ /// 
+ /// This method must be implemented by subclasses to provide
+ /// type-specific decoding logic.
  @override
  Amount decode(Uint8List data) {
    throw UnimplementedError();
  }
test/ci/evm/parsing/fetch_function_signature_test.dart (3)

55-60: LGTM! Consider adding a test for pretty-printed JSON.

The JSON serialization test is well structured. However, it might be helpful to add an additional test case with pretty-printed JSON for better readability during debugging.

 final jsonString = jsonEncode(json);
+final prettyJsonString = JsonEncoder.withIndent('  ').convert(json);
 
 expect(jsonString,
     '{"name":"execute","parameters":[{"name":null,"indexed":null,"type":"bytes","value":"0b08"},{"name":null,"indexed":null,"type":"bytes[]","value":["0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000016345785d8a0000","0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000016345785d8a00000000000000000000000000000000000000000000000018c5c71dd6f8c06a772000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000004e9fcd48af4738e3bf1382009dc1e93ebfce698f"]},{"name":null,"indexed":null,"type":"uint256","value":"1701042887"}]}');
+// Add test for pretty-printed JSON
+expect(prettyJsonString.contains('  "name": "execute"'), isTrue);

62-73: LGTM! Consider adding round-trip equality test.

The JSON deserialization test is comprehensive. To make it more robust, consider adding an equality check between the original and decoded objects.

 final decoded = ContractFunctionWithValues.fromJson(json);
+
+// Verify round-trip equality
+expect(decoded, equals(contractFunction));
 
 expect(decoded.name, "execute");
 expect(decoded.parameters.first.type, FunctionParamBytes());

This would require implementing operator == and hashCode in ContractFunctionWithValues if not already present.


358-466: Refactor complex type tests for better maintainability.

The test for complex types is comprehensive but could be more maintainable. Consider extracting helper methods and using test data builders.

+// Add helper method for complex parameter assertions
+void assertComplexParameter(
+  dynamic actual,
+  dynamic expected,
+  String message, {
+  bool ignoreCase = true,
+}) {
+  if (actual is List && expected is List) {
+    expect(actual.length, expected.length, reason: '$message: list length mismatch');
+    for (var i = 0; i < actual.length; i++) {
+      assertComplexParameter(actual[i], expected[i], '$message[$i]');
+    }
+  } else if (actual is String && expected is String && ignoreCase) {
+    expect(actual.toLowerCase(), expected.toLowerCase(), reason: message);
+  } else {
+    expect(actual, expected, reason: message);
+  }
+}
+
 final json = contractFunction.toJson();
 final decoded = ContractFunctionWithValues.fromJson(json);
 
-expect(decoded.name, "settle");
-expect(decoded.parameters[0].type, FunctionParamAddressArray());
-expect(decoded.parameters[0].value, [
+// Use helper method for assertions
+assertComplexParameter(decoded.name, "settle", "Function name");
+assertComplexParameter(decoded.parameters[0].type, FunctionParamAddressArray(), "Parameter 0 type");
+assertComplexParameter(decoded.parameters[0].value, [
   "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48".toLowerCase(),
   "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE".toLowerCase(),
   "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48".toLowerCase(),
   "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE".toLowerCase(),
-]);
+], "Parameter 0 value");
lib/src/crypto/evm/entities/contract/parameter_type/bytes_parameter_type.dart (1)

24-31: Enhance error handling and type safety in JSON conversion methods.

While the implementation is functional, it could benefit from more robust error handling:

  1. The error message should be more descriptive about expected types
  2. The hex string format should be validated before conversion
  3. Null safety should be explicitly handled

Consider this improved implementation:

  @override
  valueToJson(Uint8List value) => value.toHex;

  @override
  Uint8List valueFromJson(dynamic json) => switch (json) {
+       null => throw FormatException('Expected hex string, got null'),
-       String str => str.hexToBytes,
+       String str when str.startsWith('0x') || RegExp(r'^[0-9a-fA-F]*$').hasMatch(str) => 
+         str.hexToBytes,
+       String str => throw FormatException(
+         'Invalid hex string format. Expected hex string starting with "0x" or containing only hex characters, got: $str'),
-       _ => throw Exception('Invalid JSON type'),
+       _ => throw FormatException(
+         'Invalid JSON type. Expected hex string, got ${json.runtimeType}'),
      };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c521186 and 2596237.

📒 Files selected for processing (7)
  • lib/src/crypto/evm/entities/contract/contract_function_param.dart (1 hunks)
  • lib/src/crypto/evm/entities/contract/parameter_type/bytes_parameter_type.dart (2 hunks)
  • lib/src/crypto/evm/entities/contract/parameter_type/fixed_parameter_type.dart (3 hunks)
  • lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart (8 hunks)
  • lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart (1 hunks)
  • lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (0 hunks)
  • test/ci/evm/parsing/fetch_function_signature_test.dart (3 hunks)
💤 Files with no reviewable changes (1)
  • lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/src/crypto/evm/entities/contract/contract_function_param.dart
  • lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test/ci/proof_of_payment tests
  • GitHub Check: test/ci/fetching tests

Comment on lines +40 to +44
@override
valueToJson(Amount value) => value.toJson();

@override
Amount valueFromJson(dynamic json) => Amount.fromJson(json);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove duplicate JSON methods from FunctionParamFixed.

The JSON serialization methods are identical to those in FunctionParamFixedBase. Since this class already extends BaseFunctionParamType<Amount>, consider extending FunctionParamFixedBase instead to inherit the JSON methods.

- final class FunctionParamFixed extends BaseFunctionParamType<Amount> {
+ final class FunctionParamFixed extends FunctionParamFixedBase {
-   const FunctionParamFixed() : super('fixed');
+   const FunctionParamFixed() : super('fixed', 18);

   @override
   Uint8List encode(Amount value) {
     return FunctionParamFixed128(18).encode(value);
   }

   @override
   Amount decode(Uint8List data) {
     return FunctionParamFixed128(18).decode(data);
   }
-
-  @override
-  valueToJson(Amount value) => value.toJson();
-
-  @override
-  Amount valueFromJson(dynamic json) => Amount.fromJson(json);
 }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +61 to +65
@override
valueToJson(Amount value) => value.toJson();

@override
Amount valueFromJson(dynamic json) => Amount.fromJson(json);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove duplicate JSON methods from FunctionParamUFixed.

Similar to FunctionParamFixed, these JSON methods duplicate the implementation from FunctionParamFixedBase. Consider extending FunctionParamFixedBase to inherit the JSON methods.

- final class FunctionParamUFixed extends BaseFunctionParamType<Amount> {
+ final class FunctionParamUFixed extends FunctionParamFixedBase {
-   const FunctionParamUFixed() : super('ufixed');
+   const FunctionParamUFixed() : super('ufixed', 18);

   @override
   Uint8List encode(Amount value) {
     return FunctionParamUFixed128(18).encode(value);
   }

   @override
   Amount decode(Uint8List data) {
     return FunctionParamUFixed128(18).decode(data);
   }
-
-  @override
-  valueToJson(Amount value) => value.toJson();
-
-  @override
-  Amount valueFromJson(dynamic json) => Amount.fromJson(json);
 }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +52 to +59
@override
valueToJson(Uint8List value) => value.toHex;

@override
Uint8List valueFromJson(dynamic json) => switch (json) {
String str => str.hexToBytes,
_ => throw Exception('Invalid JSON type'),
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add size validation and consider reducing code duplication.

Two improvements are suggested:

  1. The valueFromJson method should validate the byte length matches the expected size, similar to the assertion in encode().
  2. The identical JSON conversion logic in both classes suggests an opportunity for code reuse.

Consider these improvements:

+ mixin BytesJsonConverter {
+   valueToJson(Uint8List value) => value.toHex;
+
+   Uint8List valueFromJson(dynamic json) => switch (json) {
+     null => throw FormatException('Expected hex string, got null'),
+     String str when str.startsWith('0x') || RegExp(r'^[0-9a-fA-F]*$').hasMatch(str) => 
+       str.hexToBytes,
+     String str => throw FormatException(
+       'Invalid hex string format. Expected hex string starting with "0x" or containing only hex characters, got: $str'),
+     _ => throw FormatException(
+       'Invalid JSON type. Expected hex string, got ${json.runtimeType}'),
+   };
+ }
+
- abstract class FunctionParamBytesBase extends BaseFunctionParamType<Uint8List> {
+ abstract class FunctionParamBytesBase extends BaseFunctionParamType<Uint8List>
+     with BytesJsonConverter {
   final int size;

   const FunctionParamBytesBase(super.name, this.size);

   @override
   valueToJson(Uint8List value) => value.toHex;

   @override
-  Uint8List valueFromJson(dynamic json) => switch (json) {
-    String str => str.hexToBytes,
-    _ => throw Exception('Invalid JSON type'),
-  };
+  Uint8List valueFromJson(dynamic json) {
+    final bytes = super.valueFromJson(json);
+    if (bytes.length != size) {
+      throw FormatException(
+        'Invalid byte length. Expected $size bytes, got ${bytes.length}');
+    }
+    return bytes;
+  }

This refactoring:

  1. Extracts common JSON conversion logic into a mixin
  2. Adds size validation in valueFromJson
  3. Improves error messages
  4. Maintains consistent validation across encode and valueFromJson

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +24 to +31
@override
valueToJson(Uint8List value) => value.toHex;

@override
Uint8List valueFromJson(dynamic json) => switch (json) {
String str => str.hexToBytes,
_ => throw Exception('Invalid JSON type'),
};
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Add tests for error cases and improve documentation

The JSON conversion functionality has good test coverage for the happy path, but needs:

  1. Additional test cases:

    • Invalid hex strings (malformed, odd length)
    • Null or wrong type inputs
    • Size validation for fixed-size variants
  2. Documentation improvements:

    • Add doc comments explaining expected formats
    • Document thrown exceptions
    • Include usage examples
🔗 Analysis chain

Add tests and documentation for JSON conversion methods.

The new JSON conversion functionality requires:

  1. Comprehensive test coverage:

    • Test valid hex strings with/without '0x' prefix
    • Test invalid inputs (null, malformed hex, wrong types)
    • Test size validation for fixed-size variants
    • Test edge cases (empty string, max size)
  2. Documentation:

    • Add doc comments explaining the expected format
    • Document thrown exceptions
    • Provide usage examples

Let's verify the current test coverage:

Also applies to: 52-59

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for relevant test files and their content
echo "Searching for test files..."
fd -e dart test | xargs rg -l "FunctionParamBytes"

echo "Searching for JSON conversion tests..."
fd -e dart test | xargs rg "valueToJson|valueFromJson"

Length of output: 369


Script:

#!/bin/bash
# Check content of identified test files
echo "=== Content of reverse-hash-computation_test.dart ==="
cat test/ci/evm/parsing/reverse-hash-computation_test.dart

echo -e "\n=== Content of fetch_function_signature_test.dart ==="
cat test/ci/evm/parsing/fetch_function_signature_test.dart

# Search for broader test patterns related to bytes conversion
echo -e "\n=== Searching for hex/bytes conversion tests ==="
rg -l "toHex|hexToBytes" test/

# Search for parameter type tests more broadly
echo -e "\n=== Searching for parameter type tests ==="
rg -l "ParameterType|parameter_type" test/

Length of output: 48050


Script:

#!/bin/bash
# Check content of parameter test file
echo "=== Content of parameter_test.dart ==="
cat test/ci/tron/parameter_test.dart

# Search for specific bytes parameter type tests
echo -e "\n=== Searching for bytes parameter type tests ==="
rg -A 5 "FunctionParamBytes|BytesParameterType" test/

Length of output: 10078

@dev2-nomo dev2-nomo merged commit c73c481 into main Jan 27, 2025
13 checks passed
@dev2-nomo dev2-nomo deleted the function_param_value_json_encoding branch January 27, 2025 18:01
dev2-nomo added a commit that referenced this pull request Jan 30, 2025
* feat: add ERC1155 support with balance fetching and transfer function… (#133)

* feat: add ERC1155 support with balance fetching and transfer functionality

* feat: add ERC1155 export to EVM module

* refactor: rename 'account' parameter to 'address' for consistency in ERC1155 balance functions

* feat: add isErc1155 function to check ERC1155 token support and include optional token ID in TokenInfo

* feat: add ERC1155 transaction fetching and balance handling in EVM module

* fix: update tokenValue parsing to default to zero instead of -1 in EtherscanTransaction

* Add Hex Normalization to prevent leading Zeros in RLP (#135)

* Add Hex Normalization to prevent leading Zeros in RLP

* Start of Reimplementing RLP & Testing

* Implement all Test Cases and Decoding

* Finish RawEvmTx Encoding and Decode and fix lint

* Refactor Buffer to int/BigInt and vice versa

* Refactor BigInt Conversion

* Refactor RLP encoding to use sublist for length extraction

* Remove unused import

* Fix Access List Encoding & add isHex Parameter to RLPString to indicate hex

* Add RLP test directory to GitHub Actions workflow

* Add decodeRLPCheck function for input validation and refactor length extraction (check for leading 0)

---------

Co-authored-by: dev2 <[email protected]>
Co-authored-by: thomas.fercher <[email protected]>

* FunctionParamWithValue JsonEncoding (#136)

* initial

* feat: implement JSON serialization and deserialization for function parameter types

* Fix Lint

---------

Co-authored-by: dev2 <[email protected]>

* fix: update toJson method for EvmType2GasPrice to use proper serialization (#138)

* Initial

* Update PipeTests

* Fix Tests

* Fix Lint

* Update GitHub Actions to use upload-artifact@v4

* Update Tests

* Fix Lints

---------

Co-authored-by: The Nomo App's core developer team <[email protected]>
Co-authored-by: thomas.fercher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants