some contract v2.3 tuning #12790

shileiwill · 2024-04-11T19:41:24Z

revert on duplicate registration in registrar
handle upkeep migration when 2 upkeeps use same billing token
registerUpkeep using safeTransfer
cancel upkeep refunds the right token

RyanRHall

awesome job 🎉

RyanRHall · 2024-04-11T20:31:18Z

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

  struct PendingRequest {
    address admin;
+    IERC20 billingToken;
    uint96 balance;
  }


I think we could get rid of this entire struct and change s_pendingRequests to...

mapping(bytes32 => PendingRequest) private s_pendingRequests;

... this is what our friends recommended too :)

hmm, can you elaborate a bit more on this? if we dont have this PendingRequest struct, when users cancel their pending upkeep, how we do refund?

actually nevermind - forget I said anything 🙃

RyanRHall · 2024-04-11T20:32:56Z

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

-      revert TransferFailed(request.admin);
-    }
+
+    request.billingToken.safeTransfer(request.admin, request.balance);


RyanRHall · 2024-04-11T20:33:43Z

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

+      s_pendingRequests[hash] = PendingRequest({
+        admin: params.adminAddress,
+        billingToken: params.billingToken,
+        balance: params.amount
+      });


I think here we could just do s_pendingRequests[hash] = true;

ditto, would like some clarification.

RyanRHall · 2024-04-11T20:39:53Z

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

@@ -10,7 +10,7 @@ import {Chainable} from "../../Chainable.sol";
 import {IERC677Receiver} from "../../../shared/interfaces/IERC677Receiver.sol";
 import {OCR2Abstract} from "../../../shared/ocr2/OCR2Abstract.sol";
 import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol";
-import {SafeCast} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/math/SafeCast.sol";
+import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol";


It doesn't look like we use any library functions in this contract? So I think all this really did is increase the contract size. Not a strong opinion, but I might have left this contract as-is to keep the hot path cheaper.

yes, my bad, i forgot to clean this up after moving the addFund

RyanRHall · 2024-04-11T20:44:27Z

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

@@ -201,6 +207,7 @@ contract AutomationRegistryLogicA2_3 is AutomationRegistryBase2_3, Chainable {
        billingToken.safeTransfer(destination, balanceToTransfer);


could we move this transfer to outside the loop and drop the if clause? I think that would look cleaner.

yes, agreed.

RyanRHall · 2024-04-11T20:51:03Z

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

@@ -356,6 +358,10 @@ contract AutomationRegistrar2_3 is TypeAndVersionInterface, ConfirmedOwner, IERC
    }
    bytes32 hash = keccak256(abi.encode(params));

