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

FUN-1527 Add updateFromPrevious method to Functions ToS #13795

Merged
merged 22 commits into from
Jul 28, 2024

Conversation

KuphJr
Copy link
Contributor

@KuphJr KuphJr commented Jul 9, 2024

See detailed description in Jira ticket: https://smartcontract-it.atlassian.net/browse/FUN-1527

@KuphJr KuphJr requested a review from a team as a code owner July 9, 2024 21:17
@KuphJr KuphJr changed the title Add updateFromPrevious method to Functions ToS contract Add updateFromPrevious method to Functions ToS FUN-1527 Jul 9, 2024
@KuphJr KuphJr changed the title Add updateFromPrevious method to Functions ToS FUN-1527 FUN-1527 Add updateFromPrevious method to Functions ToS Jul 9, 2024
@KuphJr KuphJr requested a review from justinkaseman July 9, 2024 22:24
agparadiso
agparadiso previously approved these changes Jul 15, 2024
@@ -197,4 +201,17 @@ contract TermsOfServiceAllowList is ITermsOfServiceAllowList, IAccessController,

return blockedSenders;
}

function migratePreviouslyAllowedSenders(address[] memory previousSendersToAdd) external override onlyOwner {
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 do we want to restrict this to be run only once?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about something like that as well, but if the diff list gets too large it may need to be run more than once.

@@ -197,4 +201,17 @@ contract TermsOfServiceAllowList is ITermsOfServiceAllowList, IAccessController,

return blockedSenders;
}

function migratePreviouslyAllowedSenders(address[] memory previousSendersToAdd) external override onlyOwner {
require(s_previousToSContract != address(0), "No previous ToS contract");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use a custom error, rather than a require statement.

@@ -197,4 +201,17 @@ contract TermsOfServiceAllowList is ITermsOfServiceAllowList, IAccessController,

return blockedSenders;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing natspec inherit

Suggested change
/// @inheritdoc ITermsOfServiceAllowList

require(s_previousToSContract != address(0), "No previous ToS contract");
IAccessController previousToSContract = IAccessController(s_previousToSContract);
for (uint256 i = 0; i < previousSendersToAdd.length; ++i) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

Gas golf: If you split this into a nested if statement it saves 36 gas. This is because having them together spends the gas to evaluate each of them before doing the boolean logic.

@cl-sonarqube-production
Copy link

@KuphJr KuphJr added this pull request to the merge queue Jul 28, 2024
Merged via the queue into develop with commit 683a12e Jul 28, 2024
115 checks passed
@KuphJr KuphJr deleted the functions-ToS-v1.1.1 branch July 28, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants