fix low-level connectiontolightclientmapping calculation in unit tests #213
Conversation
WalkthroughThe changes involve modifications to two contracts: Changes
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
🧹 Outside diff range and nitpick comments (3)
test/Dispatcher/Dispatcher.multiclient.sol (2)
92-93: Approve: Good addition of pre-condition check.The added assertion improves the test by ensuring the initial state is as expected before proceeding with the connection removal. This helps to catch potential issues early and makes the test more robust.
Consider adding a comment explaining the purpose of this assertion, e.g.:
+ // Verify the initial state: ensure the connection exists and is mapped to the correct light client assertEq(_getConnectiontoClientIdMapping(connectionHops1[0]), address(opLightClient));
104-104: Approve: Improved assertion for connection removal.The modified assertion correctly checks if the connection has been removed by verifying that the mapping now returns
address(0). This change aligns with the expected behavior and provides a clear verification of the connection removal.Consider adding a comment to explain the expected outcome:
+ // Verify the connection has been removed: mapping should return address(0) assertEq(_getConnectiontoClientIdMapping(connectionHops1[0]), address(0));test/utils/Dispatcher.base.t.sol (1)
282-285: Addviewmodifier to_getConnectiontoClientIdMappingfunctionThe function only reads from contract storage and does not modify any state. Adding the
viewmodifier makes the function's intent clear and can lead to gas optimizations.Apply this change to add the
viewmodifier:-function _getConnectiontoClientIdMapping(string memory connection) internal returns (address clientId) { +function _getConnectiontoClientIdMapping(string memory connection) internal view returns (address clientId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- test/Dispatcher/Dispatcher.multiclient.sol (2 hunks)
- test/utils/Dispatcher.base.t.sol (2 hunks)
🔇 Additional comments (1)
test/Dispatcher/Dispatcher.multiclient.sol (1)
Line range hint
92-104: Overall improvement in test reliability.The changes in this test function enhance its reliability by verifying both the initial state (connection exists) and the final state (connection removed) of the
removeConnectionoperation. This aligns well with the PR objectives of fixing a low-level calculation and implementing tests.To ensure consistency across the codebase, let's verify if similar improvements have been made in other test files:
This will help us identify if similar improvements have been made in other test files or if we need to consider applying this pattern more broadly.
✅ Verification successful
Verification Successful: Test Enhancements Applied Correctly.
The updated assertions in
test/Dispatcher/Dispatcher.multiclient.soleffectively ensure the reliability of theremoveConnectionoperation by verifying both the initial and final states. No similar assertions were found in other Solidity test files, indicating that the changes are appropriately localized.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar assertions in other test files # Test: Search for similar assertions in other test files rg --type solidity -g 'test/**/*.sol' -e 'assertEq.*getConnectiontoClientIdMapping.*address\(0\)' -e 'assertEq.*getConnectiontoClientIdMapping.*opLightClient'Length of output: 193
Script:
#!/bin/bash # Description: Check for similar assertions in other Solidity test files # Search for assertions that verify connection mappings to address(0) or opLightClient rg -g "*.sol" -e 'assertEq\s*\(\s*_getConnectiontoClientIdMapping\s*\(.*\),\s*address\(0\)\s*\)' -e 'assertEq\s*\(\s*_getConnectiontoClientIdMapping\s*\(.*\),\s*address\(opLightClient\)\s*\)'Length of output: 457
RnkSngh
force-pushed
the
raunak/fix-connection-to-client-calculation
branch
2 times, most recently
from
b64605c
to
4883b75
Compare
RnkSngh
force-pushed
the
raunak/fix-connection-to-client-calculation
branch
2 times, most recently
from
ee3e789
to
e00dda2
Compare
RnkSngh
changed the title
RnkSngh
force-pushed
the
raunak/fix-connection-to-client-calculation
branch
from
e00dda2
to
2768074
Compare
thanks to @nicopernas for finding out that the low-level caluclation was incorrect in our test suite.
This pr fixes this, and also now actually tests that this calculation is working
Summary by CodeRabbit
Bug Fixes
New Features
Chores