AUTO-9179: settle NOPs LINK payment offchain #12469

FelixFan1992 · 2024-03-18T20:03:33Z

No description provided.

github-actions · 2024-03-18T20:03:53Z

I see that you haven't updated any README files. Would it make sense to do so?

github-actions · 2024-03-18T20:09:08Z

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

FelixFan1992 · 2024-03-19T01:17:11Z

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol

+    uint256[] memory balances = new uint256[](length);
+    for (uint256 i = 0; i < length; i++) {
+      address transmitterAddr = s_transmittersList[i];
+      uint96 balance = _updateTransmitterBalanceFromPool(transmitterAddr, s_hotVars.totalPremium, uint96(length));


@RyanRHall do we need to update the reserve account like
s_reserveAmounts[address(i_link)] = s_reserveAmounts[address(i_link)] - balance;?
i assume no bc using offchain settlement means there is no LINK liquidity in service chain.

Wow I had to think about this for a long time. I think it depends on whether LINK payments are enabled or not.

If LINK payments are disabled (most likely scenario) then we expect s_reserveAmounts[LINK] == 0, and so if we try to subtract balance, it will underflow and revert (not good!) In this scenario, we don't want to touch reserve amounts, it should just stay at 0.

But there is technically nothing stopping us from configuring a registry with LINK payments enabled and settlement mode set to offchain. In this scenario we would want to decrease the s_reserveAmounts, but only by the amount of LINK spent by upkeeps with LINK configured as the payment method, and we don't currently track this value. This is a super awkward configuration, and I would be in inclined to disable it all-together.

I think we should do the following:

don't touch reserveAmounts[LINK] in settleNOPsOffchain()

add a check in setConfig() that if a billinkToken == LINK, then require(s_payoutMode == PayoutMode.ON_CHAIN). This should prevent the awkward configuration I mentioned above.

RyanRHall

looks great @FelixFan1992

RyanRHall · 2024-03-19T12:21:57Z

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryBase2_3.sol

@@ -428,6 +436,7 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
  event FundsAdded(uint256 indexed id, address indexed from, uint96 amount);
  event FundsWithdrawn(uint256 indexed id, uint256 amount, address to);
  event InsufficientFundsUpkeepReport(uint256 indexed id, bytes trigger);
+  event NOPsSettledOffchain(address[] transmitters, uint256[] balances);


Please confirm with @wentzeld that this event has all the information we need. I believe a request was made to separate balances into premium and gasFees etc.

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol

RyanRHall · 2024-03-19T13:10:03Z

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol

+      s_transmitters[transmitterAddr].balance = 0;
+    }
+
+    emit NOPsSettledOffchain(s_transmittersList, balances);


we probably want to emit payees here instead of transmitters

is this bc a transmitter can use a payee which is different than transmitter?

payee is an address which has the privilege to call withdrawPayment for a transmitter. so yes they are different.

RyanRHall · 2024-03-19T13:34:34Z

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol

+    uint256[] memory balances = new uint256[](length);
+    for (uint256 i = 0; i < length; i++) {
+      address transmitterAddr = s_transmittersList[i];
+      uint96 balance = _updateTransmitterBalanceFromPool(transmitterAddr, s_hotVars.totalPremium, uint96(length));


Wow I had to think about this for a long time. I think it depends on whether LINK payments are enabled or not.

If LINK payments are disabled (most likely scenario) then we expect s_reserveAmounts[LINK] == 0, and so if we try to subtract balance, it will underflow and revert (not good!) In this scenario, we don't want to touch reserve amounts, it should just stay at 0.

But there is technically nothing stopping us from configuring a registry with LINK payments enabled and settlement mode set to offchain. In this scenario we would want to decrease the s_reserveAmounts, but only by the amount of LINK spent by upkeeps with LINK configured as the payment method, and we don't currently track this value. This is a super awkward configuration, and I would be in inclined to disable it all-together.

I think we should do the following:

don't touch reserveAmounts[LINK] in settleNOPsOffchain()

add a check in setConfig() that if a billinkToken == LINK, then require(s_payoutMode == PayoutMode.ON_CHAIN). This should prevent the awkward configuration I mentioned above.

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

shileiwill

LGTM

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

shileiwill · 2024-03-19T19:39:27Z

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

@@ -473,6 +474,37 @@ contract AutomationRegistry2_3_SetConfig is AutomationRegistry2_3_SetUp {
    );
  }

+  function testSetConfigRevertDueToInvalidBillingToken() public {


IIUC, setConfig reverts bc the payout mode is offchain, and LINK is not allowed to use. It took me a bit to understand the definition of invalid here.

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryBase2_3.sol

shileiwill · 2024-03-19T20:40:05Z

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryLogicB2_3.sol

+      s_transmitters[transmitterAddr].balance = 0;
+    }
+
+    emit NOPsSettledOffchain(s_transmittersList, balances);


is this bc a transmitter can use a payee which is different than transmitter?

github-actions · 2024-03-19T22:46:08Z

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset.

cl-sonarqube-production · 2024-03-20T22:05:06Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

RyanRHall

Looks 🔥🔥 🔥 I only have nitpicks

RyanRHall · 2024-03-20T22:36:53Z

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

-    uint256 a = 1234;
-    address b = ZERO_ADDRESS;
-    bytes memory offchainConfigBytes = abi.encode(a, b);
+    bytes memory offchainConfigBytes = abi.encode(1234, ZERO_ADDRESS);


nitpick, but all these offchainConfigBytes don't really do anything yeah? Maybe we could clean up a step further just leave them blank as "" in setConfigTypeSafe()

RyanRHall · 2024-03-20T22:40:27Z

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

+  uint256[] internal upkeepIds;
+  uint256[] internal gasLimits;
+  bytes[] internal performDatas;
+  uint256[] internal balances;


nitpick, but it looks like these are only used within the NOPsSettlement test - I think we can move them to that contract instead of making them global across all tests?

In fact, it looks like they're only used in one test yeah? Can we just move them to that one test?

contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol

RyanRHall · 2024-03-20T22:56:52Z

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryBase2_3.sol

+      if (address(token) == address(i_link) && s_payoutMode == PayoutMode.OFF_CHAIN) {
+        revert InvalidBillingToken();
+      }


IIRC, we started reading the state vars in the loop partly to avoid stack to deep errors, right? Could we add a @dev comment explaining that so that people don't think we're lazy lolol.

contracts/src/v0.8/automation/dev/v2_3/AutomationRegistryBase2_3.sol

RyanRHall

have some nitpicks, but we can address later

AUTO-9179: settle NOPs LINK payment offchain

d3ad747

FelixFan1992 temporarily deployed to sdlc March 18, 2024 20:03 — with GitHub Actions Inactive

FelixFan1992 added 2 commits March 18, 2024 16:15

update tests

ae31545

update tests

bbd49ad

FelixFan1992 had a problem deploying to sdlc March 18, 2024 20:26 — with GitHub Actions Failure

update

589b684

FelixFan1992 temporarily deployed to sdlc March 18, 2024 20:53 — with GitHub Actions Inactive

FelixFan1992 added 2 commits March 18, 2024 17:45

Merge branch 'develop' into AUTO-9179

d82bbc7

add tests

9f6f91b

FelixFan1992 temporarily deployed to sdlc March 18, 2024 22:31 — with GitHub Actions Inactive

FelixFan1992 requested review from RyanRHall and shileiwill March 18, 2024 22:35

FelixFan1992 marked this pull request as ready for review March 18, 2024 22:35

FelixFan1992 requested review from a team and RensR as code owners March 18, 2024 22:35

FelixFan1992 removed request for a team and RensR March 18, 2024 22:35

FelixFan1992 commented Mar 19, 2024

View reviewed changes

RyanRHall suggested changes Mar 19, 2024

View reviewed changes

update based on comments

49f0232

FelixFan1992 temporarily deployed to sdlc March 19, 2024 18:41 — with GitHub Actions Inactive

shileiwill reviewed Mar 19, 2024

View reviewed changes

FelixFan1992 added 3 commits March 19, 2024 18:22

rebase 1

8c368f7

revoke lock file

fbe7f60

rebase 2

0255f41

FelixFan1992 temporarily deployed to sdlc March 19, 2024 22:45 — with GitHub Actions Inactive

FelixFan1992 added 2 commits March 19, 2024 19:02

update

6825cbc

update

901d771

FelixFan1992 temporarily deployed to sdlc March 19, 2024 23:36 — with GitHub Actions Inactive

udpate tests

82d340f

FelixFan1992 temporarily deployed to sdlc March 20, 2024 02:06 — with GitHub Actions Inactive

format

7bb724f

FelixFan1992 temporarily deployed to sdlc March 20, 2024 02:12 — with GitHub Actions Inactive

FelixFan1992 requested review from shileiwill and RyanRHall March 20, 2024 02:12

small fixes

a62bc19

FelixFan1992 temporarily deployed to sdlc March 20, 2024 02:18 — with GitHub Actions Inactive

format again

8237fc4

FelixFan1992 temporarily deployed to sdlc March 20, 2024 02:50 — with GitHub Actions Inactive

update event sig

f042d26

FelixFan1992 temporarily deployed to sdlc March 20, 2024 21:51 — with GitHub Actions Inactive

RyanRHall suggested changes Mar 20, 2024

View reviewed changes

RyanRHall self-requested a review March 20, 2024 22:59

RyanRHall approved these changes Mar 20, 2024

View reviewed changes

RyanRHall added this pull request to the merge queue Mar 20, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2024

FelixFan1992 added this pull request to the merge queue Mar 20, 2024

Merged via the queue into develop with commit 1370133 Mar 20, 2024
116 checks passed

FelixFan1992 deleted the AUTO-9179 branch March 20, 2024 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AUTO-9179: settle NOPs LINK payment offchain #12469

AUTO-9179: settle NOPs LINK payment offchain #12469

FelixFan1992 commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

FelixFan1992 Mar 19, 2024

RyanRHall Mar 19, 2024

RyanRHall left a comment

RyanRHall Mar 19, 2024

RyanRHall Mar 19, 2024

shileiwill Mar 19, 2024

FelixFan1992 Mar 20, 2024

RyanRHall Mar 19, 2024

shileiwill left a comment

shileiwill Mar 19, 2024

shileiwill Mar 19, 2024

github-actions bot commented Mar 19, 2024

cl-sonarqube-production bot commented Mar 20, 2024

RyanRHall left a comment

RyanRHall Mar 20, 2024

RyanRHall Mar 20, 2024

RyanRHall Mar 20, 2024

RyanRHall Mar 20, 2024

RyanRHall left a comment

AUTO-9179: settle NOPs LINK payment offchain #12469

AUTO-9179: settle NOPs LINK payment offchain #12469

Conversation

FelixFan1992 commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanRHall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shileiwill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2024

cl-sonarqube-production bot commented Mar 20, 2024

Quality Gate passed

RyanRHall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanRHall left a comment

Choose a reason for hiding this comment