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

Jump to bottom

add getters #12903

Merged
merged 4 commits into from
Apr 24, 2024
Merged

add getters #12903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a242565
support decimals
shileiwill Apr 15, 2024
ddfe7cf
address comments
shileiwill Apr 18, 2024
90c20f5
add getters
shileiwill Apr 19, 2024
50e0f2f
Merge branch 'develop' into getters
shileiwill Apr 23, 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/tasty-lions-rhyme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

add getters #internal
5 changes: 5 additions & 0 deletions contracts/.changeset/kind-snakes-invent.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@chainlink/contracts": patch
---

add getters #internal
16 changes: 14 additions & 2 deletions contracts/src/v0.8/automation/dev/interfaces/v2_3/IAutomationRegistryMaster2_3.sol
View file
Open in desktop

Large diffs are not rendered by default.

17 changes: 17 additions & 0 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 @@ -703,6 +703,23 @@ contract SetConfig is SetUp {
);
}

function testSetConfigOnTransmittersAndPayees() public {
AutomationRegistryBase2_3.TransmitterPayeeInfo[] memory transmitterPayeeInfos = registry
.getTransmittersWithPayees();
assertEq(transmitterPayeeInfos.length, TRANSMITTERS.length);

for (uint256 i = 0; i < transmitterPayeeInfos.length; i++) {
address transmitterAddress = transmitterPayeeInfos[i].transmitterAddress;
address payeeAddress = transmitterPayeeInfos[i].payeeAddress;

address expectedTransmitter = TRANSMITTERS[i];
address expectedPayee = PAYEES[i];

assertEq(transmitterAddress, expectedTransmitter);
assertEq(payeeAddress, expectedPayee);
}
}

function testSetConfigWithNewTransmittersSuccess() public {
registry = deployRegistry(AutoBase.PayoutMode.OFF_CHAIN);

Expand Down
7 changes: 6 additions & 1 deletion contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryBase2_3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
* @member index of oracle in s_signersList/s_transmittersList
* @member balance a node's balance in LINK
* @member lastCollected the total balance at which the node last withdrew
@ @dev uint96 is safe for balance / last collected because transmitters are only ever paid in LINK
* @dev uint96 is safe for balance / last collected because transmitters are only ever paid in LINK
*/
struct Transmitter {
bool active;
Expand All @@ -340,6 +340,11 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
uint96 lastCollected;
}

struct TransmitterPayeeInfo {
address transmitterAddress;
address payeeAddress;
}

struct Signer {
bool active;
// Index of oracle in s_signersList/s_transmittersList
Expand Down
31 changes: 31 additions & 0 deletions 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 @@ -582,4 +582,35 @@ contract AutomationRegistryLogicC2_3 is AutomationRegistryBase2_3 {
function linkAvailableForPayment() public view returns (int256) {
return _linkAvailableForPayment();
}

/**
* @notice returns the BillingOverrides config for a given upkeep
*/
function getBillingOverrides(uint256 upkeepID) external view returns (BillingOverrides memory) {
return s_billingOverrides[upkeepID];
}

/**
* @notice returns the BillingConfig for a given billing token, this includes decimals and price feed etc
*/
function getBillingConfig(IERC20 billingToken) external view returns (BillingConfig memory) {
return s_billingConfigs[billingToken];
}

/**
* @notice returns all active transmitters with their associated payees
*/
function getTransmittersWithPayees() external view returns (TransmitterPayeeInfo[] memory) {
uint256 transmitterCount = s_transmittersList.length;
TransmitterPayeeInfo[] memory transmitters = new TransmitterPayeeInfo[](transmitterCount);

for (uint256 i = 0; i < transmitterCount; i++) {
address transmitterAddress = s_transmittersList[i];
address payeeAddress = s_transmitterPayees[transmitterAddress];

transmitters[i] = TransmitterPayeeInfo(transmitterAddress, payeeAddress);
}

return transmitters;
}
}
Comment on lines +603 to 616
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey sorry to do this after you've rebased.... but do you think we could change this into...

function getTransmitters() returns(address[]) {...}

function getPayees() returns(address[]) {...}

This is derived from our effort to avoid "monolithic getters" and instead prefer individual getters. Doing this tends to avoid migration issues better. Ex if a future version of automaition adds a struct to TransmitterPayeeInfo like maybe they add the field balance, then anyone using getTransmittersWithPayees() will break when they upgrade to the new registry. If we force all consumers to use these individual getters, it makes migrating easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, we work to improve whatever can be improved :). I get your point, and totally agree with the benefit of having smaller getters.

The only counter opinion for this particular usecase is that the transmitter and payee is 1:1, and the order really matters. So having them together have the benefit of grouping things in the right order. Not a strong opinion tho, they are sort/count manually with two getters. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking more about the lifetime of this struct, TransmitterPayeeInfo - we could be careful not to change it and maybe just add a comment. But it does remind me of the Upkeep struct where we add things to it version after version and now we have that whole UpkeepInfoLegacyV2PlusBlahBlah lol

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to know more information about transmitters, I'd lean towards organizing that info into individual getters

Copy link
Contributor Author

@shileiwill shileiwill Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

discussed offline, keeping it as is bc of the 1:1 mapping (order matters) of transmitter and payee.

79 changes: 78 additions & 1 deletion ...ated/i_automation_registry_master_wrapper_2_3/i_automation_registry_master_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 @@ -31,7 +31,7 @@ dummy_protocol_wrapper: ../../contracts/solc/v0.8.16/DummyProtocol/DummyProtocol
gas_wrapper: ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2/KeeperRegistryCheckUpkeepGasUsageWrapper1_2.abi ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2/KeeperRegistryCheckUpkeepGasUsageWrapper1_2.bin 4a5dcdac486d18fcd58e3488c15c1710ae76b977556a3f3191bd269a4bc75723
gas_wrapper_mock: ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock.abi ../../contracts/solc/v0.8.6/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock/KeeperRegistryCheckUpkeepGasUsageWrapper1_2Mock.bin a9b08f18da59125c6fc305855710241f3d35161b8b9f3e3f635a7b1d5c6da9c8
i_automation_registry_master_wrapper_2_2: ../../contracts/solc/v0.8.19/IAutomationRegistryMaster/IAutomationRegistryMaster.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster/IAutomationRegistryMaster.bin 9ff7087179f89f9b05964ebc3e71332fce11f1b8e85058f7b16b3bc0dd6fb96b
i_automation_registry_master_wrapper_2_3: ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.bin fbfa3f5d78a357ecb7a1bc597c629ff30d42fedc48ba7f57e1622a6302d36523
i_automation_registry_master_wrapper_2_3: ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.bin 19f51996d05341f1229f21be26b5d72ff58e321f0b6da69c260f768e4622ae8e
i_automation_v21_plus_common: ../../contracts/solc/v0.8.19/IAutomationV21PlusCommon/IAutomationV21PlusCommon.abi ../../contracts/solc/v0.8.19/IAutomationV21PlusCommon/IAutomationV21PlusCommon.bin e8a601ec382c0a2e83c49759de13b0622b5e04e6b95901e96a1e9504329e594c
i_chain_module: ../../contracts/solc/v0.8.19/IChainModule/IChainModule.abi ../../contracts/solc/v0.8.19/IChainModule/IChainModule.bin 383611981c86c70522f41b8750719faacc7d7933a22849d5004799ebef3371fa
i_keeper_registry_master_wrapper_2_1: ../../contracts/solc/v0.8.16/IKeeperRegistryMaster/IKeeperRegistryMaster.abi ../../contracts/solc/v0.8.16/IKeeperRegistryMaster/IKeeperRegistryMaster.bin ee0f150b3afbab2df3d24ff3f4c87851efa635da30db04cd1f70cb4e185a1781
Expand Down
Loading