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.

Sign up for GitHub

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
Merged

FUN-1527 Add updateFromPrevious method to Functions ToS #13795

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
041314e
Add updateFromPrevious method to Functions ToS contract
KuphJr Jul 9, 2024
bea9eb5
Update test
KuphJr Jul 9, 2024
6a3c3f1
Merge bea9eb52a76bc974ec52c696af9977b6b76f8d6b into c5f9173c1fd6e86a0…
KuphJr Jul 9, 2024
b1fab51
Update gethwrappers
app-token-issuer-infra-releng[bot] Jul 9, 2024
d920cd5
Added test
KuphJr Jul 9, 2024
79e76d4
Merge branch 'develop' into functions-ToS-v1.1.1
KuphJr Jul 9, 2024
b9bd2dc
prettier
KuphJr Jul 9, 2024
47e335b
Address feedback
KuphJr Jul 9, 2024
85f887d
Added tag to changeset
KuphJr Jul 9, 2024
93d2cf6
Addressed feedback on tests
KuphJr Jul 9, 2024
e44bb72
Updated gas snapshot
KuphJr Jul 9, 2024
8afc399
Add missing deployment arg to ToS in core tests
KuphJr Jul 9, 2024
c387b1d
New approach which enables batching
KuphJr Jul 12, 2024
8f4f6d6
Add require
KuphJr Jul 12, 2024
2c389b1
prettier, gas snapshot & wrappers
KuphJr Jul 12, 2024
6165bf6
Merge branch 'develop' into functions-ToS-v1.1.1
KuphJr Jul 12, 2024
4d7a859
update snapshot
KuphJr Jul 12, 2024
af9cda3
Merge branch 'develop' into functions-ToS-v1.1.1
KuphJr Jul 12, 2024
3289f77
Merge branch 'develop' into functions-ToS-v1.1.1
KuphJr Jul 26, 2024
00826e2
addressed feedback
KuphJr Jul 28, 2024
ec0f31c
prettier
KuphJr Jul 28, 2024
9e3c020
Merge branch 'develop' into functions-ToS-v1.1.1
KuphJr Jul 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/kind-garlics-smash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

Updated Functions ToS contract wrappers #internal
5 changes: 5 additions & 0 deletions contracts/.changeset/empty-ants-suffer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@chainlink/contracts': patch
---

Added updateFromPrevious method to Functions ToS contract
234 changes: 118 additions & 116 deletions contracts/gas-snapshots/functions.gas-snapshot
View file
Open in desktop

Large diffs are not rendered by default.

21 changes: 19 additions & 2 deletions contracts/src/v0.8/functions/dev/v1_X/accessControl/TermsOfServiceAllowList.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ contract TermsOfServiceAllowList is ITermsOfServiceAllowList, IAccessController,
using EnumerableSet for EnumerableSet.AddressSet;

/// @inheritdoc ITypeAndVersion
string public constant override typeAndVersion = "Functions Terms of Service Allow List v1.1.0";
string public constant override typeAndVersion = "Functions Terms of Service Allow List v1.1.1";
address private s_previousToSContract;

EnumerableSet.AddressSet private s_allowedSenders;
EnumerableSet.AddressSet private s_blockedSenders;
Expand All @@ -41,7 +42,8 @@ contract TermsOfServiceAllowList is ITermsOfServiceAllowList, IAccessController,
constructor(
TermsOfServiceAllowListConfig memory config,
address[] memory initialAllowedSenders,
address[] memory initialBlockedSenders
address[] memory initialBlockedSenders,
address previousToSContract
) ConfirmedOwner(msg.sender) {
updateConfig(config);

Expand All @@ -56,6 +58,8 @@ contract TermsOfServiceAllowList is ITermsOfServiceAllowList, IAccessController,
}
s_blockedSenders.add(initialBlockedSenders[j]);
}

s_previousToSContract = previousToSContract;
}

// ================================================================
Expand Down Expand Up @@ -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

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.

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.

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.

