fix: comment out invalid RLP encoding check and add hex string parsin… #142
Conversation
…g test for RawEvmTransaction
|
Warning Rate limit exceeded@dev2-nomo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request modifies the error handling logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant R as RawEvmTransaction
participant D as RLP Decoder
T->>R: fromUnsignedHex(msgHex)
R->>D: decodeRLP(encodedData)
D-->>R: decodedData (no exception on single-byte check)
R-->>T: Transaction Object (nonce=81, value=0)
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/src/crypto/evm/utils/rlp.dart (1)
187-291: Add documentation for RLP decoding edge cases.The
decodeRLPfunction handles various RLP encoding cases but would benefit from documentation of:
- Expected behavior for single bytes less than 0x80
- Edge cases that differ from the RLP specification
- Examples of valid and invalid encodings
Consider adding documentation like:
(RLPItem, int) decodeRLP( Uint8List input, { int offset = 0, }) { + // RLP Decoding Rules: + // 1. For single byte values: + // - Values < 0x80 should not be prefixed in standard RLP + // - This implementation accepts prefixed single bytes for compatibility + // 2. For strings: + // - 0-55 bytes: prefix with 0x80 + length + // - >55 bytes: prefix with 0xb7 + length of length + length + // 3. For lists: + // - 0-55 bytes: prefix with 0xc0 + length + // - >55 bytes: prefix with 0xf7 + length of length + length final bytes = input.buffer.asByteData(); // ... rest of the implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/evm/utils/rlp.dart(1 hunks)test/ci/evm/parsing/parse_hex_transaction_test.dart(1 hunks)
🔇 Additional comments (1)
lib/src/crypto/evm/utils/rlp.dart (1)
281-285: Document rationale for commenting out RLP validation.The commented-out code was enforcing a rule from the RLP specification that single bytes less than 0x80 should not be prefixed. Removing this validation makes the decoder more permissive but could lead to inconsistencies with other implementations.
Please:
- Add a comment explaining why this validation was removed
- Verify compatibility with other EVM implementations using this script:
Additionally, consider searching the web for RLP implementation details:
✅ Verification successful
Action Required: Document the Rationale for Relaxed RLP Validation
The removal of the check for “if (length == 1 && result[0] < 0x80)” was an intentional relaxation of strict RLP validation. Earlier implementations enforced that single-byte values below 0x80 must be encoded as themselves (i.e. without a prefix). However, in some contexts—particularly when dealing with legacy data or accommodating non-canonical encodings—this strict rule can hinder interoperability. Research into how various Ethereum clients handle this case confirms that while the specification mandates unprefixed single-byte encoding, some environments have historically tolerated or even produced prefixed forms (as seen in discussions around EthereumJS’s handling of zero-byte inputs).
Recommended Actions:
Document in Code:
Add a comment near lines 281–285 inlib/src/crypto/evm/utils/rlp.dartexplaining that the validation for prefixed single-byte values was removed to allow for non-canonical variants encountered in the field. Mention the potential risk of inconsistency with clients that strictly enforce the specification.Verify Compatibility:
Although our initial automated search for similar validations in other EVM libraries was unsuccessful due to file type filtering issues, web research confirms that major clients uniformly encode single-byte values (i.e. bytes below 0x80 are normally unprefixed). This strongly suggests that our relaxed decoder should interoperate correctly with other implementations. Nonetheless, consider adding or updating tests to verify that the decoder correctly handles both canonical and the occasionally encountered non-canonical prefixed forms.These precautions and added documentation will ensure that future maintainers are aware of the rationale behind the change and the potential interoperability implications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for similar RLP implementations to compare validation approaches # Search for RLP implementations in common EVM libraries rg -A 10 "RLP.*decode" --type rust --type go --type python # Search for specific validation of single byte values rg -A 5 "0x80.*prefix" --type rust --type go --type pythonLength of output: 183
Web query:
How do different Ethereum clients handle RLP encoding of single bytes less than 0x80?Length of output: 8799
| test('Parse hex string to RawEvmTransaction', () { | ||
| const msgHex = | ||
| "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; | ||
|
|
||
| final tx = RawEvmTransaction.fromUnsignedHex(msgHex); | ||
|
|
||
| expect(tx.nonce, BigInt.from(81)); | ||
| expect(tx.value, BigInt.zero); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for transaction parsing.
The test only verifies nonce and value fields, but the hex string contains additional fields that should be validated:
- Gas price (
3b9aca00) - Gas limit (
0124f8) - To address (
e9e7CEA3DedcA5984780Bafc599bD69ADd087D56) - Data (appears to be a token transfer with method ID
a9059cbb)
Consider expanding the test to verify all transaction fields:
test('Parse hex string to RawEvmTransaction', () {
const msgHex =
"f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080";
final tx = RawEvmTransaction.fromUnsignedHex(msgHex);
expect(tx.nonce, BigInt.from(81));
+ expect(tx.gasPrice, BigInt.from(1000000000)); // 1 Gwei
+ expect(tx.gasLimit, BigInt.from(75000));
+ expect(tx.to, "0xe9e7CEA3DedcA5984780Bafc599bD69ADd087D56");
expect(tx.value, BigInt.zero);
+ expect(tx.data.length, 68); // 4 bytes method ID + 32 bytes address + 32 bytes amount
+ expect(tx.data.sublist(0, 4).toHex, "a9059cbb"); // transfer method ID
+ expect(tx.data.sublist(16, 36).toHex.toLowerCase(), "a7fa4bb0bba164f999e8c7b83c9da96a3be44616"); // recipient address
+ expect(BigInt.parse(tx.data.sublist(36, 68).toHex, radix: 16), BigInt.from(10000)); // transfer amount
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('Parse hex string to RawEvmTransaction', () { | |
| const msgHex = | |
| "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; | |
| final tx = RawEvmTransaction.fromUnsignedHex(msgHex); | |
| expect(tx.nonce, BigInt.from(81)); | |
| expect(tx.value, BigInt.zero); | |
| }); | |
| test('Parse hex string to RawEvmTransaction', () { | |
| const msgHex = | |
| "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; | |
| final tx = RawEvmTransaction.fromUnsignedHex(msgHex); | |
| expect(tx.nonce, BigInt.from(81)); | |
| expect(tx.gasPrice, BigInt.from(1000000000)); // 1 Gwei | |
| expect(tx.gasLimit, BigInt.from(75000)); | |
| expect(tx.to, "0xe9e7CEA3DedcA5984780Bafc599bD69ADd087D56"); | |
| expect(tx.value, BigInt.zero); | |
| expect(tx.data.length, 68); // 4 bytes method ID + 32 bytes address + 32 bytes amount | |
| expect(tx.data.sublist(0, 4).toHex, "a9059cbb"); // transfer method ID | |
| expect(tx.data.sublist(16, 36).toHex.toLowerCase(), "a7fa4bb0bba164f999e8c7b83c9da96a3be44616"); // recipient address | |
| expect(BigInt.parse(tx.data.sublist(36, 68).toHex, radix: 16), BigInt.from(10000)); // transfer amount | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/crypto/evm/utils/rlp.dart (1)
281-285: Add test coverage for RLP validation changes.To ensure the modified RLP validation behavior is working as expected, consider adding test cases that cover:
- Single bytes < 0x80 at root level (should fail)
- Single bytes < 0x80 at non-root level (should pass)
- Various nested RLP structures with raw bytes
Would you like me to help generate comprehensive test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/crypto/evm/utils/rlp.dart(1 hunks)
🔇 Additional comments (1)
lib/src/crypto/evm/utils/rlp.dart (1)
281-285: Verify if relaxing RLP validation aligns with the specification.The change to only validate single-byte RLP encoding at the root level (offset == 0) could have security implications. While this may fix immediate parsing issues, it's important to ensure this aligns with the RLP specification.
Let's verify this against the RLP specification and common implementations:
Consider adding a comment explaining why this validation is only needed at the root level. For example:
+ // Only validate single byte RLP encoding at root level to allow nested items + // that may contain raw bytes in their payload if (offset == 0 && length == 1 && result[0] < 0x80) {
…g test for RawEvmTransaction
Summary by CodeRabbit