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

Enumberable set cleanup #12885

Merged
merged 5 commits into from
Apr 23, 2024
Merged

Enumberable set cleanup #12885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
85d1307
support decimals
shileiwill Apr 15, 2024
3b691c1
address comments
shileiwill Apr 18, 2024
ff05527
properly cleanup the enumberable set
shileiwill Apr 18, 2024
af3b5db
add unit test and remove in reverse order
shileiwill Apr 18, 2024
ed86b01
Merge branch 'develop' into enumberableSet
shileiwill Apr 22, 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/sour-needles-collect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

cleanup set #bugfix
5 changes: 5 additions & 0 deletions contracts/.changeset/hungry-days-occur.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@chainlink/contracts": patch
---

set cleanup #bugfix
76 changes: 62 additions & 14 deletions contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {Vm} from "forge-std/Test.sol";
import {BaseTest} from "./BaseTest.t.sol";
import {AutomationRegistryBase2_3 as AutoBase} from "../v2_3/AutomationRegistryBase2_3.sol";
import {AutomationRegistrar2_3 as Registrar} from "../v2_3/AutomationRegistrar2_3.sol";
import {IAutomationRegistryMaster2_3 as Registry, AutomationRegistryBase2_3} from "../interfaces/v2_3/IAutomationRegistryMaster2_3.sol";
import {IAutomationRegistryMaster2_3 as Registry, AutomationRegistryBase2_3, IAutomationV21PlusCommon} from "../interfaces/v2_3/IAutomationRegistryMaster2_3.sol";
import {ChainModuleBase} from "../../chains/ChainModuleBase.sol";
import {IERC20Metadata as IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import {IWrappedNative} from "../interfaces/v2_3/IWrappedNative.sol";
Expand Down Expand Up @@ -334,6 +334,7 @@ contract SetConfig is SetUp {
);

address module = address(new ChainModuleBase());

AutomationRegistryBase2_3.OnchainConfig cfg =
AutomationRegistryBase2_3.OnchainConfig({
checkGasLimit: 5_000_000,
Expand All @@ -347,7 +348,7 @@ contract SetConfig is SetUp {
fallbackLinkPrice: 2_000_000_000, // $20
fallbackNativePrice: 400_000_000_000, // $4,000
transcoder: 0xB1e66855FD67f6e85F0f0fA38cd6fBABdf00923c,
registrars: new address[](0),
registrars: _getRegistrars(),
upkeepPrivilegeManager: PRIVILEGE_MANAGER,
chainModule: module,
reorgProtectionEnabled: true,
Expand Down Expand Up @@ -507,8 +508,22 @@ contract SetConfig is SetUp {
decimals: 18
});

// the first time uses the default onchain config with 2 registrars
bytes memory onchainConfigBytesWithBilling1 = abi.encode(cfg, billingTokens1, billingConfigs1);

// set config once
registry.setConfig(
SIGNERS,
TRANSMITTERS,
F,
onchainConfigBytesWithBilling1,
OFFCHAIN_CONFIG_VERSION,
offchainConfigBytes
);

(, IAutomationV21PlusCommon.OnchainConfigLegacy memory onchainConfig1, , , ) = registry.getState();
assertEq(onchainConfig1.registrars.length, 2);

// BillingConfig2
address billingTokenAddress2 = address(usdToken18);
address[] memory billingTokens2 = new address[](1);
Expand All @@ -524,17 +539,33 @@ contract SetConfig is SetUp {
decimals: 18
});

bytes memory onchainConfigBytesWithBilling2 = abi.encode(cfg, billingTokens2, billingConfigs2);
address[] memory newRegistrars = new address[](3);
newRegistrars[0] = address(uint160(uint256(keccak256("newRegistrar1"))));
newRegistrars[1] = address(uint160(uint256(keccak256("newRegistrar2"))));
newRegistrars[2] = address(uint160(uint256(keccak256("newRegistrar3"))));

// set config once
registry.setConfig(
SIGNERS,
TRANSMITTERS,
F,
onchainConfigBytesWithBilling1,
OFFCHAIN_CONFIG_VERSION,
offchainConfigBytes
);
// new onchain config with 3 new registrars, all other fields stay the same as the default
AutomationRegistryBase2_3.OnchainConfig memory cfg2 = AutomationRegistryBase2_3.OnchainConfig({
checkGasLimit: 5_000_000,
stalenessSeconds: 90_000,
gasCeilingMultiplier: 0,
maxPerformGas: 10_000_000,
maxCheckDataSize: 5_000,
maxPerformDataSize: 5_000,
maxRevertDataSize: 5_000,
fallbackGasPrice: 20_000_000_000,
fallbackLinkPrice: 2_000_000_000, // $20
fallbackNativePrice: 400_000_000_000, // $4,000
transcoder: 0xB1e66855FD67f6e85F0f0fA38cd6fBABdf00923c,
registrars: newRegistrars,
upkeepPrivilegeManager: PRIVILEGE_MANAGER,
chainModule: module,
reorgProtectionEnabled: true,
financeAdmin: FINANCE_ADMIN
});

// the second time uses the new onchain config with 3 new registrars and also new billing tokens/configs
bytes memory onchainConfigBytesWithBilling2 = abi.encode(cfg2, billingTokens2, billingConfigs2);

// set config twice
registry.setConfig(
Expand All @@ -546,8 +577,18 @@ contract SetConfig is SetUp {
offchainConfigBytes
);

(, , address[] memory signers, address[] memory transmitters, uint8 f) = registry.getState();

(
,
IAutomationV21PlusCommon.OnchainConfigLegacy memory onchainConfig2,
address[] memory signers,
address[] memory transmitters,
uint8 f
) = registry.getState();

assertEq(onchainConfig2.registrars.length, 3);
for (uint256 i = 0; i < newRegistrars.length; i++) {
assertEq(newRegistrars[i], onchainConfig2.registrars[i]);
}
assertEq(signers, SIGNERS);
assertEq(transmitters, TRANSMITTERS);
assertEq(f, F);
Expand Down Expand Up @@ -766,6 +807,13 @@ contract SetConfig is SetUp {
assertEq(transmitters, NEW_TRANSMITTERS);
}

function _getRegistrars() private pure returns (address[] memory) {
address[] memory registrars = new address[](2);
registrars[0] = address(uint160(uint256(keccak256("registrar1"))));
registrars[1] = address(uint160(uint256(keccak256("registrar2"))));
return registrars;
}
Comment on lines +810 to +815
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick - but since the function is only used once, it might be neater to just inline it? not a strong opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason of having this function is that we cant initiate and add elements to the registrars array outside of a function. If you see the original code, the registrars were under contracts.


function _configDigestFromConfigData(
uint256 chainId,
address contractAddress,
Expand Down
4 changes: 3 additions & 1 deletion contracts/src/v0.8/automation/dev/v2_3/AutomationRegistry2_3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,9 @@ contract AutomationRegistry2_3 is AutomationRegistryBase2_3, OCR2Abstract, Chain
offchainConfig
);

delete s_registrars;
for (uint256 idx = s_registrars.length(); idx > 0; idx--) {
s_registrars.remove(s_registrars.at(idx - 1));
}

for (uint256 idx = 0; idx < onchainConfig.registrars.length; idx++) {
s_registrars.add(onchainConfig.registrars[idx]);
Expand Down
5 changes: 4 additions & 1 deletion contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicC2_3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,10 @@ contract AutomationRegistryLogicC2_3 is AutomationRegistryBase2_3 {
payments[i + activeTransmittersLength] = transmitter.balance;
s_transmitters[deactivatedAddr].balance = 0;
}
delete s_deactivatedTransmitters;

for (uint256 idx = s_deactivatedTransmitters.length(); idx > 0; idx--) {
s_deactivatedTransmitters.remove(s_deactivatedTransmitters.at(idx - 1));
}

emit NOPsSettledOffchain(payees, payments);
}
Expand Down
2 changes: 1 addition & 1 deletion ...gethwrappers/generated/automation_registry_wrapper_2_3/automation_registry_wrapper_2_3.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/gethwrappers/generation/generated-wrapper-dependency-versions-do-not-edit.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ automation_registry_logic_a_wrapper_2_3: ../../contracts/solc/v0.8.19/Automation
automation_registry_logic_b_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_2/AutomationRegistryLogicB2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_2/AutomationRegistryLogicB2_2.bin a6d33dfbbfb0ff253eb59a51f4f6d6d4c22ea5ec95aae52d25d49a312b37a22f
automation_registry_logic_b_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.bin 2d0f45d2087f6f3c8bfa0a16b26a1c8c1d5c64b89859478c609201535c96eeed
automation_registry_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistry2_2/AutomationRegistry2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_2/AutomationRegistry2_2.bin de60f69878e9b32a291a001c91fc8636544c2cfbd9b507c8c1a4873b602bfb62
automation_registry_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin 2f9db5da86183eaf4f78f726458e5a928d37f7c90c4024923847b25186b644c5
automation_registry_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin 68acf720890c89512a43c3468712f5da6c1c9613bc7c462b51599293f0cce9f4
automation_utils_2_1: ../../contracts/solc/v0.8.16/AutomationUtils2_1/AutomationUtils2_1.abi ../../contracts/solc/v0.8.16/AutomationUtils2_1/AutomationUtils2_1.bin 815b17b63f15d26a0274b962eefad98cdee4ec897ead58688bbb8e2470e585f5
automation_utils_2_2: ../../contracts/solc/v0.8.19/AutomationUtils2_2/AutomationUtils2_2.abi ../../contracts/solc/v0.8.19/AutomationUtils2_2/AutomationUtils2_2.bin 8743f6231aaefa3f2a0b2d484258070d506e2d0860690e66890dccc3949edb2e
automation_utils_2_3: ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.abi ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.bin 11e2b481dc9a4d936e3443345d45d2cc571164459d214917b42a8054b295393b
Expand Down
Loading