previousToSContract.hasAccess(previousSendersToAdd[i], "") &&
!s_blockedSenders.contains(previousSendersToAdd[i])
) {
s_allowedSenders.add(previousSendersToAdd[i]);
}
}
}
}
4 changes: 4 additions & 0 deletions contracts/src/v0.8/functions/dev/v1_X/accessControl/interfaces/ITermsOfServiceAllowList.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ interface ITermsOfServiceAllowList {
uint64 blockedSenderIdxStart,
uint64 blockedSenderIdxEnd
) external view returns (address[] memory blockedSenders);

/// @notice Enables migrating any previously allowed senders to the new contract
/// @param previousSendersToAdd - List of addresses to migrate. These address must be allowed on the previous ToS contract and not blocked
function migratePreviouslyAllowedSenders(address[] memory previousSendersToAdd) external;
}

// ================================================================
Expand Down
46 changes: 45 additions & 1 deletion contracts/src/v0.8/functions/tests/v1_X/FunctionsTermsOfServiceAllowList.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import "forge-std/Vm.sol";
/// @notice #constructor
contract FunctionsTermsOfServiceAllowList_Constructor is FunctionsRoutesSetup {
function test_Constructor_Success() public {
assertEq(s_termsOfServiceAllowList.typeAndVersion(), "Functions Terms of Service Allow List v1.1.0");
assertEq(s_termsOfServiceAllowList.typeAndVersion(), "Functions Terms of Service Allow List v1.1.1");
assertEq(s_termsOfServiceAllowList.owner(), OWNER_ADDRESS);
}
}
Expand Down Expand Up @@ -511,3 +511,47 @@ contract FunctionsTermsOfServiceAllowList_GetBlockedSendersInRange is FunctionsR
s_termsOfServiceAllowList.getBlockedSendersInRange(1, BlockedSendersCount + 1);
}
}

