From b321989cdbefc82c54d7b139905ab60da9c7468d Mon Sep 17 00:00:00 2001 From: Ermyas Abebe Date: Sun, 17 Sep 2023 12:54:16 +1000 Subject: [PATCH] Rename types and errors for clarity --- src/MultiMessageReceiver.sol | 26 ++++---- src/MultiMessageSender.sol | 76 +++++++++++----------- src/libraries/Error.sol | 9 +-- test/unit-tests/MultiMessageReceiver.t.sol | 12 ++-- test/unit-tests/MultiMessageSender.t.sol | 12 ++-- 5 files changed, 66 insertions(+), 69 deletions(-) diff --git a/src/MultiMessageReceiver.sol b/src/MultiMessageReceiver.sol index d0c2e5d..e349ce5 100644 --- a/src/MultiMessageReceiver.sol +++ b/src/MultiMessageReceiver.sol @@ -29,9 +29,9 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ /// @notice stores each msg id related info mapping(bytes32 => bool) public isExecuted; - mapping(bytes32 => ExecutionData) public msgReceived; - mapping(bytes32 => mapping(address => bool)) public isDuplicateAdapter; - mapping(bytes32 => uint256) public messageVotes; + mapping(bytes32 => ExecutionData) public msgExecData; + mapping(bytes32 => mapping(address => bool)) public msgDeliveries; + mapping(bytes32 => uint256) public msgDeliveryCount; /*///////////////////////////////////////////////////////////////// MODIFIERS @@ -108,7 +108,7 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ /// although each adapters' internal msgId is attached at the end of calldata, it's not useful to MultiMessageReceiver. bytes32 msgId = MessageLibrary.computeMsgId(_message); - if (isDuplicateAdapter[msgId][msg.sender]) { + if (msgDeliveries[msgId][msg.sender]) { revert Error.DUPLICATE_MESSAGE_DELIVERY_BY_ADAPTER(); } @@ -116,17 +116,17 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ revert Error.MSG_ID_ALREADY_EXECUTED(); } - isDuplicateAdapter[msgId][msg.sender] = true; + msgDeliveries[msgId][msg.sender] = true; /// increment quorum - ++messageVotes[msgId]; + ++msgDeliveryCount[msgId]; /// stores the execution data required - ExecutionData memory prevStored = msgReceived[msgId]; + ExecutionData memory prevStored = msgExecData[msgId]; /// stores the message if the amb is the first one delivering the message if (prevStored.target == address(0)) { - msgReceived[msgId] = ExecutionData( + msgExecData[msgId] = ExecutionData( _message.target, _message.callData, _message.nativeValue, _message.nonce, _message.expiration ); } @@ -138,7 +138,7 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ /// @dev has reached the power threshold (the same message has been delivered by enough multiple bridges). /// Param values can be found in the MultiMessageMsgSent event from the source chain MultiMessageSender contract. function executeMessage(bytes32 msgId) external { - ExecutionData memory _execData = msgReceived[msgId]; + ExecutionData memory _execData = msgExecData[msgId]; /// @dev validates if msg execution is not beyond expiration if (block.timestamp > _execData.expiration) { @@ -153,8 +153,8 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ isExecuted[msgId] = true; /// @dev validates message quorum - if (messageVotes[msgId] < quorum) { - revert Error.INVALID_QUORUM_FOR_EXECUTION(); + if (msgDeliveryCount[msgId] < quorum) { + revert Error.QUORUM_NOT_ACHIEVED(); } /// @dev queues the action on timelock for execution @@ -199,13 +199,13 @@ contract MultiMessageReceiver is IMultiMessageReceiver, ExecutorAware, Initializ /// @notice view message info, return (executed, msgPower, delivered adapters) function getMessageInfo(bytes32 msgId) public view returns (bool, uint256, string[] memory) { - uint256 msgCurrentVotes = messageVotes[msgId]; + uint256 msgCurrentVotes = msgDeliveryCount[msgId]; string[] memory successfulBridge = new string[](msgCurrentVotes); if (msgCurrentVotes != 0) { uint256 currIndex; for (uint256 i; i < trustedExecutor.length;) { - if (isDuplicateAdapter[msgId][trustedExecutor[i]]) { + if (msgDeliveries[msgId][trustedExecutor[i]]) { successfulBridge[currIndex] = IMessageReceiverAdapter(trustedExecutor[i]).name(); ++currIndex; } diff --git a/src/MultiMessageSender.sol b/src/MultiMessageSender.sol index 6eef133..4d5cb79 100644 --- a/src/MultiMessageSender.sol +++ b/src/MultiMessageSender.sol @@ -10,6 +10,7 @@ import "./interfaces/IGAC.sol"; /// libraries import "./libraries/Message.sol"; import "./libraries/Error.sol"; +import "forge-std/console.sol"; /// @title MultiMessageSender /// @dev handles the routing of message from external sender to bridge adapters @@ -19,8 +20,8 @@ contract MultiMessageSender { //////////////////////////////////////////////////////////////*/ IGAC public immutable gac; - uint256 public constant MINIMUM_EXPIRATION = 2 days; - uint256 public constant MAXIMUM_EXPIRATION = 30 days; + uint256 public constant MIN_EXPIRATION = 2 days; + uint256 public constant MAX_EXPIRATION = 30 days; /*///////////////////////////////////////////////////////////////// STATE VARIABLES @@ -78,13 +79,10 @@ contract MultiMessageSender { /// @dev validates the expiration provided by the user modifier validateExpiration(uint256 _expiration) { - if (_expiration < MINIMUM_EXPIRATION) { - revert Error.INVALID_EXPIRATION_MIN(); + if (_expiration < MIN_EXPIRATION || _expiration > MAX_EXPIRATION) { + revert Error.INVALID_EXPIRATION_DURATION(); } - if (_expiration > MAXIMUM_EXPIRATION) { - revert Error.INVALID_EXPIRATION_MAX(); - } _; } @@ -146,10 +144,10 @@ contract MultiMessageSender { } /// @notice Add bridge sender adapters - /// @param _senderAdapters is the adapter address to add - function addSenderAdapters(address[] calldata _senderAdapters) external onlyOwner { - for (uint256 i; i < _senderAdapters.length;) { - _addSenderAdapter(_senderAdapters[i]); + /// @param _adapters is the adapter address to add + function addSenderAdapters(address[] calldata _adapters) external onlyOwner { + for (uint256 i; i < _adapters.length;) { + _addSenderAdapter(_adapters[i]); unchecked { ++i; @@ -158,10 +156,10 @@ contract MultiMessageSender { } /// @notice Remove bridge sender adapters - /// @param _senderAdapters is the adapter address to remove - function removeSenderAdapters(address[] calldata _senderAdapters) external onlyOwner { - for (uint256 i; i < _senderAdapters.length;) { - _removeSenderAdapter(_senderAdapters[i]); + /// @param _adapters is the adapter address to remove + function removeSenderAdapters(address[] calldata _adapters) external onlyOwner { + for (uint256 i; i < _adapters.length;) { + _removeSenderAdapter(_adapters[i]); unchecked { ++i; @@ -236,10 +234,9 @@ contract MultiMessageSender { ); bytes32 msgId = MessageLibrary.computeMsgId(message); - address[] memory adapters = _getSenderAdapters(_excludedAdapters); - uint256 adapterLength = adapters.length; + (address[] memory adapters, uint256 adaptersCount) = _getSenderAdapters(_excludedAdapters); - if (adapterLength == 0) { + if (adaptersCount == 0) { revert Error.NO_SENDER_ADAPTER_FOUND(); } @@ -249,36 +246,35 @@ contract MultiMessageSender { ); /// refund remaining fee - /// FIXME: add an explicit refund address config if (address(this).balance > 0) { _safeTransferETH(gac.getRefundAddress(), address(this).balance); } } - function _addSenderAdapter(address _senderAdapter) private { - if (_senderAdapter == address(0)) { + function _addSenderAdapter(address _adapter) private { + if (_adapter == address(0)) { revert Error.ZERO_ADDRESS_INPUT(); } /// @dev reverts if it finds a duplicate - _checkDuplicates(_senderAdapter); + _checkDuplicates(_adapter); - senderAdapters.push(_senderAdapter); - emit SenderAdapterUpdated(_senderAdapter, true); + senderAdapters.push(_adapter); + emit SenderAdapterUpdated(_adapter, true); } - function _removeSenderAdapter(address _senderAdapter) private { + function _removeSenderAdapter(address _adapter) private { uint256 lastIndex = senderAdapters.length - 1; for (uint256 i; i < senderAdapters.length; ++i) { - if (senderAdapters[i] == _senderAdapter) { + if (senderAdapters[i] == _adapter) { if (i < lastIndex) { senderAdapters[i] = senderAdapters[lastIndex]; } senderAdapters.pop(); - emit SenderAdapterUpdated(_senderAdapter, false); + emit SenderAdapterUpdated(_adapter, false); return; } } @@ -292,8 +288,10 @@ contract MultiMessageSender { ) internal returns (bool[] memory) { uint256 len = _adapters.length; bool[] memory successes = new bool[](len); + console.log("Dispatching Messages"); for (uint256 i; i < len;) { IMessageSenderAdapter bridgeAdapter = IMessageSenderAdapter(_adapters[i]); + console.log("Dispatching Message: ", _adapters[i]); /// @dev assumes CREATE2 deployment for mma sender & receiver uint256 fee = bridgeAdapter.getMessageFee(_dstChainId, _mmaReceiver, abi.encode(_message)); @@ -315,12 +313,15 @@ contract MultiMessageSender { return successes; } - function _getSenderAdapters(address[] memory _exclusions) internal view returns (address[] memory) { - address[] memory targetAdapters = new address[](senderAdapters.length - _exclusions.length); - uint256 len; - for (uint256 i; i < senderAdapters.length;) { + function _getSenderAdapters(address[] memory _exclusions) internal view returns (address[] memory, uint256) { + uint256 allLen = senderAdapters.length; + uint256 exclLen = _exclusions.length; + + address[] memory inclAdapters = new address[](allLen - exclLen); + uint256 inclAdaptersLen; + for (uint256 i; i < allLen;) { bool excluded = false; - for (uint256 j; j < _exclusions.length;) { + for (uint256 j; j < exclLen;) { if (senderAdapters[i] == _exclusions[j]) { excluded = true; break; @@ -330,23 +331,22 @@ contract MultiMessageSender { } } if (!excluded) { - targetAdapters[len] = senderAdapters[i]; - ++len; + inclAdapters[inclAdaptersLen++] = senderAdapters[i]; } unchecked { ++i; } } - return targetAdapters; + return (inclAdapters, inclAdaptersLen); } /// @dev validates if the sender adapter already exists - /// @param _senderAdapter is the address of the sender to check - function _checkDuplicates(address _senderAdapter) internal view { + /// @param _adapter is the address of the sender to check + function _checkDuplicates(address _adapter) internal view { uint256 len = senderAdapters.length; for (uint256 i; i < len;) { - if (senderAdapters[i] == _senderAdapter) { + if (senderAdapters[i] == _adapter) { revert Error.DUPLICATE_SENDER_ADAPTER(); } diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index d83004c..96c34b6 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -42,7 +42,7 @@ library Error { error MSG_EXECUTION_PASSED_DEADLINE(); /// @dev is thrown if quorum is not reached - error INVALID_QUORUM_FOR_EXECUTION(); + error QUORUM_NOT_ACHIEVED(); /// @dev is thrown if message execution fails on the destination chain error EXECUTION_FAILS_ON_DST(); @@ -53,11 +53,8 @@ library Error { /// @dev is thrown if caller is not admin of timelock error CALLER_NOT_ADMIN(); - /// @dev is thrown if the expiration is less than minimum expiration - error INVALID_EXPIRATION_MIN(); - - /// @dev is thrown if the delay is more than maximum delay - error INVALID_EXPIRATION_MAX(); + /// @dev is thrown if the expiration is outside a defined range + error INVALID_EXPIRATION_DURATION(); /*///////////////////////////////////////////////////////////////// ADAPTER ERRORS diff --git a/test/unit-tests/MultiMessageReceiver.t.sol b/test/unit-tests/MultiMessageReceiver.t.sol index 48469c4..d9b3bd3 100644 --- a/test/unit-tests/MultiMessageReceiver.t.sol +++ b/test/unit-tests/MultiMessageReceiver.t.sol @@ -172,12 +172,12 @@ contract MultiMessageReceiverTest is Setup { assertFalse(receiver.isExecuted(msgId)); - assertTrue(receiver.isDuplicateAdapter(msgId, wormholeAdapterAddr)); + assertTrue(receiver.msgDeliveries(msgId, wormholeAdapterAddr)); - assertEq(receiver.messageVotes(msgId), 1); + assertEq(receiver.msgDeliveryCount(msgId), 1); (address target, bytes memory callData, uint256 nativeValue, uint256 nonce, uint256 expiration) = - receiver.msgReceived(msgId); + receiver.msgExecData(msgId); assertEq(target, message.target); assertEq(callData, message.callData); assertEq(nativeValue, message.nativeValue); @@ -205,7 +205,7 @@ contract MultiMessageReceiverTest is Setup { vm.startPrank(axelarAdapterAddr); receiver.receiveMessage(message, "AXELAR"); - assertEq(receiver.messageVotes(msgId), 2); + assertEq(receiver.msgDeliveryCount(msgId), 2); } /// @dev only adapters can call @@ -407,7 +407,7 @@ contract MultiMessageReceiverTest is Setup { } /// @dev cannot execute message without quorum - function test_execute_message_invalid_quorum_for_execution() public { + function test_execute_message_quorum_not_met_for_exec() public { vm.startPrank(wormholeAdapterAddr); MessageLibrary.Message memory message = MessageLibrary.Message({ @@ -423,7 +423,7 @@ contract MultiMessageReceiverTest is Setup { receiver.receiveMessage(message, "WORMHOLE"); - vm.expectRevert(Error.INVALID_QUORUM_FOR_EXECUTION.selector); + vm.expectRevert(Error.QUORUM_NOT_ACHIEVED.selector); receiver.executeMessage(msgId); } diff --git a/test/unit-tests/MultiMessageSender.t.sol b/test/unit-tests/MultiMessageSender.t.sol index e5c82c7..21ac91a 100644 --- a/test/unit-tests/MultiMessageSender.t.sol +++ b/test/unit-tests/MultiMessageSender.t.sol @@ -160,24 +160,24 @@ contract MultiMessageSenderTest is Setup { /// @dev message expiration has to be within allowed range function test_remote_call_invalid_expiration() public { - uint256 invalidExpMin = sender.MINIMUM_EXPIRATION() - 1 days; - uint256 invalidExpMax = sender.MAXIMUM_EXPIRATION() + 1 days; + uint256 invalidExpMin = sender.MIN_EXPIRATION() - 1 days; + uint256 invalidExpMax = sender.MAX_EXPIRATION() + 1 days; // test expiration validation in remoteCall() which does not accept excluded adapters vm.startPrank(caller); - vm.expectRevert(Error.INVALID_EXPIRATION_MIN.selector); + vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall(DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMin); - vm.expectRevert(Error.INVALID_EXPIRATION_MAX.selector); + vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall(DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMax); // test expiration validation in remoteCall() which accepts excluded adapters address[] memory excludedAdapters = new address[](0); vm.startPrank(caller); - vm.expectRevert(Error.INVALID_EXPIRATION_MIN.selector); + vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall(DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMin, excludedAdapters); - vm.expectRevert(Error.INVALID_EXPIRATION_MAX.selector); + vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall(DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMax, excludedAdapters); }