feat: stop messaging #7
Conversation
OPT-29 BaseOpUSDCBridgeAdapter stopMessaging() & receiveStopMessaging()
stopMessaging():
receiveStopMessaging():
|
| * @param _expectedSender The expected sender of the message | ||
| */ | ||
| function _checkCrossDomainMsgSender(address _expectedSender) internal view { | ||
| if (msg.sender != MESSENGER || ICrossDomainMessenger(MESSENGER).xDomainMessageSender() != _expectedSender) { |
There was a problem hiding this comment.
Since its just one line of code I dont think its worth it to make it an internal function as it adds extra JUMP opcodes making it more expensive, lets just put the if statement in the receiveStopMessaging
| if (msg.sender != MESSENGER || ICrossDomainMessenger(MESSENGER).xDomainMessageSender() != linkedAdapter) { | ||
| revert IOpUSDCBridgeAdapter_NotLinkedAdapter(); | ||
| } | ||
| _checkCrossDomainMsgSender(linkedAdapter); |
| event MessagingStopped(); | ||
|
|
||
| function testStopMessaging() public { | ||
| address _linkedAdapter = makeAddr('linkedAdapter'); |
There was a problem hiding this comment.
fuzz _linkedAdapter and _minGasLimit
| bytes memory _messageData = abi.encodeWithSignature('receiveStopMessaging()'); | ||
| uint32 _minGasLimit = 21_000; | ||
|
|
||
| stdstore.target(address(adapter)).sig('linkedAdapter()').checked_write(_linkedAdapter); |
There was a problem hiding this comment.
honestly I think its much cleaner and simpler if we just use setLinkedAdapter instead of stdstore, they do the same thing and we already have setLinkedAdapter fully covered in other test cases
| abi.encode('') | ||
| ); | ||
|
|
||
| vm.expectEmit(true, true, true, true); |
There was a problem hiding this comment.
Lets split the expect emit into a different test case, and also use vm.expectCall for this one to ensure we are calling what we mock
| // Execute | ||
| vm.prank(_user); | ||
| vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_MessagingDisabled.selector); | ||
| adapter.send(0, 0); |
There was a problem hiding this comment.
fuzz the variables instead of 0,0
| // link adapter and isMessagingDisabled are in the same storage slot | ||
| // _linkedAdapter = eeb02b2b8d37153666cba8021567b92a6e22de64 | ||
| // isMessagingDisabled = true = 0x00000001 | ||
| stdstore.target(address(adapter)).sig('linkedAdapter()').checked_write( |
There was a problem hiding this comment.
this use of stdstore is creative, but its much simpler and easy to maintain if we just create a test setter function to manipulate storage, see how I have a TestOPUSDCBridgeAdapter that extends the real one, add a setter function to make isMessageDisabled true in the test contract.
This also makes it much easier to maintain if we ever need to change the storage layout
| @@ -1,11 +1,19 @@ | |||
| pragma solidity ^0.8.25; | |||
|
|
|||
| import {L1OpUSDCBridgeAdapter} from 'contracts/L1OpUSDCBridgeAdapter.sol'; | |||
| import {Test} from 'forge-std/Test.sol'; | |||
| import {StdStorage, Test, stdStorage} from 'forge-std/Test.sol'; | |||
There was a problem hiding this comment.
StdStorage and stdStorage are now unused i believe, so we can remove them
| adapter.setLinkedAdapter(_linkedAdapter); | ||
|
|
||
| // Execute | ||
| vm.prank(_notLinkedAdapter); |
There was a problem hiding this comment.
msg.sender is supposed to be MESSENGER I would change this to _notMessenger to be clear.
We also need one more test case for the case of when msg.sender is messenger but xDomainMessageSender() is not the linkedAdapter
| function receiveStopMessaging() external virtual linkedAdapterMustBeInitialized { | ||
| // Ensure the message is coming from the linked adapter | ||
| if (msg.sender != MESSENGER || ICrossDomainMessenger(MESSENGER).xDomainMessageSender() != linkedAdapter) { | ||
| revert IOpUSDCBridgeAdapter_NotLinkedAdapter(); |
There was a problem hiding this comment.
I think the error name could be improve to be a bit more self-explanatory, wdyt about using something like OpUSDCBridgeAdapter_CallerNotLinkedAdapter or IOpUSDCBridgeAdapter_InvalidSender?
hexshire
changed the title
🤖 Linear
Closes OPT-29