+    if (s_pendingRequests[hash].admin != address(0)) {
+      revert DuplicateEntry();


can we add a foundry test for the duplicate registration scenario?

RyanRHall · 2024-04-11T20:53:37Z

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

+    assertEq(
+      linkToken.balanceOf(address(newRegistry)),
+      newRegistry.getBalance(linkUpkeepID) + newRegistry.getBalance(linkUpkeepID2)
+    );


awesome 🙌

github-actions · 2024-04-11T21:44:44Z

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

github-actions · 2024-04-11T21:46:23Z

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

cl-sonarqube-production · 2024-04-11T22:10:48Z

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

Just the one question!

RyanRHall · 2024-04-11T22:50:49Z

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

-  function addFunds(uint256 id, uint96 amount) external payable {
-    Upkeep memory upkeep = s_upkeep[id];
-    if (upkeep.maxValidBlocknumber != UINT32_MAX) revert UpkeepCancelled();
-
-    if (msg.value != 0) {
-      if (upkeep.billingToken != IERC20(i_wrappedNativeToken)) {
-        revert InvalidToken();
-      }
-      amount = SafeCast.toUint96(msg.value);
-    }
-
-    s_upkeep[id].balance = upkeep.balance + amount;
-    s_reserveAmounts[upkeep.billingToken] = s_reserveAmounts[upkeep.billingToken] + amount;
-
-    if (msg.value == 0) {
-      // ERC20 payment
-      bool success = upkeep.billingToken.transferFrom(msg.sender, address(this), amount);
-      if (!success) revert TransferFailed();
-    } else {
-      // native payment
-      i_wrappedNativeToken.deposit{value: amount}();
-    }
-
-    emit FundsAdded(id, msg.sender, amount);
-  }


could we add this back to the root contract now?

unfortunately no~ the addFunds requires SafeCast and SafeERC20 libraries, so when we move back the function, we also need to move back the libs. it is oversize.

RyanRHall · 2024-04-11T22:51:14Z

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

    for (uint256 idx = 0; idx < ids.length; idx++) {
      id = ids[idx];
      upkeep = s_upkeep[id];

      if (idx == 0) {
-        billingToken = s_upkeep[id].billingToken;
+        billingToken = upkeep.billingToken;


* some contract v2.3 tuning * address comments

some contract v2.3 tuning

775327c

shileiwill requested a review from RyanRHall April 11, 2024 19:41

shileiwill requested review from a team as code owners April 11, 2024 19:41

shileiwill temporarily deployed to sdlc April 11, 2024 19:41 — with GitHub Actions Inactive

shileiwill force-pushed the AUTO-10130 branch from e966522 to 8b406cc Compare April 11, 2024 20:26

shileiwill temporarily deployed to sdlc April 11, 2024 20:26 — with GitHub Actions Inactive

RyanRHall requested changes Apr 11, 2024

View reviewed changes

shileiwill force-pushed the AUTO-10130 branch from 0771d66 to 070def2 Compare April 11, 2024 21:40

shileiwill temporarily deployed to sdlc April 11, 2024 21:40 — with GitHub Actions Inactive

shileiwill temporarily deployed to sdlc April 11, 2024 21:42 — with GitHub Actions Inactive

address comments

879f9a8

shileiwill force-pushed the AUTO-10130 branch from ab47199 to 879f9a8 Compare April 11, 2024 21:53

shileiwill temporarily deployed to sdlc April 11, 2024 21:53 — with GitHub Actions Inactive

Merge branch 'develop' into AUTO-10130

dc2c057

shileiwill temporarily deployed to sdlc April 11, 2024 21:53 — with GitHub Actions Inactive

shileiwill requested a review from RyanRHall April 11, 2024 21:53

RyanRHall reviewed Apr 11, 2024

View reviewed changes

shileiwill enabled auto-merge April 11, 2024 23:31

RyanRHall approved these changes Apr 12, 2024

View reviewed changes

shileiwill added this pull request to the merge queue Apr 12, 2024

Merged via the queue into develop with commit 521a035 Apr 12, 2024
109 checks passed

shileiwill deleted the AUTO-10130 branch April 12, 2024 14:14

app-token-issuer-infra-releng bot mentioned this pull request Apr 12, 2024

[DO NOT MERGE] Release Preview - Changeset #12800

Closed

This was referenced Apr 12, 2024

[DO NOT MERGE] Release Preview - Changeset #12803

Closed

[DO NOT MERGE] Release Preview - Changeset #12815

Closed

cedric-cordenier pushed a commit that referenced this pull request Apr 15, 2024

some contract v2.3 tuning (#12790)

38f7b46

* some contract v2.3 tuning * address comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

some contract v2.3 tuning #12790

some contract v2.3 tuning #12790

shileiwill commented Apr 11, 2024 •

edited

RyanRHall left a comment

RyanRHall Apr 11, 2024

shileiwill Apr 11, 2024

RyanRHall Apr 11, 2024

RyanRHall Apr 11, 2024

RyanRHall Apr 11, 2024

shileiwill Apr 11, 2024

RyanRHall Apr 11, 2024

shileiwill Apr 11, 2024

RyanRHall Apr 11, 2024

shileiwill Apr 11, 2024

RyanRHall Apr 11, 2024

shileiwill Apr 11, 2024

RyanRHall Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

cl-sonarqube-production bot commented Apr 11, 2024

RyanRHall left a comment

RyanRHall Apr 11, 2024

shileiwill Apr 11, 2024

RyanRHall Apr 11, 2024

		@@ -201,6 +207,7 @@ contract AutomationRegistryLogicA2_3 is AutomationRegistryBase2_3, Chainable {
		billingToken.safeTransfer(destination, balanceToTransfer);

some contract v2.3 tuning #12790

some contract v2.3 tuning #12790

Conversation

shileiwill commented Apr 11, 2024 • edited

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

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 Apr 11, 2024

github-actions bot commented Apr 11, 2024

cl-sonarqube-production bot commented Apr 11, 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

shileiwill commented Apr 11, 2024 •

edited