Reduce contract size to ensure the contract is deployable #44
Conversation
WalkthroughThe recent update involves streamlining 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 (
|
nicopernas
force-pushed
the
nico/reduce-sizes
branch
from
5f6d163
to
20b7284
Compare
nicopernas
force-pushed
the
nico/reduce-sizes
branch
from
20b7284
to
0a8dbb8
Compare
![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 (2)
- contracts/core/Dispatcher.sol (1 hunks)
- test/Dispatcher.t.sol (1 hunks)
Files skipped from review due to trivial changes (1)
- test/Dispatcher.t.sol
| // FIXME this is commented out to make the contract size smaller. We need to optimise for size | ||
| // function onCloseIbcChannel(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external { | ||
| // // verify VIBC/IBC hub chain has processed ChanCloseConfirm event | ||
| // consensusStateManager.verifyMembership( | ||
| // proof, | ||
| // bytes('channel/path/to/be/added/here'), | ||
| // bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}') | ||
| // ); | ||
| // | ||
| // // ensure port owns channel | ||
| // Channel memory channel = portChannelMap[portAddress][channelId]; | ||
| // if (channel.counterpartyChannelId == bytes32(0)) { | ||
| // revert channelNotOwnedByPortAddress(); | ||
| // } | ||
| // | ||
| // // confirm with dApp by calling its callback | ||
| // IbcChannelReceiver reciever = IbcChannelReceiver(portAddress); | ||
| // reciever.onCloseIbcChannel(channelId, channel.counterpartyPortId, channel.counterpartyChannelId); | ||
| // delete portChannelMap[portAddress][channelId]; | ||
| // emit CloseIbcChannel(portAddress, channelId); | ||
| // } |
There was a problem hiding this comment.
The onCloseIbcChannel function has been commented out to reduce the contract size, which is a critical step towards ensuring the contract's deployability. However, this change impacts the contract's ability to handle channel closure events. It's important to consider the implications of this removal on the overall system functionality and whether alternative optimizations could achieve size reduction without sacrificing important features.
Consider optimizing contract size through other methods such as code refactoring, using libraries, or splitting the contract into smaller contracts to preserve important functionalities like channel closure handling.
| * The dApp's onCloseIbcChannel callback is invoked. | ||
| * dApp should throw an error if the channel should not be closed. | ||
| */ | ||
| function onCloseIbcChannel(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external { | ||
| // verify VIBC/IBC hub chain has processed ChanCloseConfirm event | ||
| consensusStateManager.verifyMembership( | ||
| proof, | ||
| bytes('channel/path/to/be/added/here'), | ||
| bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}') | ||
| ); | ||
|
|
||
| // ensure port owns channel | ||
| Channel memory channel = portChannelMap[portAddress][channelId]; | ||
| if (channel.counterpartyChannelId == bytes32(0)) { | ||
| revert channelNotOwnedByPortAddress(); | ||
| } | ||
|
|
||
| // confirm with dApp by calling its callback | ||
| IbcChannelReceiver reciever = IbcChannelReceiver(portAddress); | ||
| reciever.onCloseIbcChannel(channelId, channel.counterpartyPortId, channel.counterpartyChannelId); | ||
| delete portChannelMap[portAddress][channelId]; | ||
| emit CloseIbcChannel(portAddress, channelId); | ||
| } | ||
| // FIXME this is commented out to make the contract size smaller. We need to optimise for size | ||
| // function onCloseIbcChannel(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external { | ||
| // // verify VIBC/IBC hub chain has processed ChanCloseConfirm event | ||
| // consensusStateManager.verifyMembership( | ||
| // proof, | ||
| // bytes('channel/path/to/be/added/here'), | ||
| // bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}') | ||
| // ); | ||
| // | ||
| // // ensure port owns channel | ||
| // Channel memory channel = portChannelMap[portAddress][channelId]; | ||
| // if (channel.counterpartyChannelId == bytes32(0)) { | ||
| // revert channelNotOwnedByPortAddress(); | ||
| // } | ||
| // | ||
| // // confirm with dApp by calling its callback | ||
| // IbcChannelReceiver reciever = IbcChannelReceiver(portAddress); | ||
| // reciever.onCloseIbcChannel(channelId, channel.counterpartyPortId, channel.counterpartyChannelId); | ||
| // delete portChannelMap[portAddress][channelId]; | ||
| // emit CloseIbcChannel(portAddress, channelId); | ||
| // } | ||
|
|
||
| // | ||
| // IBC Packet methods |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The contract uses SPDX license identifier UNLICENSED, which is appropriate for private or unpublished code but might not be suitable if the code is intended to be open-sourced. Ensure that the licensing aligns with the project's distribution and usage intentions.
Consider specifying a more specific license if the code is intended for open-source distribution, to clearly communicate usage rights and restrictions.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3-3]
The contract specifies Solidity version ^0.8.9. It's good practice to use a fixed Solidity version or a more constrained version range to avoid unexpected behavior due to compiler updates.
Consider using a fixed version or a narrower version range for Solidity to ensure consistent contract behavior.
- pragma solidity ^0.8.9;
+ pragma solidity 0.8.9;
Optimization to reduce contract size to ensure the contract is deployable.
Summary by CodeRabbit