Rlp fix again #143
Rlp fix again #143
Conversation
WalkthroughThis change introduces a new global boolean variable Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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 (
|
|
Reverted back to old tests and introduced the STRICT_RLP_DECODE Flag which is enabled by default. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/crypto/evm/utils/rlp.dart (1)
6-8: Consider making the variable immutable and scoped.The global mutable state can lead to unpredictable behavior if modified during runtime. Consider:
- Making it
finalto prevent modification after initialization- Moving it into a configuration class to better manage the state
Also, there's a typo in the documentation: "recommned" should be "recommend".
-/// We recommned leaving this enabled unless you have a specific reason to disable it. +/// We recommend leaving this enabled unless you have a specific reason to disable it. -bool STRICT_RLP_DECODE = true; +final bool STRICT_RLP_DECODE = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/evm/utils/rlp.dart(2 hunks)test/ci/rlp/rlp_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test/ci/tron tests
- GitHub Check: test/ci/proof_of_payment tests
- GitHub Check: test/ci/fetching tests
- GitHub Check: test/ci/evm tests
- GitHub Check: test/ci/derivation tests
🔇 Additional comments (2)
lib/src/crypto/evm/utils/rlp.dart (1)
285-285: LGTM! Conditional check for strict RLP decoding.The implementation correctly uses the
STRICT_RLP_DECODEflag to conditionally enforce strict RLP decoding rules.test/ci/rlp/rlp_test.dart (1)
356-372: LGTM! Comprehensive test coverage for invalid RLP encodings.The test cases effectively verify that invalid RLP encodings are properly rejected. The commented explanation for the excluded test case is helpful.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ci/evm/parsing/parse_hex_transaction_test.dart (1)
55-56: Improve test isolation by usingsetUpandtearDown.Modifying a global flag directly in the test could affect other tests if they run in parallel. Consider using
setUpandtearDownto manage the flag state.Apply this diff to improve test isolation:
test('Parse hex string to RawEvmTransaction', () { - STRICT_RLP_DECODE = true; + setUp(() { + STRICT_RLP_DECODE = true; + }); + + tearDown(() { + STRICT_RLP_DECODE = false; + }); + const msgHex = "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; expect( () => RawEvmTransaction.fromUnsignedHex(msgHex), throwsA( isA<RLPException>(), ), ); - STRICT_RLP_DECODE = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/ci/evm/parsing/parse_hex_transaction_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test/ci/tron tests
- GitHub Check: test/ci/proof_of_payment tests
- GitHub Check: test/ci/fetching tests
- GitHub Check: test/ci/evm tests
- GitHub Check: test/ci/derivation tests
| STRICT_RLP_DECODE = true; | ||
| const msgHex = | ||
| "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; | ||
|
|
||
| expect( | ||
| () => RawEvmTransaction.fromUnsignedHex(msgHex), | ||
| throwsA( | ||
| isA<RLPException>(), | ||
| ), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add missing import or declaration for STRICT_RLP_DECODE.
The test uses STRICT_RLP_DECODE without importing or declaring it. This could cause compilation errors.
Add the import statement at the beginning of the file:
import 'package:walletkit_dart/walletkit_dart.dart';
+import 'package:walletkit_dart/src/crypto/evm/utils/rlp.dart' show STRICT_RLP_DECODE;📝 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.
| STRICT_RLP_DECODE = true; | |
| const msgHex = | |
| "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; | |
| expect( | |
| () => RawEvmTransaction.fromUnsignedHex(msgHex), | |
| throwsA( | |
| isA<RLPException>(), | |
| ), | |
| ); | |
| // At the top of test/ci/evm/parsing/parse_hex_transaction_test.dart | |
| import 'package:walletkit_dart/walletkit_dart.dart'; | |
| import 'package:walletkit_dart/src/crypto/evm/utils/rlp.dart' show STRICT_RLP_DECODE; | |
| // ... other imports or file header contents | |
| void main() { | |
| // ... possibly other tests and setup | |
| test('Parsing hex transaction with strict RLP decode enabled', () { | |
| STRICT_RLP_DECODE = true; | |
| const msgHex = | |
| "f86b8151843b9aca00830124f894e9e7CEA3DedcA5984780Bafc599bD69ADd087D5680b844a9059cbb000000000000000000000000a7fa4bb0bba164f999e8c7b83c9da96a3be44616000000000000000000000000000000000000000000000000000000000000271081388080"; | |
| expect( | |
| () => RawEvmTransaction.fromUnsignedHex(msgHex), | |
| throwsA( | |
| isA<RLPException>(), | |
| ), | |
| ); | |
| }); | |
| } |
Summary by CodeRabbit
New Features
Tests