billing config override #12536

shileiwill · 2024-03-21T21:49:21Z

No description provided.

github-actions · 2024-03-21T21:49:39Z

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

shileiwill · 2024-03-21T21:52:19Z

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

+    if (address(billingConfigOverridden.priceFeed) != ZERO_ADDRESS) {
+      // use the overridden configs
+      paymentParams.billingToken.gasFeePPB = billingConfigOverridden.gasFeePPB;
+      paymentParams.billingToken.flatFeeMicroLink = billingConfigOverridden.flatFeeMicroLink;


@RyanRHall comparing the BillingConfig struct and this PaymentsParams struct, I think only these 2 configs are override-able. Let me know if anything else to override?

also, since the _calculatePaymentAmount func is upkeepID agnostic, i hope it is okay to override before the _calculatePaymentAmount call.

hey sorry this was a bit confusing. I think you could define a new struct, something like...

struct BillingOverrides { gasFeePPB; flatFeeMicroLink; // note this will become flatFeeMilliCents soon }

shileiwill · 2024-03-21T21:52:58Z

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

@@ -705,6 +710,8 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
      maxL1Fee = hotVars.gasCeilingMultiplier * hotVars.chainModule.getMaxL1Fee(maxCalldataSize);
    }

+    // TODO: this is for checkupkeep, I guess we also need to override the billing config based on upkeep ID?


IIUC, we also need to override for checkUpkeep too.

github-actions · 2024-03-22T06:21:11Z

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

RyanRHall

looks great 👍 just a couple small requests :)

RyanRHall · 2024-03-22T14:47:51Z

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

+   */
+  struct BillingOverrides {
+    uint32 gasFeePPB;
+    uint24 flatFeeMicroLink;


Update to flatFeeMilliCents after rebase :)

RyanRHall · 2024-03-22T15:11:15Z

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

+    if (s_upkeep[upkeepId].overridesEnabled) {
+      BillingOverrides memory billingOverrides = s_billingOverrides[upkeepId];
+      // use the overridden configs
+      paymentParams.gasFeePPB = billingOverrides.gasFeePPB;
+      paymentParams.flatFeeMicroLink = billingOverrides.flatFeeMicroLink;
+    }


what do you think of moving this code into _calculatePaymentAmount()? We could add upkeepID and overridesEnabled as fields of PaymentParams? this way the override code is all in one place and we don't have to write the same logic twice. Not a strong opinion!

100% agreed on reusing code wherever possible. I actually thought (see in another comment) about moving the overriding logic to _calculatePaymentAmount(), but figured that it is dryer to have the override outside of it.

The main reason is that struct PaymentParams, struct BillingTokenPaymentParams and _calculatePaymentAmount() are all pure payment related and agnostic to upkeep ID. I feel better if we just do the override beforehand, get the right PaymentParams and pass in to the _calculate func and structs. so the payment related structs/func are not tied to upkeepID.

Also not a strong opinion, I do see the pros and cons on both sides...

sounds good to me, happy to leave it as it 👍

RyanRHall · 2024-03-22T15:14:12Z

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

