Conversation
WalkthroughIntroduces new Solidity test suites and test cases for multiple contracts and libraries: PeripheryRegistryFacet (access control validation), ExcessivelySafeCall (utility functions with data capping), LibBytes (byte manipulation), LibUtil (revert message handling and address checks), and OFTComposeMsgCodec (encoding/decoding). Also extends existing LibUtil tests with additional coverage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Coverage ReportLine Coverage: 87.91% (3013 / 3427 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/solidity/Libraries/LibBytes.t.sol`:
- Around line 1-3: Remove the blank line between the SPDX license identifier and
the pragma declaration in the test/solidity/Libraries/LibBytes.t.sol header so
the SPDX line is immediately followed by the pragma solidity ^0.8.17; statement;
simply delete the empty line after the SPDX comment to comply with the coding
guideline.
In `@test/solidity/Libraries/OFTComposeMsgCodec.t.sol`:
- Around line 1-3: Remove the blank line separating the SPDX license comment and
the pragma statement so the file starts with the SPDX comment immediately
followed by the pragma directive; locate the top of
test/solidity/Libraries/OFTComposeMsgCodec.t.sol where the lines "//
SPDX-License-Identifier: LGPL-3.0-only" and "pragma solidity ^0.8.17;" are and
put them adjacent with no empty line between.
🧹 Nitpick comments (3)
test/solidity/Helpers/ExcessivelySafeCall.t.sol (1)
4-6: Use consistent import remapping.Line 5 uses the
lifi/remapping while line 6 usessrc/directly. For consistency, prefer thelifi/remapping throughout.Suggested fix
import { Test } from "forge-std/Test.sol"; import { ExcessivelySafeCall } from "lifi/Helpers/ExcessivelySafeCall.sol"; -import { InvalidCallData } from "src/Errors/GenericErrors.sol"; +import { InvalidCallData } from "lifi/Errors/GenericErrors.sol";As per coding guidelines: Use remappings defined in
remappings.txtfor imports:lifi/→src/.test/solidity/Facets/PeripheryRegistryFacet.t.sol (1)
39-52: LGTM!The success test properly verifies both event emission and storage update. The structure follows the setup → execute → assert pattern with proper use of
vm.expectEmit.Optional: Consider adding test for non-existent registry lookup.
For more comprehensive coverage, you could add a test verifying the behavior when calling
getPeripheryContractwith a name that hasn't been registered.🧪 Optional test for non-existent periphery lookup
function test_getPeripheryContract_ReturnsZeroForUnregistered() public { assertEq(registry.getPeripheryContract("NonExistent"), address(0)); }test/solidity/Libraries/OFTComposeMsgCodec.t.sol (1)
66-74: Prefer camelCase for local variables (msg_→encodedMsg).♻️ Proposed refactor
- bytes memory msg_ = harness.encode(nonce, srcEid, amountLD, compose); + bytes memory encodedMsg = harness.encode(nonce, srcEid, amountLD, compose); - assertEq(harness.nonce(msg_), nonce); - assertEq(harness.srcEid(msg_), srcEid); - assertEq(harness.amountLD(msg_), amountLD); - assertEq(harness.composeFrom(msg_), composeFrom); + assertEq(harness.nonce(encodedMsg), nonce); + assertEq(harness.srcEid(encodedMsg), srcEid); + assertEq(harness.amountLD(encodedMsg), amountLD); + assertEq(harness.composeFrom(encodedMsg), composeFrom); assertEq( - keccak256(harness.composeMsg(msg_)), + keccak256(harness.composeMsg(encodedMsg)), keccak256(composePayload) );As per coding guidelines: Functions and variables use camelCase; constants and immutables are CONSTANT_CASE.
| // SPDX-License-Identifier: LGPL-3.0-only | ||
| pragma solidity ^0.8.17; | ||
|
|
There was a problem hiding this comment.
Remove the blank line between SPDX and pragma.
SPDX must be immediately followed by the pragma.
🧩 Proposed fix
-// SPDX-License-Identifier: LGPL-3.0-only
-
-pragma solidity ^0.8.17;
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity ^0.8.17;📝 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.
| // SPDX-License-Identifier: LGPL-3.0-only | |
| pragma solidity ^0.8.17; | |
| // SPDX-License-Identifier: LGPL-3.0-only | |
| pragma solidity ^0.8.17; |
🤖 Prompt for AI Agents
In `@test/solidity/Libraries/LibBytes.t.sol` around lines 1 - 3, Remove the blank
line between the SPDX license identifier and the pragma declaration in the
test/solidity/Libraries/LibBytes.t.sol header so the SPDX line is immediately
followed by the pragma solidity ^0.8.17; statement; simply delete the empty line
after the SPDX comment to comply with the coding guideline.
| // SPDX-License-Identifier: LGPL-3.0-only | ||
| pragma solidity ^0.8.17; | ||
|
|
There was a problem hiding this comment.
Remove the blank line between SPDX and pragma.
This file violates the SPDX/pragma adjacency rule.
🛠️ Proposed fix
-// SPDX-License-Identifier: LGPL-3.0-only
-
-pragma solidity ^0.8.17;
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity ^0.8.17;As per coding guidelines: Own files must use // SPDX-License-Identifier: LGPL-3.0-only immediately followed by the pragma statement with no blank line in between.
📝 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.
| // SPDX-License-Identifier: LGPL-3.0-only | |
| pragma solidity ^0.8.17; | |
| // SPDX-License-Identifier: LGPL-3.0-only | |
| pragma solidity ^0.8.17; |
🤖 Prompt for AI Agents
In `@test/solidity/Libraries/OFTComposeMsgCodec.t.sol` around lines 1 - 3, Remove
the blank line separating the SPDX license comment and the pragma statement so
the file starts with the SPDX comment immediately followed by the pragma
directive; locate the top of test/solidity/Libraries/OFTComposeMsgCodec.t.sol
where the lines "// SPDX-License-Identifier: LGPL-3.0-only" and "pragma solidity
^0.8.17;" are and put them adjacent with no empty line between.
2 participants
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)