Ds/validate event test #297
Conversation
|
Warning Rate limit exceeded@dshiell has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 30 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 (16)
WalkthroughThe pull request introduces modifications to two key files: Changes
Possibly related PRs
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 (7)
test/proofApi/CrossL2EventProposer.t.sol (6)
9-9: Repeated forge-std/Test.sol import.
It appears that “forge-std/Test.sol” is imported twice (lines 1 and 9). Consider removing the duplicate import if it is not strictly required.
64-71: Commented-out test code for test_clientUpdate_success.
If these tests are still relevant, consider re-enabling and updating them to match the new signature-verification approach. Otherwise, fully remove them to avoid confusion.
87-88: New data variable (cdataI).
Similar to updateD1, ensure this test data aligns with the targeted receipt or event you want to simulate. Documenting the origin or purpose of this hex can aid maintainability.
90-93: Receipt update data (updateD, cdata).
Large hex strings can be difficult to maintain. If feasible, consider programmatically generating or referencing them from test fixtures to reduce the chance of transcription errors.
100-104: Commented-out alternative SequencerSignatureVerifier parameters.
If you no longer need these lines, remove them to keep the test file clear of unused code. Otherwise, consider adding a short comment clarifying why they’re retained.
113-125: Multiple commented-out test blocks.
A large portion of tests are disabled, which may reduce overall coverage. If these tests are obsolete, remove them. Otherwise, consider reworking and re-enabling them to ensure thorough coverage.Would you like help converting these tests to match the new approach?
Also applies to: 127-139, 141-153, 155-165, 167-174, 176-188
contracts/libs/Ibc.sol (1)
342-342: Clarifying comment on unindexed data.
The code comment clarifies that the final bytes segment is simply an ABI-encoded uint64. This is helpful context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/libs/Ibc.sol(2 hunks)test/proofApi/CrossL2EventProposer.t.sol(3 hunks)
🔇 Additional comments (5)
test/proofApi/CrossL2EventProposer.t.sol (4)
7-7: New import seems reasonable.
The direct import of SequencerSignatureVerifier indicates a new dependency for verifying sequencer-signed data. This aligns well with the newly introduced test logic.
84-85: New data variable (updateD1).
The test data is large and opaque, making it difficult to determine correctness by inspection. Confirm that this hex data accurately represents the intended scenario.
95-97: Instantiation of SequencerSignatureVerifier with custom parameters.
This code looks consistent with the overall strategy of verifying block ancestry or signature authenticity from a specified address. Make sure any environment-specific addresses (like 0x11a7...) are set to valid test addresses.
112-112: Empty line addition.
No functional issue; possibly just a spacing change.
contracts/libs/Ibc.sol (1)
308-314: Typed receipt detection in parseLog.
• Good approach to strip the type byte when the receipt is typed.
• Validate that other typed receipt formats won’t require additional processing.
Also applies to: 316-317
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/proofApi/CrossL2EventProposer.t.sol (3)
94-102: Consider parameterizing the SequencerSignatureVerifier configuration.The hardcoded addresses and values in the SequencerSignatureVerifier constructor could make tests brittle and environment-dependent. Consider:
- Moving these values to test configuration files
- Using environment variables
- Creating helper functions to generate test-specific configurations
This will make the tests more maintainable and easier to run in different environments.
- SequencerSignatureVerifier sigV = new SequencerSignatureVerifier( - address(0x11a72195e668328fEe607a309EfA2C42B2893E1d), - bytes32(0x0000000000000000000000000000000000000000000000000000000000000385) - ); + SequencerSignatureVerifier sigV = new SequencerSignatureVerifier( + vm.envAddress("SEQUENCER_ADDRESS"), + bytes32(uint256(vm.envUint("SEQUENCER_CHAIN_ID"))) + );
63-70: Document why the test is commented out.The
test_clientUpdate_successfunction is commented out without explanation. If this is temporary:
- Add a TODO comment explaining why it's disabled
- Create a tracking issue for re-enabling it
- If it's obsolete, consider removing it entirely
Line range hint
111-182: Track TODOs systematically.The skipped tests have well-documented implementation steps, but they should be tracked systematically.
Would you like me to:
- Create GitHub issues for each TODO with the implementation steps?
- Set up a tracking project board for these test implementations?
- Generate a test implementation plan with priorities and dependencies?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/libs/Ibc.sol(2 hunks)test/proofApi/CrossL2EventProposer.t.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/libs/Ibc.sol
Summary by CodeRabbit
New Features
Bug Fixes
Tests