@@ -944,6 +966,13 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
    PaymentParams memory paymentParams,
    uint256 upkeepId
  ) internal returns (PaymentReceipt memory) {
+    if (s_upkeep[upkeepId].overridesEnabled) {


this is an extra SLOAD - can we have this function accept this overridedEnabled as an argument? We read the whole upkeep struct into memory in handleReport() see above comment about adding overridedEnabled and upkeepID to PaymentParams

adding overridedEnabled and upkeepID to PaymentParams
if the goal is to save gas, then I totally missed this. I think this is in hot path, gas usage should be prioritized.

In this _handlePayment() func, we do have other places SLOAD from s_upkeep, do you think it worth putting them closer in one word? E.g.

uint96 balance = s_upkeep[upkeepId].balance;

ooo yeah I think we can save two SLOADs here! Let's change handlePayment() to have this sig...

function _handlePayment( HotVars memory hotVars, PaymentParams memory paymentParams, uint256 upkeepId, Upkeep memory upkeep )

and then in _handleReport() we just pass the upkeep separately like this...

_handlePayment( hotVars, PaymentParams({ // ... }), report.upkeepIds[i], upkeepTransmitInfo[i].upkeep );

RyanRHall · 2024-03-22T15:16:04Z

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

+   * @param id the upkeepID
+   * @param billingOverrides the override-able billing config
+   */
+  function setBillingOverrides(uint256 id, BillingOverrides calldata billingOverrides) external {


great use of calldata 👍

RyanRHall · 2024-03-22T15:17:50Z

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

+  function setBillingOverrides(uint256 id, BillingOverrides calldata billingOverrides) external {
+    _onlyPrivilegeManagerAllowed();
+
+    s_upkeep[id].overridesEnabled = true;
+    s_billingOverrides[id] = billingOverrides;
+    emit BillingConfigOverridden(id);
+  }


in this function, could we add a check for valid upkeepID? aka revert if the upkeepID does not exist.

RyanRHall · 2024-03-22T15:18:57Z

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

-    if (msg.sender != s_storage.upkeepPrivilegeManager) {
-      revert OnlyCallableByUpkeepPrivilegeManager();
-    }
+    _onlyPrivilegeManagerAllowed();


nice, much cleaner 👍

github-actions · 2024-03-22T17:21:54Z

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

github-actions · 2024-03-22T18:07:22Z

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

shileiwill · 2024-03-22T18:11:52Z

contracts/test/v0.8/automation/AutomationRegistry2_3.test.ts

@@ -672,6 +672,7 @@ describe('AutomationRegistry2_3', () => {
      )

      const conditionalPrice = await registry.getMaxPaymentForGas(
+        upkeepId,


I will use another PR to haul all those getMaxPaymentForGas ts tests to foundry, so we can also further test the overrides logic. I hope that is okay.

totally fine!

shileiwill · 2024-03-22T18:17:40Z

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

@@ -251,10 +253,11 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
   */
  struct Upkeep {
    bool paused;
+    bool overridesEnabled;


Note: this newly added field will not exist in IAutomationRegistryMaster2_3.sol -> UpkeepInfoLegacy. But this shouldnt be a backward compatible issue.

cl-sonarqube-production · 2024-03-22T18:27:06Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

RyanRHall

LGTM 🎉

RyanRHall · 2024-03-22T19:07:18Z

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

@@ -428,6 +439,8 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
  }

  event AdminPrivilegeConfigSet(address indexed admin, bytes privilegeConfig);
+  event BillingConfigOverridden(uint256 indexed id);


can we add the BillingConfigOverride to this event?

Do you mean renaming this event from BillingConfigOverridden to BillingConfigOverride ?

oh sorry I meant like event BillingConfigOverridden(uint256 indexed id, BillingConfigOverride override);

shileiwill temporarily deployed to sdlc March 21, 2024 21:49 — with GitHub Actions Inactive

shileiwill commented Mar 21, 2024

View reviewed changes

shileiwill force-pushed the AUTO-9181 branch from 3502123 to 6539af5 Compare March 22, 2024 06:16

RyanRHall suggested changes Mar 22, 2024

View reviewed changes

shileiwill force-pushed the AUTO-9181 branch from 6539af5 to baf139c Compare March 22, 2024 17:17

shileiwill temporarily deployed to sdlc March 22, 2024 17:17 — with GitHub Actions Inactive

shileiwill force-pushed the AUTO-9181 branch from baf139c to 2960d18 Compare March 22, 2024 18:07

shileiwill temporarily deployed to sdlc March 22, 2024 18:07 — with GitHub Actions Inactive

shileiwill force-pushed the AUTO-9181 branch from 2960d18 to 9d65565 Compare March 22, 2024 18:08

shileiwill temporarily deployed to sdlc March 22, 2024 18:08 — with GitHub Actions Inactive

shileiwill marked this pull request as ready for review March 22, 2024 18:08

shileiwill requested review from a team as code owners March 22, 2024 18:08

shileiwill temporarily deployed to sdlc March 22, 2024 18:08 — with GitHub Actions Inactive

shileiwill requested a review from RyanRHall March 22, 2024 18:08

shileiwill commented Mar 22, 2024

View reviewed changes

billing config override

9dd21e1

shileiwill force-pushed the AUTO-9181 branch from ec41fa1 to 9dd21e1 Compare March 22, 2024 18:13

shileiwill requested a review from RensR as a code owner March 22, 2024 18:13

shileiwill temporarily deployed to sdlc March 22, 2024 18:13 — with GitHub Actions Inactive

Merge branch 'develop' into AUTO-9181

9c177cd

shileiwill temporarily deployed to sdlc March 22, 2024 18:13 — with GitHub Actions Inactive

shileiwill commented Mar 22, 2024

View reviewed changes

shileiwill enabled auto-merge March 22, 2024 19:04

RyanRHall approved these changes Mar 22, 2024

View reviewed changes

shileiwill added this pull request to the merge queue Mar 22, 2024

Merged via the queue into develop with commit 87b0d8f Mar 22, 2024
116 checks passed

shileiwill deleted the AUTO-9181 branch March 22, 2024 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

billing config override #12536

billing config override #12536

shileiwill commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

shileiwill Mar 21, 2024

RyanRHall Mar 22, 2024

shileiwill Mar 21, 2024

github-actions bot commented Mar 22, 2024

RyanRHall left a comment

RyanRHall Mar 22, 2024

RyanRHall Mar 22, 2024

shileiwill Mar 22, 2024

RyanRHall Mar 22, 2024

RyanRHall Mar 22, 2024

shileiwill Mar 22, 2024

RyanRHall Mar 22, 2024

RyanRHall Mar 22, 2024

RyanRHall Mar 22, 2024

RyanRHall Mar 22, 2024

github-actions bot commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

shileiwill Mar 22, 2024

RyanRHall Mar 22, 2024

shileiwill Mar 22, 2024

cl-sonarqube-production bot commented Mar 22, 2024

RyanRHall left a comment

RyanRHall Mar 22, 2024

shileiwill Mar 22, 2024

RyanRHall Mar 22, 2024

shileiwill Mar 22, 2024

billing config override #12536

billing config override #12536

Conversation

shileiwill commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

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 22, 2024

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

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

github-actions bot commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Mar 22, 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