/// @notice #migratePreviouslyAllowedSenders
contract FunctionsTermsOfServiceAllowList_MigratePreviouslyAllowedSenders is FunctionsRoutesSetup {
function setUp() public virtual override {
FunctionsRoutesSetup.setUp();
}

function test_MigratePreviouslyAllowedSenders_RevertIfNotOwner() public {
// Send as stranger
vm.stopPrank();
vm.startPrank(STRANGER_ADDRESS);

vm.expectRevert("Only callable by owner");
address[] memory empty = new address[](0);
s_termsOfServiceAllowList.migratePreviouslyAllowedSenders(empty);
}

function test_MigratePreviouslyAllowedSenders_Success() public {
address previouslyAllowedSender1 = 0x1000000000000000000000000000000000000000;
address previouslyAllowedSender2 = 0x2000000000000000000000000000000000000000;
address currentlyBlockedSender = 0xB000000000000000000000000000000000000000;

address[] memory mockPreviousAllowlist = new address[](3);
mockPreviousAllowlist[0] = previouslyAllowedSender1;
mockPreviousAllowlist[1] = currentlyBlockedSender;
mockPreviousAllowlist[2] = previouslyAllowedSender2;

s_termsOfServiceAllowList.blockSender(currentlyBlockedSender);

vm.mockCall(
MOCK_PREVIOUS_TOS_ADDRESS,
abi.encodeWithSelector(TermsOfServiceAllowList.hasAccess.selector),
abi.encode(true)
);
s_termsOfServiceAllowList.migratePreviouslyAllowedSenders(mockPreviousAllowlist);

address[] memory currentlyAllowedSenders = s_termsOfServiceAllowList.getAllAllowedSenders();

address[] memory expectedAllowedSenders = new address[](2);
expectedAllowedSenders[0] = previouslyAllowedSender1;
expectedAllowedSenders[1] = previouslyAllowedSender2;
assertEq(currentlyAllowedSenders, expectedAllowedSenders);
}
KuphJr marked this conversation as resolved.
Show resolved Hide resolved
}
4 changes: 3 additions & 1 deletion contracts/src/v0.8/functions/tests/v1_X/Setup.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ contract FunctionsRouterSetup is BaseTest {
int256 internal LINK_USD_RATE = 1_500_000_000;
uint8 internal LINK_USD_DECIMALS = 8;

address public MOCK_PREVIOUS_TOS_ADDRESS = 0x746f730000000000000000000000000000000000;
uint256 internal TOS_SIGNER_PRIVATE_KEY = 0x3;
address internal TOS_SIGNER = vm.addr(TOS_SIGNER_PRIVATE_KEY);

Expand All @@ -57,7 +58,8 @@ contract FunctionsRouterSetup is BaseTest {
s_termsOfServiceAllowList = new TermsOfServiceAllowList(
getTermsOfServiceConfig(),
initialAllowedSenders,
initialBlockedSenders
initialBlockedSenders,
MOCK_PREVIOUS_TOS_ADDRESS
);
}

Expand Down
22 changes: 18 additions & 4 deletions core/gethwrappers/functions/generated/functions_allow_list/functions_allow_list.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/gethwrappers/functions/generation/generated-wrapper-dependency-versions-do-not-edit.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
GETH_VERSION: 1.13.8
functions: ../../../contracts/solc/v0.8.19/functions/v1_X/FunctionsRequest.abi ../../../contracts/solc/v0.8.19/functions/v1_X/FunctionsRequest.bin 3c972870b0afeb6d73a29ebb182f24956a2cebb127b21c4f867d1ecf19a762db
functions_allow_list: ../../../contracts/solc/v0.8.19/functions/v1_X/TermsOfServiceAllowList.abi ../../../contracts/solc/v0.8.19/functions/v1_X/TermsOfServiceAllowList.bin 268de8b3c061b53a1a2a1ccc0149eff68545959e29cd41b5f2e9f5dab19075cf
functions_allow_list: ../../../contracts/solc/v0.8.19/functions/v1_X/TermsOfServiceAllowList.abi ../../../contracts/solc/v0.8.19/functions/v1_X/TermsOfServiceAllowList.bin a2dd330e82529a7af4e969b3b62925ab09fe5400e0129892cb1a5d3795f97be8
functions_billing_registry_events_mock: ../../../contracts/solc/v0.8.6/functions/v0_0_0/FunctionsBillingRegistryEventsMock.abi ../../../contracts/solc/v0.8.6/functions/v0_0_0/FunctionsBillingRegistryEventsMock.bin 50deeb883bd9c3729702be335c0388f9d8553bab4be5e26ecacac496a89e2b77
functions_client: ../../../contracts/solc/v0.8.19/functions/v1_X/FunctionsClient.abi ../../../contracts/solc/v0.8.19/functions/v1_X/FunctionsClient.bin 2368f537a04489c720a46733f8596c4fc88a31062ecfa966d05f25dd98608aca
functions_client_example: ../../../contracts/solc/v0.8.19/functions/v1_X/FunctionsClientExample.abi ../../../contracts/solc/v0.8.19/functions/v1_X/FunctionsClientExample.bin abf32e69f268f40e8530eb8d8e96bf310b798a4c0049a58022d9d2fb527b601b
Expand Down
4 changes: 3 additions & 1 deletion core/services/ocr2/plugins/functions/integration_tests/v1/internal/testutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,9 @@ func StartNewChainWithContracts(t *testing.T, nClients int) (*bind.TransactOpts,
}
var initialAllowedSenders []common.Address
var initialBlockedSenders []common.Address
allowListAddress, _, allowListContract, err := functions_allow_list.DeployTermsOfServiceAllowList(owner, b, allowListConfig, initialAllowedSenders, initialBlockedSenders)
// The allowlist requires a pointer to the previous allowlist. If none exists, use the null address.
var nullPreviousAllowlist common.Address
allowListAddress, _, allowListContract, err := functions_allow_list.DeployTermsOfServiceAllowList(owner, b, allowListConfig, initialAllowedSenders, initialBlockedSenders, nullPreviousAllowlist)
require.NoError(t, err)

// Deploy Coordinator contract (matches updateConfig() in FunctionsBilling.sol)
Expand Down
Loading