Socket test #186
Socket test #186
Conversation
WalkthroughThis update introduces extensive new unit tests for the Socket protocol, adds new mock contracts for ERC721, fee management, plugs, and targets, and enhances the MockFastSwitchboard. RescueFundsLib is upgraded to support ERC721 rescue. Utility and application test files are refactored for clarity. Documentation and comments are standardized across several protocol contracts. Changes
Sequence Diagram(s)sequenceDiagram
participant TestContract
participant Socket
participant MockPlug
participant MockSwitchboard
participant MockFeeManager
participant MockERC721
TestContract->>Socket: execute(params, transmission)
Socket->>MockFeeManager: payAndCheckFees(params, transmission)
Socket->>MockSwitchboard: getTransmitter(...)
Socket->>MockPlug: processPayload(...)
alt Rescue ERC721
TestContract->>Socket: rescueFunds(MockERC721)
Socket->>MockERC721: safeTransferFrom(contract, recipient, tokenId, data)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
contracts/utils/RescueFundsLib.sol (2)
8-19: Consider using standard interface imports.These interfaces should use OpenZeppelin's standard implementations instead of custom definitions to ensure compatibility and reduce maintenance overhead.
-interface IERC721 { - function safeTransferFrom( - address from, - address to, - uint256 tokenId, - bytes calldata data - ) external; -} - -interface IERC165 { - function supportsInterface(bytes4 interfaceId) external view returns (bool); -} +import "@openzeppelin/contracts/interfaces/IERC721.sol"; +import "@openzeppelin/contracts/interfaces/IERC165.sol";
40-49: Enhance documentation for dual-purpose amount parameter.The amount_ parameter serves as both token amount (for fungibles) and tokenId (for NFTs). This should be clearly documented in the function's NatSpec.
/** * @dev Rescues funds from a contract. * @param token_ The address of the token contract. * @param rescueTo_ The address of the user. - * @param amount_ The amount of tokens to be rescued. + * @param amount_ The amount of tokens to be rescued (for ERC20) or tokenId (for ERC721). */test/mock/MockPlug.sol (1)
64-67: Replace magic number with named constant.The 3072 bytes size should be documented and made a constant to clarify its purpose for testing maxCopyBytes truncation.
+ uint256 private constant LARGE_DATA_SIZE = 3072; // 3KB to test maxCopyBytes truncation (maxCopyBytes is 2KB) + function returnLargeData() external pure returns (bytes memory) { - // Return 3KB of data to test maxCopyBytes truncation (maxCopyBytes is 2KB) - return new bytes(3072); + return new bytes(LARGE_DATA_SIZE); }test/Utils.t.sol (1)
71-149: Thorough converter tests with good error handling.Tests cover all important cases including zero addresses, invalid addresses, and round-trip conversions. The wrapper function for testing reverts is well-implemented.
One minor issue: the fuzz test assumption on line 142 is redundant since
addris already of typeaddress.- vm.assume(uint256(uint160(addr)) <= type(uint160).max); + // No assumption needed - addr is already a valid addresstest/Socket.t.sol (4)
374-374: Remove console.log statement.This should be removed or wrapped in a conditional flag for production tests.
- console.log("gasUsed", gasUsed); + // console.log("gasUsed", gasUsed); // Uncomment for debugging
434-434: Remove console.log statement.This should be removed or wrapped in a conditional flag for production tests.
- console.log("gasUsed", gasUsed); + // console.log("gasUsed", gasUsed); // Uncomment for debugging
799-799: Hardcoded storage slot is fragile.This assumes counter is at slot 57. If the contract layout changes, this test will break. Consider exposing a test helper function to set the counter instead.
Add a setter function in SocketTestWrapper:
function setCounterForTesting(uint64 _counter) external { counter = _counter; }
925-925: Remove console.log statement.This should be removed or wrapped in a conditional flag for production tests.
- console.log("Gas used for execution with fees:", gasUsed); + // console.log("Gas used for execution with fees:", gasUsed); // Uncomment for debugging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
contracts/protocol/Socket.sol(1 hunks)contracts/protocol/SocketConfig.sol(2 hunks)contracts/protocol/SocketUtils.sol(0 hunks)contracts/utils/RescueFundsLib.sol(2 hunks)test/SetupTest.t.sol(1 hunks)test/Socket.t.sol(1 hunks)test/Utils.t.sol(1 hunks)test/apps/ParallelCounter.t.sol(2 hunks)test/mock/MockERC721.sol(1 hunks)test/mock/MockFastSwitchboard.sol(2 hunks)test/mock/MockFeesManager.sol(1 hunks)test/mock/MockPlug.sol(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/protocol/SocketUtils.sol
🔇 Additional comments (28)
test/apps/ParallelCounter.t.sol (1)
40-42: Good refactoring to avoid stack too deep.Breaking down the complex assertion logic into helper functions effectively resolves the stack depth issue while maintaining test clarity.
contracts/utils/RescueFundsLib.sol (1)
41-49: Solid ERC721 rescue implementation.The ERC165 interface detection with proper fallback handling correctly identifies and transfers NFTs while maintaining backward compatibility for fungible tokens.
test/mock/MockFeesManager.sol (1)
1-44: Well-designed mock for testing fee manager interactions.This mock effectively tracks fee manager calls and provides predictable return values for testing. The reset functionality and parameter tracking support comprehensive test scenarios.
test/mock/MockPlug.sol (1)
11-75: Comprehensive mock plug implementation for testing.MockPlug provides excellent test coverage capabilities with proper IPlug implementation, socket interaction helpers, and edge case testing functions. The design supports thorough validation of socket protocol behavior.
contracts/protocol/Socket.sol (1)
103-103: Clean comment repositioning maintains code clarity.The comment about the first untrusted call in the system remains accurate and appropriately positioned for the switchboard call.
contracts/protocol/SocketConfig.sol (3)
82-83: Documentation improvement looks good.The NatSpec comments properly describe the function's purpose and access control requirements.
97-100: Documentation improvement looks good.The NatSpec comments clearly describe the function's purpose and parameters.
112-113: Documentation improvement looks good.The NatSpec comments clearly describe the function's purpose and access control.
test/mock/MockERC721.sol (5)
1-12: Mock contract structure is well-designed.Uses Solady's gas-optimized ERC721 implementation and maintains clean state organization.
13-19: Metadata functions implemented correctly.Proper overrides that return the stored name and symbol values.
21-30: Mint functions provide good testing flexibility.Both specific tokenId minting and auto-increment minting are useful for different test scenarios.
32-43: Burn function implements proper access control.Correctly checks owner, individual approval, and approval for all before allowing burn operations.
45-47: TokenURI implementation is appropriate for mock.Returning empty string is sufficient for testing scenarios.
test/SetupTest.t.sol (3)
130-146: Good refactoring for improved modularity.Separating counting and population logic makes the code cleaner and prevents unnecessary calls when no valid plugs exist.
148-159: Clean extraction of counting logic.Single responsibility function that clearly counts valid plugs.
161-184: Well-structured config population function.Good optimization by fetching switchboard ID once before the loop. Clean separation of concerns.
test/Utils.t.sol (1)
8-69: Comprehensive IdUtils tests with good coverage.Tests cover normal cases, edge cases (zero/max values), and includes fuzz testing. Bit packing verification is correctly implemented.
test/mock/MockFastSwitchboard.sol (3)
14-28: Good state additions for testing capabilities.The new boolean flags provide observability and configurability for testing different switchboard scenarios.
30-46: Enhanced functions improve testing capabilities.State tracking in
attestand configurable return value inallowPayloadprovide better control for testing scenarios.
48-65: Function implementations appropriate for mock testing.Minimal implementations provide expected behavior while maintaining interface compatibility.
test/Socket.t.sol (8)
23-51: Test wrapper implementation is correct.The wrapper properly exposes internal functions for testing without modifying their behavior.
58-155: Well-structured test base with proper setup.The base contract provides comprehensive test infrastructure with mock contracts, role assignments, and utility functions for parameter creation and signature generation.
442-471: Connect/disconnect tests provide good coverage.Tests properly validate switchboard validation, new connections, and error cases for unconnected plugs.
477-548: Comprehensive switchboard management tests.Excellent coverage of registration, status transitions, and access control validation.
554-622: Setter tests properly validate access control and configuration updates.Good coverage of all configuration parameters with proper role validation.
628-751: Digest creation tests ensure proper uniqueness.Comprehensive validation of digest generation with different parameters and payload sizes.
816-830: Simulation access control test is correct.Properly validates that only off-chain callers can access simulation functionality.
836-888: Comprehensive rescue functionality tests.Excellent coverage of ETH, ERC20, and ERC721 rescue scenarios with proper access control validation.
| // @notice function to set the socket fee manager | ||
| // @dev only callable by governance role | ||
| // @param socketFeeManager_ address of the socket fee manager | ||
| function setSocketFeeManager(address socketFeeManager_) external onlyRole(GOVERNANCE_ROLE) { | ||
| emit SocketFeeManagerUpdated(address(socketFeeManager), socketFeeManager_); | ||
| socketFeeManager = ISocketFeeManager(socketFeeManager_); | ||
| emit SocketFeeManagerUpdated(address(socketFeeManager), socketFeeManager_); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix event parameter order.
The documentation improvements are good, but the event emission has incorrect parameter order. socketFeeManager is the new value, not the old one.
- emit SocketFeeManagerUpdated(address(socketFeeManager), socketFeeManager_);
+ emit SocketFeeManagerUpdated(address(socketFeeManager), address(socketFeeManager_));Wait, this needs verification of the event signature first.
🏁 Script executed:
#!/bin/bash
# Check the SocketFeeManagerUpdated event signature to verify parameter order
rg -A 3 "event SocketFeeManagerUpdated"Length of output: 371
Fix event emission in setSocketFeeManager to use old and new manager addresses correctly
The current implementation emits the updated address for both oldSocketFeeManager and newSocketFeeManager. You must capture the old value before overwriting state and then emit the event with the correct parameters.
• File: contracts/protocol/SocketConfig.sol
• Location: setSocketFeeManager (around lines 89–94)
@@ contracts/protocol/SocketConfig.sol:89-94
- function setSocketFeeManager(address socketFeeManager_) external onlyRole(GOVERNANCE_ROLE) {
- socketFeeManager = ISocketFeeManager(socketFeeManager_);
- emit SocketFeeManagerUpdated(address(socketFeeManager), socketFeeManager_);
+ function setSocketFeeManager(address socketFeeManager_) external onlyRole(GOVERNANCE_ROLE) {
+ address oldSocketFeeManager = address(socketFeeManager);
+ socketFeeManager = ISocketFeeManager(socketFeeManager_);
+ emit SocketFeeManagerUpdated(oldSocketFeeManager, socketFeeManager_);This should ensure oldSocketFeeManager reflects the previous manager and newSocketFeeManager the newly assigned one.
📝 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.
| // @notice function to set the socket fee manager | |
| // @dev only callable by governance role | |
| // @param socketFeeManager_ address of the socket fee manager | |
| function setSocketFeeManager(address socketFeeManager_) external onlyRole(GOVERNANCE_ROLE) { | |
| emit SocketFeeManagerUpdated(address(socketFeeManager), socketFeeManager_); | |
| socketFeeManager = ISocketFeeManager(socketFeeManager_); | |
| emit SocketFeeManagerUpdated(address(socketFeeManager), socketFeeManager_); | |
| // @notice function to set the socket fee manager | |
| // @dev only callable by governance role | |
| // @param socketFeeManager_ address of the socket fee manager | |
| function setSocketFeeManager(address socketFeeManager_) external onlyRole(GOVERNANCE_ROLE) { | |
| // capture the previous manager before updating | |
| address oldSocketFeeManager = address(socketFeeManager); | |
| socketFeeManager = ISocketFeeManager(socketFeeManager_); | |
| emit SocketFeeManagerUpdated(oldSocketFeeManager, socketFeeManager_); | |
| } |
🤖 Prompt for AI Agents
In contracts/protocol/SocketConfig.sol around lines 89 to 94, the
setSocketFeeManager function emits the SocketFeeManagerUpdated event using the
updated socketFeeManager address for both old and new values. To fix this, store
the current socketFeeManager address in a local variable before updating it,
then emit the event with the old address as the first argument and the new
address as the second argument.
| function _assertForwarderAddresses( | ||
| address forwarderArb1, | ||
| bytes32 onChainArb1, | ||
| uint32 arbChainSlug, | ||
| address forwarderOpt1, | ||
| bytes32 onChainOpt1, | ||
| uint32 optChainSlug, | ||
| address forwarderArb2, | ||
| bytes32 onChainArb2, | ||
| uint32 arbChainSlug2, | ||
| address forwarderOpt2, | ||
| bytes32 onChainOpt2, | ||
| uint32 optChainSlug2 | ||
| ) internal view { | ||
| _assertForwarderAddress(forwarderArb1, onChainArb1, arbChainSlug); | ||
| _assertForwarderAddress(forwarderOpt1, onChainOpt1, optChainSlug); | ||
| _assertForwarderAddress(forwarderArb2, onChainArb2, arbChainSlug2); | ||
| _assertForwarderAddress(forwarderOpt2, onChainOpt2, optChainSlug2); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify parameter list using struct or better organization.
This function has 12 parameters with confusing duplicate chainSlug values (arbChainSlug vs arbChainSlug2). This should use a struct or be reorganized to reduce complexity and improve readability.
+struct ForwarderAssertionData {
+ address forwarder;
+ bytes32 onChainAddress;
+ uint32 chainSlug;
+}
+
-function _assertForwarderAddresses(
- address forwarderArb1,
- bytes32 onChainArb1,
- uint32 arbChainSlug,
- address forwarderOpt1,
- bytes32 onChainOpt1,
- uint32 optChainSlug,
- address forwarderArb2,
- bytes32 onChainArb2,
- uint32 arbChainSlug2,
- address forwarderOpt2,
- bytes32 onChainOpt2,
- uint32 optChainSlug2
-) internal view {
+function _assertForwarderAddresses(ForwarderAssertionData[4] memory assertions) internal view {
+ for (uint256 i = 0; i < assertions.length; i++) {
+ _assertForwarderAddress(
+ assertions[i].forwarder,
+ assertions[i].onChainAddress,
+ assertions[i].chainSlug
+ );
+ }📝 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.
| function _assertForwarderAddresses( | |
| address forwarderArb1, | |
| bytes32 onChainArb1, | |
| uint32 arbChainSlug, | |
| address forwarderOpt1, | |
| bytes32 onChainOpt1, | |
| uint32 optChainSlug, | |
| address forwarderArb2, | |
| bytes32 onChainArb2, | |
| uint32 arbChainSlug2, | |
| address forwarderOpt2, | |
| bytes32 onChainOpt2, | |
| uint32 optChainSlug2 | |
| ) internal view { | |
| _assertForwarderAddress(forwarderArb1, onChainArb1, arbChainSlug); | |
| _assertForwarderAddress(forwarderOpt1, onChainOpt1, optChainSlug); | |
| _assertForwarderAddress(forwarderArb2, onChainArb2, arbChainSlug2); | |
| _assertForwarderAddress(forwarderOpt2, onChainOpt2, optChainSlug2); | |
| } | |
| // Add a struct to encapsulate forwarder assertion data | |
| struct ForwarderAssertionData { | |
| address forwarder; | |
| bytes32 onChainAddress; | |
| uint32 chainSlug; | |
| } | |
| function _assertForwarderAddresses(ForwarderAssertionData[4] memory assertions) internal view { | |
| for (uint256 i = 0; i < assertions.length; i++) { | |
| _assertForwarderAddress( | |
| assertions[i].forwarder, | |
| assertions[i].onChainAddress, | |
| assertions[i].chainSlug | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In test/apps/ParallelCounter.t.sol around lines 85 to 103, the function
_assertForwarderAddresses has an overly long parameter list with 12 parameters,
including confusing duplicate chainSlug values. Refactor this by defining a
struct to group related forwarder address data and chain slugs together, then
update the function to accept fewer, more organized parameters using these
structs. This will simplify the function signature and improve readability.
| function increment() external payable { | ||
| if (shouldRevert) { | ||
| revert(); | ||
| } | ||
| counter++; | ||
| lastCallData = msg.data; | ||
| lastValue = msg.value; | ||
| emit Called(msg.sender, msg.value, msg.data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix redundant msg.data assignment in increment function.
The increment() function assigns msg.data to lastCallData, but for a function call, this will always be the 4-byte function selector. This should capture the actual call parameters or be removed if not needed.
function increment() external payable {
if (shouldRevert) {
revert();
}
counter++;
- lastCallData = msg.data;
lastValue = msg.value;
emit Called(msg.sender, msg.value, msg.data);
}📝 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.
| function increment() external payable { | |
| if (shouldRevert) { | |
| revert(); | |
| } | |
| counter++; | |
| lastCallData = msg.data; | |
| lastValue = msg.value; | |
| emit Called(msg.sender, msg.value, msg.data); | |
| } | |
| function increment() external payable { | |
| if (shouldRevert) { | |
| revert(); | |
| } | |
| counter++; | |
| lastValue = msg.value; | |
| emit Called(msg.sender, msg.value, msg.data); | |
| } |
🤖 Prompt for AI Agents
In test/mock/MockPlug.sol around lines 89 to 97, the increment function assigns
msg.data to lastCallData, but msg.data only contains the 4-byte function
selector for this call, which is redundant. Remove the assignment to
lastCallData or modify it to capture meaningful call parameters if needed;
otherwise, simply delete the line assigning msg.data to lastCallData.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/Socket.t.sol (2)
916-916: This should remove commented-out code.Commented-out code should be removed to keep tests clean. If this functionality is needed, implement it properly or remove it entirely.
- // mockSwitchboard.setTransmitter(transmitter); - uint256 gasBefore = gasleft();
938-938: This should remove commented-out code.Remove this commented line to maintain code cleanliness.
- // mockSwitchboard.setTransmitter(transmitter); - // Should still execute successfully without fee manager
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/protocol/Socket.sol(3 hunks)test/Socket.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/protocol/Socket.sol
| uint256 counterSlot = uint256(57); | ||
| vm.store(address(socketWrapper), bytes32(counterSlot), bytes32(uint256(counter))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
This should avoid hardcoding storage slot 57.
Directly manipulating storage slot 57 assumes a specific storage layout that could change if the Socket contract is modified. This makes the test fragile.
Consider exposing a test-only setter function in SocketTestWrapper to set the counter value, or use a more robust method to determine the storage slot dynamically.
🤖 Prompt for AI Agents
In test/Socket.t.sol around lines 798 to 799, avoid hardcoding storage slot 57
when setting the counter value as it makes the test fragile. Instead, add a
test-only setter function in the SocketTestWrapper contract that allows setting
the counter directly. Then update the test to call this setter function rather
than manipulating the storage slot manually.
| vm.expectRevert("Socket does not accept ETH"); | ||
| (bool success, ) = address(socket).call{value: 1 ether}(""); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
This should use a custom error selector instead of string comparison.
Testing for specific revert messages via string comparison is fragile and gas-inefficient. The Socket contract should define a custom error like error EthNotAccepted() and this test should expect that selector.
🤖 Prompt for AI Agents
In test/Socket.t.sol around lines 871 to 873, the test currently expects a
revert using a string message which is fragile and inefficient. Modify the
Socket contract to define a custom error such as `error EthNotAccepted()`, then
update the test to expect this custom error selector instead of the string
revert message. Use vm.expectRevert with the encoded selector of the custom
error to make the test more robust and gas-efficient.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores