fix connectedchannels on close deletion issue #3
Conversation
WalkthroughThe modification in the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- contracts/CustomChanIbcContract.sol (2 hunks)
Additional comments: 1
contracts/CustomChanIbcContract.sol (1)
- 163-166: The optimization for removing a channel from the
connectedChannelsarray by shifting elements and then popping the last element is correctly implemented. This approach is indeed more gas-efficient than deleting elements directly, as it avoids leaving gaps in the array. However, ensure comprehensive testing is conducted to verify that this logic correctly maintains the integrity of theconnectedChannelsarray under various scenarios, including removing channels from the beginning, middle, and end of the array.
| @@ -30,7 +30,7 @@ contract CustomChanIbcContract is IbcReceiverBase, IbcReceiver { | |||
| function updateDispatcher(IbcDispatcher _dispatcher) external onlyOwner { | |||
| dispatcher = _dispatcher; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The contract lacks event emissions for significant state changes, such as channel creation, reception of packets, acknowledgments, timeouts, and channel closure. Emitting events for these operations would enhance transparency and enable off-chain applications to react to on-chain events more efficiently.
+ event ChannelOpened(bytes32 channelId, bytes32 counterpartyChannelId);
+ event ChannelClosed(bytes32 channelId);
+ event PacketReceived(bytes32 channelId, bytes message);
+ event AcknowledgementSent(bytes32 channelId, bytes ackMessage);
+ event TimeoutOccurred(bytes32 channelId);Add corresponding emit statements in the respective functions where these state changes occur.
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.
| event ChannelOpened(bytes32 channelId, bytes32 counterpartyChannelId); | |
| event ChannelClosed(bytes32 channelId); | |
| event PacketReceived(bytes32 channelId, bytes message); | |
| event AcknowledgementSent(bytes32 channelId, bytes ackMessage); | |
| event TimeoutOccurred(bytes32 channelId); |
The updateDispatcher function allows updating the dispatcher address but lacks validation to ensure the new dispatcher address is not the zero address. Setting the dispatcher to the zero address could lead to loss of functionality and potential security risks.
+ require(_dispatcher != IbcDispatcher(address(0)), "Dispatcher address cannot be zero.");Insert this validation at the beginning of the updateDispatcher function.
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.
The contract uses a fixed array of supported versions (supportedVersions) without providing a mechanism to update this array. Consider adding functionality to manage this array, allowing the addition or removal of supported versions. This would increase the contract's flexibility in adapting to protocol changes or version updates.
+ function addSupportedVersion(string calldata version) external onlyOwner {
+ supportedVersions.push(version);
+ }
+
+ function removeSupportedVersion(uint256 index) external onlyOwner {
+ require(index < supportedVersions.length, "Invalid index");
+ for (uint256 i = index; i < supportedVersions.length - 1; i++) {
+ supportedVersions[i] = supportedVersions[i + 1];
+ }
+ supportedVersions.pop();
+ }Implement these functions to manage the supportedVersions array dynamically.
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.
| function addSupportedVersion(string calldata version) external onlyOwner { | |
| supportedVersions.push(version); | |
| } | |
| function removeSupportedVersion(uint256 index) external onlyOwner { | |
| require(index < supportedVersions.length, "Invalid index"); | |
| for (uint256 i = index; i < supportedVersions.length - 1; i++) { | |
| supportedVersions[i] = supportedVersions[i + 1]; | |
| } | |
| supportedVersions.pop(); | |
| } |
Summary by CodeRabbit