emit events before callback module

Signed-off-by: Jun Kimura <[email protected]>

.gas-snapshot

@@ -1,20 +1,20 @@
 IBCMockAppTest:testHandshake() (gas: 4420488)
 IBCMockAppTest:testHandshakeBetweenDifferentPorts() (gas: 3334373)
-IBCMockAppTest:testPacketRelay() (gas: 13931365)
+IBCMockAppTest:testPacketRelay() (gas: 13935239)
 IBCMockAppTest:testPacketTimeout() (gas: 4279263)
 IBCTest:testBenchmarkCreateMockClient() (gas: 233366)
 IBCTest:testBenchmarkLCUpdateMockClient() (gas: 62005)
-IBCTest:testBenchmarkRecvPacket() (gas: 158899)
+IBCTest:testBenchmarkRecvPacket() (gas: 158870)
 IBCTest:testBenchmarkSendPacket() (gas: 128432)
 IBCTest:testBenchmarkUpdateMockClient() (gas: 160229)
 IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 947, ~: 947)
-TestICS02:testCreateClient() (gas: 36498151)
-TestICS02:testInvalidCreateClient() (gas: 36395370)
-TestICS02:testInvalidUpdateClient() (gas: 36394278)
-TestICS02:testRegisterClient() (gas: 36049944)
-TestICS02:testRegisterClientDuplicatedClientType() (gas: 36035253)
-TestICS02:testRegisterClientInvalidClientType() (gas: 36064714)
-TestICS02:testUpdateClient() (gas: 36562478)
+TestICS02:testCreateClient() (gas: 36496843)
+TestICS02:testInvalidCreateClient() (gas: 36394062)
+TestICS02:testInvalidUpdateClient() (gas: 36392970)
+TestICS02:testRegisterClient() (gas: 36048636)
+TestICS02:testRegisterClientDuplicatedClientType() (gas: 36033945)
+TestICS02:testRegisterClientInvalidClientType() (gas: 36063406)
+TestICS02:testUpdateClient() (gas: 36561170)
 TestICS03Handshake:testConnOpenAck() (gas: 1858230)
 TestICS03Handshake:testConnOpenConfirm() (gas: 2054143)
 TestICS03Handshake:testConnOpenInit() (gas: 1429838)
@@ -37,32 +37,32 @@ TestICS04Handshake:testChanOpenInit() (gas: 2543524)
 TestICS04Handshake:testChanOpenTry() (gas: 3099898)
 TestICS04Handshake:testInvalidChanOpenAck() (gas: 2439749)
 TestICS04Handshake:testInvalidChanOpenConfirm() (gas: 2517338)
-TestICS04Handshake:testInvalidChanOpenInit() (gas: 1758704)
-TestICS04Handshake:testInvalidChanOpenTry() (gas: 1773674)
-TestICS04Packet:testAcknowledgementPacket() (gas: 3351152)
+TestICS04Handshake:testInvalidChanOpenInit() (gas: 1760558)
+TestICS04Handshake:testInvalidChanOpenTry() (gas: 1775528)
+TestICS04Packet:testAcknowledgementPacket() (gas: 3351116)
 TestICS04Packet:testInvalidSendPacket() (gas: 3551583)
-TestICS04Packet:testRecvPacket() (gas: 10995054)
+TestICS04Packet:testRecvPacket() (gas: 10996130)
 TestICS04Packet:testRecvPacketTimeoutHeight() (gas: 3259727)
 TestICS04Packet:testRecvPacketTimeoutTimestamp() (gas: 3278877)
 TestICS04Packet:testSendPacket() (gas: 6412442)
 TestICS04Packet:testTimeoutOnClose() (gas: 3553289)
-TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46742362)
+TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46742335)
 TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3456630)
-TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5266752)
-TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5236270)
+TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5266005)
+TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5236258)
 TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4407259)
-TestICS04Upgrade:testUpgradeFull() (gas: 57784890)
+TestICS04Upgrade:testUpgradeFull() (gas: 57784728)
 TestICS04Upgrade:testUpgradeInit() (gas: 3069408)
 TestICS04Upgrade:testUpgradeNoChanges() (gas: 2471936)
 TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3903220)
-TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5238516)
-TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5612449)
-TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4071036)
+TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5237770)
+TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5610957)
+TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4071025)
 TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17681581)
 TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21317741)
-TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 44264108)
-TestICS04Upgrade:testUpgradeToOrdered() (gas: 56493668)
-TestICS04Upgrade:testUpgradeToUnordered() (gas: 45102699)
+TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 44264077)
+TestICS04Upgrade:testUpgradeToOrdered() (gas: 56509529)
+TestICS04Upgrade:testUpgradeToUnordered() (gas: 45113829)
 TestICS04UpgradeApp:testUpgradeAuthorizationChanneNotFound() (gas: 61690)
 TestICS04UpgradeApp:testUpgradeAuthorizationRePropose() (gas: 2565532)
 TestICS04UpgradeApp:testUpgradeAuthorizationRemove() (gas: 2473992)

contracts/core/04-channel/IBCChannelHandshake.sol

@@ -20,11 +20,14 @@ import {IBCChannelLib} from "./IBCChannelLib.sol";
 contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChannelErrors {
     using IBCHeight for Height.Data;
 
+    // ------------- IIBCChannelHandshake implementation ------------------- //
+
     /**
      * @dev channelOpenInit is called by a module to initiate a channel opening handshake with a module on another chain.
      */
     function channelOpenInit(IIBCChannelHandshake.MsgChannelOpenInit calldata msg_)
-        external
+        public
+        override
         returns (string memory, string memory)
     {
         if (msg_.channel.connection_hops.length != 1) {
@@ -52,6 +55,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
 
         string memory channelId = generateChannelIdentifier();
         initializeSequences(msg_.portId, channelId);
+        emit GeneratedChannelIdentifier(channelId);
 
         IIBCModule module = lookupModuleByPort(msg_.portId);
         string memory version = module.onChanOpenInit(
@@ -74,15 +78,15 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
             msg_.channel.connection_hops,
             version
         );
-        emit GeneratedChannelIdentifier(channelId);
         return (channelId, version);
     }
 
     /**
      * @dev channelOpenTry is called by a module to accept the first step of a channel opening handshake initiated by a module on another chain.
      */
     function channelOpenTry(IIBCChannelHandshake.MsgChannelOpenTry calldata msg_)
-        external
+        public
+        override
         returns (string memory, string memory)
     {
         if (msg_.channel.connection_hops.length != 1) {
@@ -127,6 +131,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
 
         string memory channelId = generateChannelIdentifier();
         initializeSequences(msg_.portId, channelId);
+        emit GeneratedChannelIdentifier(channelId);
 
         IIBCModule module = lookupModuleByPort(msg_.portId);
         string memory version = module.onChanOpenTry(
@@ -149,14 +154,13 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
             msg_.channel.connection_hops,
             version
         );
-        emit GeneratedChannelIdentifier(channelId);
         return (channelId, version);
     }
 
     /**
      * @dev channelOpenAck is called by the handshake-originating module to acknowledge the acceptance of the initial request by the counterparty module on the other chain.
      */
-    function channelOpenAck(IIBCChannelHandshake.MsgChannelOpenAck calldata msg_) external {
+    function channelOpenAck(IIBCChannelHandshake.MsgChannelOpenAck calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state != Channel.State.STATE_INIT) {
             revert IBCChannelUnexpectedChannelState(channel.state);
@@ -195,7 +199,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
     /**
      * @dev channelOpenConfirm is called by the counterparty module to close their end of the channel, since the other end has been closed.
      */
-    function channelOpenConfirm(IIBCChannelHandshake.MsgChannelOpenConfirm calldata msg_) external {
+    function channelOpenConfirm(IIBCChannelHandshake.MsgChannelOpenConfirm calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state != Channel.State.STATE_TRYOPEN) {
             revert IBCChannelUnexpectedChannelState(channel.state);
@@ -227,7 +231,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
     /**
      * @dev channelCloseInit is called by either module to close their end of the channel. Once closed, channels cannot be reopened.
      */
-    function channelCloseInit(IIBCChannelHandshake.MsgChannelCloseInit calldata msg_) external {
+    function channelCloseInit(IIBCChannelHandshake.MsgChannelCloseInit calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state == Channel.State.STATE_UNINITIALIZED_UNSPECIFIED) {
             revert IBCChannelChannelNotFound(msg_.portId, msg_.channelId);
@@ -249,7 +253,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
      * @dev channelCloseConfirm is called by the counterparty module to close their end of the
      * channel, since the other end has been closed.
      */
-    function channelCloseConfirm(IIBCChannelHandshake.MsgChannelCloseConfirm calldata msg_) external {
+    function channelCloseConfirm(IIBCChannelHandshake.MsgChannelCloseConfirm calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state == Channel.State.STATE_UNINITIALIZED_UNSPECIFIED) {
             revert IBCChannelChannelNotFound(msg_.portId, msg_.channelId);
@@ -285,6 +289,8 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         );
     }
 
+    // ------------- Private functions ------------------- //
+
     /**
      * @dev writeChannel writes a channel which has successfully passed the OpenInit or OpenTry handshake step.
      */
@@ -296,7 +302,7 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         ChannelCounterparty.Data calldata counterparty,
         string[] calldata connectionHops,
         string memory version
-    ) internal {
+    ) private {
         Channel.Data storage channel = channels[portId][channelId];
         channel.state = state;
         channel.ordering = order;
@@ -309,13 +315,20 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         updateChannelCommitment(portId, channelId);
     }
 
+    function initializeSequences(string memory portId, string memory channelId) internal {
+        nextSequenceSends[portId][channelId] = 1;
+        nextSequenceRecvs[portId][channelId] = 1;
+        nextSequenceAcks[portId][channelId] = 1;
+        recvStartSequences[portId][channelId].sequence = 1;
+        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(portId, channelId)] =
+            keccak256(abi.encodePacked((bytes8(uint64(1)))));
+    }
+
     function updateChannelCommitment(string memory portId, string memory channelId) private {
         commitments[IBCCommitment.channelCommitmentKey(portId, channelId)] =
             keccak256(Channel.encode(channels[portId][channelId]));
     }
 
-    /* Verification functions */
-
     function verifyChannelState(
         ConnectionEnd.Data storage connection,
         Height.Data calldata height,
@@ -345,17 +358,6 @@ contract IBCChannelHandshake is IBCModuleManager, IIBCChannelHandshake, IIBCChan
         );
     }
 
-    /* Internal functions */
-
-    function initializeSequences(string memory portId, string memory channelId) internal {
-        nextSequenceSends[portId][channelId] = 1;
-        nextSequenceRecvs[portId][channelId] = 1;
-        nextSequenceAcks[portId][channelId] = 1;
-        recvStartSequences[portId][channelId].sequence = 1;
-        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(portId, channelId)] =
-            keccak256(abi.encodePacked((bytes8(uint64(1)))));
-    }
-
     function getCounterpartyHops(string memory connectionId) internal view returns (string[] memory hops) {
         hops = new string[](1);
         hops[0] = connections[connectionId].counterparty.connection_id;

contracts/core/04-channel/IBCChannelPacketSendRecv.sol

@@ -24,6 +24,8 @@ contract IBCChannelPacketSendRecv is
 {
     using IBCHeight for Height.Data;
 
+    // --------- IIBCChannelPacketSendRecv Implementation --------- //
+
     /**
      * @dev sendPacket is called by a module in order to send an IBC packet on a channel.
      * The packet sequence generated for the packet to be sent is returned. An error
@@ -35,7 +37,7 @@ contract IBCChannelPacketSendRecv is
         Height.Data calldata timeoutHeight,
         uint64 timeoutTimestamp,
         bytes calldata data
-    ) external returns (uint64) {
+    ) public override returns (uint64) {
         authenticateChannelCapability(sourcePort, sourceChannel);
 
         Channel.Data storage channel = channels[sourcePort][sourceChannel];
@@ -86,7 +88,7 @@ contract IBCChannelPacketSendRecv is
         string calldata destinationChannel,
         uint64 sequence,
         bytes calldata acknowledgement
-    ) public {
+    ) public override {
         authenticateChannelCapability(destinationPortId, destinationChannel);
         Channel.Data storage channel = channels[destinationPortId][destinationChannel];
         if (channel.state != Channel.State.STATE_OPEN) {
@@ -98,26 +100,11 @@ contract IBCChannelPacketSendRecv is
         _writeAcknowledgement(destinationPortId, destinationChannel, sequence, acknowledgement);
     }
 
-    function _writeAcknowledgement(
-        string calldata destinationPortId,
-        string calldata destinationChannel,
-        uint64 sequence,
-        bytes memory acknowledgement
-    ) internal {
-        bytes32 ackCommitmentKey =
-            IBCCommitment.packetAcknowledgementCommitmentKeyCalldata(destinationPortId, destinationChannel, sequence);
-        if (commitments[ackCommitmentKey] != bytes32(0)) {
-            revert IBCChannelAcknowledgementAlreadyWritten(destinationPortId, destinationChannel, sequence);
-        }
-        commitments[ackCommitmentKey] = keccak256(abi.encodePacked(sha256(acknowledgement)));
-        emit WriteAcknowledgement(destinationPortId, destinationChannel, sequence, acknowledgement);
-    }
-
     /**
      * @dev recvPacket is called by a module in order to receive & process an IBC packet
      * sent on the corresponding channel end on the counterparty chain.
      */
-    function recvPacket(MsgPacketRecv calldata msg_) external {
+    function recvPacket(MsgPacketRecv calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.packet.destinationPort][msg_.packet.destinationChannel];
         if (channel.state == Channel.State.STATE_OPEN) {} else if (
             channel.state == Channel.State.STATE_FLUSHING || channel.state == Channel.State.STATE_FLUSHCOMPLETE
@@ -204,6 +191,7 @@ contract IBCChannelPacketSendRecv is
         } else {
             revert IBCChannelUnknownChannelOrder(channel.ordering);
         }
+        emit RecvPacket(msg_.packet);
         bytes memory acknowledgement = lookupModuleByChannel(
             msg_.packet.destinationPort, msg_.packet.destinationChannel
         ).onRecvPacket(msg_.packet, _msgSender());
@@ -212,7 +200,6 @@ contract IBCChannelPacketSendRecv is
                 msg_.packet.destinationPort, msg_.packet.destinationChannel, msg_.packet.sequence, acknowledgement
             );
         }
-        emit RecvPacket(msg_.packet);
     }
 
     /**
@@ -223,7 +210,7 @@ contract IBCChannelPacketSendRecv is
      * which is no longer necessary since the packet has been received and acted upon.
      * It will also increment NextSequenceAck in case of ORDERED channels.
      */
-    function acknowledgePacket(MsgPacketAcknowledgement calldata msg_) external {
+    function acknowledgePacket(MsgPacketAcknowledgement calldata msg_) public override {
         Channel.Data storage channel = channels[msg_.packet.sourcePort][msg_.packet.sourceChannel];
         if (channel.state != Channel.State.STATE_OPEN) {
             if (channel.state != Channel.State.STATE_FLUSHING) {
@@ -295,13 +282,28 @@ contract IBCChannelPacketSendRecv is
         }
 
         delete commitments[packetCommitmentKey];
+        emit AcknowledgePacket(msg_.packet, msg_.acknowledgement);
         lookupModuleByChannel(msg_.packet.sourcePort, msg_.packet.sourceChannel).onAcknowledgementPacket(
             msg_.packet, msg_.acknowledgement, _msgSender()
         );
-        emit AcknowledgePacket(msg_.packet, msg_.acknowledgement);
     }
 
-    /* Verification functions */
+    // --------- Private Functions --------- //
+
+    function _writeAcknowledgement(
+        string calldata destinationPortId,
+        string calldata destinationChannel,
+        uint64 sequence,
+        bytes memory acknowledgement
+    ) private {
+        bytes32 ackCommitmentKey =
+            IBCCommitment.packetAcknowledgementCommitmentKeyCalldata(destinationPortId, destinationChannel, sequence);
+        if (commitments[ackCommitmentKey] != bytes32(0)) {
+            revert IBCChannelAcknowledgementAlreadyWritten(destinationPortId, destinationChannel, sequence);
+        }
+        commitments[ackCommitmentKey] = keccak256(abi.encodePacked(sha256(acknowledgement)));
+        emit WriteAcknowledgement(destinationPortId, destinationChannel, sequence, acknowledgement);
+    }
 
     function verifyPacketCommitment(
         ConnectionEnd.Data storage connection,
@@ -357,8 +359,6 @@ contract IBCChannelPacketSendRecv is
         );
     }
 
-    // private functions
-
     function calcBlockDelay(uint64 timeDelay) private view returns (uint64) {
         if (timeDelay == 0) {
             return 0;