Link liquidity pool, add withdraw functions for finance team and some refactoring #12375

shileiwill · 2024-03-11T16:16:08Z

AUTO-9114

github-actions · 2024-03-11T16:16:28Z

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

github-actions · 2024-03-11T16:17:56Z

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

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

RyanRHall · 2024-03-11T16:50:08Z

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

@@ -295,7 +298,6 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
   */
  struct State {


we actually have to keep this struct as-is for backwards compatibility :/

but I'm happy to just return 0 for this field in the getState() function

Got it, we kept the field name as is in this PR. we use the new s_reserveLinkBalance as the return value in getState() [ref] Does this look good?

we will return 0 once we change the s_reserveLinkBalance to a mapping. very soon, in the next PR!

could we revert this change in this PR? sorry to be nit-picky but this essentially breaks our backwards compatibility and I don't want to forget to fix it in a later PR.

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

RyanRHall · 2024-03-11T17:42:53Z

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

+  error OnlyFinanceAdmin();
+  error TransferFailed();
+  error InsufficientBalance(uint256 available, uint256 requested);


nitpick - can we keep 'em alphabetized :)))

RyanRHall · 2024-03-11T17:43:54Z

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

@@ -276,14 +279,14 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
    address upkeepPrivilegeManager;
    IChainModule chainModule;
    bool reorgProtectionEnabled;
+    address financeAdmin;


could we add a TODO comment to struct pack this better? We can address in a follow up PR.

https://smartcontract-it.atlassian.net/browse/AUTO-9251

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

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

+    _onlyFinanceAdminAllowed();
+    if (to == ZERO_ADDRESS) revert InvalidRecipient();
+
+    bool transferStatus = IERC20(assetAddress).transfer(to, amount);


here, can we check the reserve amount for the taken being transferred?

just to confirm, you meant IERC20(assetAddress).balanceOf(registryMasterAddress) should be smaller? add a require statement here?

If we are on the same page, we actually have foundry tests covering the above:

assertEq(getNonLinkBalance(address(aMockAddress)), 1); assertEq(getNonLinkBalance(address(registryMaster)), 1e10 - 1);

we can do this in the follow up PR, where we convert the reserve amount to a mapping :)

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

RyanRHall · 2024-03-12T16:30:31Z

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

+    //change back to the original address
+    changePrank(originalAddr);


I think this pattern of pranking is a touch verbose - I think we could simplify like this...

// set config with the finance admin setConfigForWithdraw(); // try to withdraw 1 link while there is 0 balance vm.expectRevert(abi.encodeWithSelector(IAutomationRegistryMaster2_3.InsufficientBalance.selector, 0, 1)); vm.prank(FINANCE_ADMIN_ADDRESS); registryMaster.withdrawLinkFees(aMockAddress, 1);

... or ...

// set config with the finance admin setConfigForWithdraw(); vm.startPrank(FINANCE_ADMIN_ADDRESS); // better if we have to make more than 1 call // try to withdraw 1 link while there is 0 balance vm.expectRevert(abi.encodeWithSelector(IAutomationRegistryMaster2_3.InsufficientBalance.selector, 0, 1)); registryMaster.withdrawLinkFees(aMockAddress, 1); vm.stopPrank();

I also wouldn't be opposed to adding a vm.stopPrank() at the end of setup() - this guarantees us a "clean state" at the beginning of every test

github-actions · 2024-03-12T18:51:02Z

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

github-actions · 2024-03-12T18:51:08Z

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

RyanRHall

2 super small requests

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

RyanRHall · 2024-03-12T19:49:04Z

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

