Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce contract size to ensure the contract is deployable #44

Merged
merged 1 commit into from
Feb 19, 2024
+51 −49
Merged

Reduce contract size to ensure the contract is deployable #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 21 additions & 20 deletions contracts/core/Dispatcher.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,26 +296,27 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
* 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);
// }
Comment on lines +299 to +319
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


//
// IBC Packet methods
Comment on lines 296 to 322
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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;

Expand Down
59 changes: 30 additions & 29 deletions test/Dispatcher.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,35 +219,36 @@ contract ChannelOpenTestBase is Base {
}
}

contract DispatcherCloseChannelTest is ChannelOpenTestBase {
function test_closeChannelInit_success() public {
vm.expectEmit(true, true, true, true);
emit CloseIbcChannel(address(mars), channelId);
mars.triggerChannelClose(channelId);
}

function test_closeChannelInit_mustOwner() public {
Mars earth = new Mars(dispatcher);
vm.expectRevert(abi.encodeWithSignature('channelNotOwnedBySender()'));
earth.triggerChannelClose(channelId);
}

function test_closeChannelConfirm_success() public {
vm.expectEmit(true, true, true, true);
emit CloseIbcChannel(address(mars), channelId);
dispatcher.onCloseIbcChannel(address(mars), channelId, validProof);
}

function test_closeChannelConfirm_mustOwner() public {
vm.expectRevert(abi.encodeWithSignature('channelNotOwnedByPortAddress()'));
dispatcher.onCloseIbcChannel(address(mars), 'channel-999', validProof);
}

function test_closeChannelConfirm_invalidProof() public {
vm.expectRevert('Invalid dummy membership proof');
dispatcher.onCloseIbcChannel(address(mars), channelId, invalidProof);
}
}
// FIXME this is commented out to make the contract size smaller. We need to optimise for size
// contract DispatcherCloseChannelTest is ChannelOpenTestBase {
// function test_closeChannelInit_success() public {
// vm.expectEmit(true, true, true, true);
// emit CloseIbcChannel(address(mars), channelId);
// mars.triggerChannelClose(channelId);
// }
//
// function test_closeChannelInit_mustOwner() public {
// Mars earth = new Mars(dispatcher);
// vm.expectRevert(abi.encodeWithSignature('channelNotOwnedBySender()'));
// earth.triggerChannelClose(channelId);
// }
//
// function test_closeChannelConfirm_success() public {
// vm.expectEmit(true, true, true, true);
// emit CloseIbcChannel(address(mars), channelId);
// dispatcher.onCloseIbcChannel(address(mars), channelId, validProof);
// }
//
// function test_closeChannelConfirm_mustOwner() public {
// vm.expectRevert(abi.encodeWithSignature('channelNotOwnedByPortAddress()'));
// dispatcher.onCloseIbcChannel(address(mars), 'channel-999', validProof);
// }
//
// function test_closeChannelConfirm_invalidProof() public {
// vm.expectRevert('Invalid dummy membership proof');
// dispatcher.onCloseIbcChannel(address(mars), channelId, invalidProof);
// }
// }

contract DispatcherSendPacketTest is ChannelOpenTestBase {
// default params
Expand Down
Loading