[DEPLOY-694]: Adds zero-value check to ScrollSequencerUptimeFeed #11710
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
| @@ -96,6 +98,10 @@ contract ScrollSequencerUptimeFeed is | |||
|
|
|||
| /// @notice internal method that stores the L1 sender | |||
| function _setL1Sender(address to) private { | |||
| if (to == address(0)) { | |||
| revert ZeroAddress(); | |||
There was a problem hiding this comment.
Would it make sense to reuse the InvalidSender error here?
There was a problem hiding this comment.
I think it mainly comes down to preference here -
The InvalidSender error looks like it was meant for a very specific scenario involving msg.sender, so I figured re-using it in other places could be a bit misleading for someone trying to debug or perform error handling
With that in mind, I created a separate error, but I'm open to changing it to the way you described above if you have a strong preference here
There was a problem hiding this comment.
I see your point. My suggestion was related to how can the name be used to debug. For example if I see just the error name displayed in an event log I wouldn't know which address is this referring to. Maybe we can still create a new error but with a more descriptive naming?
There was a problem hiding this comment.
Ah I see what you mean - I've added a msg argument to the ZeroAddress error, and specified which address is causing the issue using the parameter. Is this more of what you had in mind?
| @@ -41,6 +41,8 @@ contract ScrollSequencerUptimeFeed is | |||
| error InvalidSender(); | |||
| /// @notice Replacement for AggregatorV3Interface "No data present" | |||
| error NoDataPresent(); | |||
| /// @notice Address must not be the zero address | |||
| error ZeroAddress(string msg); | |||
There was a problem hiding this comment.
Let's remove the string arg here, that takes up a lot of gas and contract size. The error name has enough info
chris-de-leon-cll
temporarily deployed
to
sdlc
GitHub Actions
Inactive
chris-de-leon-cll
temporarily deployed
to
sdlc
GitHub Actions
Inactive
| @@ -68,6 +70,10 @@ contract ScrollSequencerUptimeFeed is | |||
| /// @param l2CrossDomainMessengerAddr Address of the L2CrossDomainMessenger contract | |||
| /// @param initialStatus The initial status of the feed | |||
| constructor(address l1SenderAddress, address l2CrossDomainMessengerAddr, bool initialStatus) { | |||
| if (l2CrossDomainMessengerAddr == address(0)) { | |||
| revert ZeroAddress(); | |||
There was a problem hiding this comment.
I would expect a test for each revert. Since this adds a revert I would expect a test. Same for the other one
chris-de-leon-cll
temporarily deployed
to
sdlc
GitHub Actions
Inactive
chris-de-leon-cll
temporarily deployed
to
sdlc
GitHub Actions
Inactive
| function _setL1Sender(address to) private { | ||
| if (to == address(0)) { | ||
| revert ZeroAddress(); |
There was a problem hiding this comment.
I just noticed that this might cause an issue since we deploy the contracts initially without setting the L1 address (setting it to 0) here, then we update it later. Not sure if it's the optimal ordering of the contracts deployment but in this case we would need to remove this zero check.
To clarify I'm only referring to the L1 zero check, not the messenger contract address zero check, the latter is fine.
| @@ -40,6 +40,14 @@ contract ScrollSequencerUptimeFeedTest is L2EPTest { | |||
| contract ScrollSequencerUptimeFeed_Constructor is ScrollSequencerUptimeFeedTest { | |||
| /// @notice it should have been deployed with the correct initial state | |||
| function test_InitialState() public { | |||
| // L1 sender address must not be the zero address | |||
| vm.expectRevert(abi.encodeWithSelector(ScrollSequencerUptimeFeed.ZeroAddress.selector)); | |||
There was a problem hiding this comment.
nit: you dont have to encode it if it's just the selector
chris-de-leon-cll
temporarily deployed
to
sdlc
GitHub Actions
Inactive
chris-de-leon-cll
temporarily deployed
to
sdlc
GitHub Actions
Inactive
chris-de-leon-cll
changed the title
mohamed-mehany
force-pushed
the
feat/DEPLOY-694-scroll-suf-zero-value-checks
branch
from
7867213 to
bb434b1
Compare
|
SonarQube Quality Gate
|
No description provided.