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.

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

[DEPLOY-694]: Adds zero-value check to ScrollSequencerUptimeFeed #11710

Merged
merged 7 commits into from
Jan 24, 2024
Next Next commit
adds zero value address check to ScrollSequencerUptimeFeed _setL1Send…
…er helper function
  • Loading branch information
chris-de-leon-cll authored and mohamed-mehany committed Jan 24, 2024
commit 22018eb50e45af19fb7fb3cdcc76588e943a4c0e
Original file line number Diff line number Diff line change
Expand Up @@ -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();

event L1SenderTransferred(address indexed from, address indexed to);
/// @dev Emitted when an `updateStatus` call is ignored due to unchanged status or stale timestamp
Expand Down Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to reuse the InvalidSender error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@chris-de-leon-cll chris-de-leon-cll Jan 9, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

address from = s_l1Sender;
if (from != to) {
s_l1Sender = to;
Expand Down