@@ -295,7 +298,6 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
   */
  struct State {


could we revert this change in this PR? sorry to be nit-picky but this essentially breaks our backwards compatibility and I don't want to forget to fix it in a later PR.

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

github-actions · 2024-03-12T20:24:13Z

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

github-actions · 2024-03-12T20:24:15Z

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

cl-sonarqube-production · 2024-03-12T21:18:50Z

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

FelixFan1992 · 2024-03-12T23:18:07Z

contracts/src/v0.8/automation/dev/interfaces/v2_3/IAutomationRegistryMaster2_3.sol

  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 OwnerFundsWithdrawn(uint96 amount);


can we keep this event? if not, it won't be compatible with existing atlas.

@cmalec what do you think?

RyanRHall

looks good from my end - can we confirm with @cmalec that changing / removing that OwnerFundsWithdrawn event isn't important?

… refactoring (#12375) * add financeAdmin to onchainConfig * remove ownerLinkbalance and rename expectedLinkBalance * add withdraw functions * add foundry test and generate wrappers

shileiwill requested review from wentzeld and RyanRHall March 11, 2024 16:16

shileiwill temporarily deployed to sdlc March 11, 2024 16:16 — with GitHub Actions Inactive

shileiwill changed the title ~~Some refactoring and Add withdraw functions for finance team~~ Link liquidity pool, add withdraw functions for finance team and some refactoring Mar 11, 2024

RyanRHall suggested changes Mar 11, 2024

View reviewed changes

shileiwill temporarily deployed to sdlc March 11, 2024 20:02 — with GitHub Actions Inactive

shileiwill force-pushed the AUTO-9114-new branch from 46d20bc to 1126f0f Compare March 12, 2024 15:36

shileiwill temporarily deployed to sdlc March 12, 2024 15:36 — with GitHub Actions Inactive

shileiwill added 3 commits March 12, 2024 08:43

add financeAdmin to onchainConfig

931757c

remove ownerLinkbalance and rename expectedLinkBalance

b8b4ed2

add withdraw functions

a4a7ce7

shileiwill force-pushed the AUTO-9114-new branch from 1126f0f to f3add8e Compare March 12, 2024 15:43

shileiwill temporarily deployed to sdlc March 12, 2024 15:43 — with GitHub Actions Inactive

shileiwill marked this pull request as ready for review March 12, 2024 15:44

shileiwill requested review from a team as code owners March 12, 2024 15:44

RyanRHall suggested changes Mar 12, 2024

View reviewed changes

shileiwill force-pushed the AUTO-9114-new branch from f3add8e to 693316c Compare March 12, 2024 16:55

shileiwill temporarily deployed to sdlc March 12, 2024 16:55 — with GitHub Actions Inactive

shileiwill force-pushed the AUTO-9114-new branch from 693316c to 18ce03a Compare March 12, 2024 18:46

shileiwill temporarily deployed to sdlc March 12, 2024 18:46 — with GitHub Actions Inactive

shileiwill temporarily deployed to sdlc March 12, 2024 18:47 — with GitHub Actions Inactive

shileiwill force-pushed the AUTO-9114-new branch from 61bccc0 to 2aaf4cf Compare March 12, 2024 18:53

shileiwill temporarily deployed to sdlc March 12, 2024 18:53 — with GitHub Actions Inactive

shileiwill requested a review from RyanRHall March 12, 2024 18:55

RyanRHall suggested changes Mar 12, 2024

View reviewed changes

shileiwill force-pushed the AUTO-9114-new branch from 7cdad21 to 42a30eb Compare March 12, 2024 20:20

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

add foundry test and generate wrappers

feb1514

shileiwill force-pushed the AUTO-9114-new branch from 0fe802e to feb1514 Compare March 12, 2024 21:04

Merge branch 'develop' into AUTO-9114-new

1cb2016

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

shileiwill requested a review from RyanRHall March 12, 2024 21:05

FelixFan1992 reviewed Mar 12, 2024

View reviewed changes

shileiwill enabled auto-merge March 12, 2024 23:34

RyanRHall approved these changes Mar 13, 2024

View reviewed changes

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

Merged via the queue into develop with commit 831aea8 Mar 13, 2024
109 checks passed

shileiwill deleted the AUTO-9114-new branch March 13, 2024 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link liquidity pool, add withdraw functions for finance team and some refactoring #12375

Link liquidity pool, add withdraw functions for finance team and some refactoring #12375

shileiwill commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

RyanRHall Mar 11, 2024

RyanRHall Mar 11, 2024

shileiwill Mar 11, 2024

shileiwill Mar 12, 2024

RyanRHall Mar 12, 2024

RyanRHall Mar 11, 2024

RyanRHall Mar 11, 2024

shileiwill Mar 11, 2024

RyanRHall Mar 11, 2024

shileiwill Mar 11, 2024

shileiwill Mar 11, 2024 •

edited

Loading

RyanRHall Mar 13, 2024

RyanRHall Mar 12, 2024

RyanRHall Mar 12, 2024

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

RyanRHall left a comment

RyanRHall Mar 12, 2024

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

cl-sonarqube-production bot commented Mar 12, 2024

FelixFan1992 Mar 12, 2024

RyanRHall left a comment

		@@ -295,7 +298,6 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner {
		*/
		struct State {

		//change back to the original address
		changePrank(originalAddr);

Link liquidity pool, add withdraw functions for finance team and some refactoring #12375

Link liquidity pool, add withdraw functions for finance team and some refactoring #12375

Conversation

shileiwill commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

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

shileiwill Mar 11, 2024 • edited Loading

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

github-actions bot commented Mar 12, 2024

RyanRHall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

cl-sonarqube-production bot commented Mar 12, 2024

Quality Gate passed

Choose a reason for hiding this comment

RyanRHall left a comment

Choose a reason for hiding this comment

shileiwill Mar 11, 2024 •

edited

Loading