[TIP-120] DAO Vote: PCVGuardian v3 #952

eswak · 2022-07-04T14:00:29Z

A new version of the PCV Guardian :

Adds **Ratio functions to allow to move percentages with withdraw, withdrawETH, and withdrawERC20
Rewrite all tests in Forge
Factorize the code that is repeated in all withdraw functions inside a modifier (access control, check if target is a safe address, unpause source deposit if paused, deposit after & pause after boolean parameters handling)
Changed the default behavior : if the source deposit is paused before the call, re-pause it after withdrawal even if pauseAfter = false
Very small gas optimization by changing order of role checks because most of the time, TC Timelock (pcv safe mover role) or Guardian (guardian role) will perform the calls, and not the DAO Timelock (governor role), and the order of checks can save some external calls to core.hasRole(role, address).
All Tests Passing
Remove Any .only's on Tests

thomas-waite

Really like these changes overall, will be a big quality of life improvement.

Have some feedback on the unit tests - in general I think they should be broken up into many more and each major function should be covered by its own test. Also think the test names could be more descriptive/illustrative

contracts/pcv/IPCVGuardian.sol

contracts/pcv/PCVGuardian.sol

contracts/test/unit/PCVGuardian.sol

rename fixture addresses & fix podexecutor unit tests

proposals/description/pcv_guardian_v3.ts

xklob · 2022-07-12T02:27:39Z

proposals/description/pcv_guardian_v3.ts

+      target: 'pcvSentinel',
+      values: '0',
+      method: 'knight(address)',
+      arguments: (addresses) => [addresses.fuseWithdrawalGuard],


Really confusing that this one has no version number, and the old has a v1 version number. Thoughts on just using v2 for the new version number?

we argued about this with @thomas-waite in the dev chat several days ago, the most up to date version doesn't have a version number, otherwise you don't know if there is a V3 you should use instead, when you use the V2

proposals/description/pcv_guardian_v3.ts

xklob · 2022-07-12T02:29:45Z

contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol

@@ -54,7 +54,9 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib {
        sentinel.knight(address(guard));
    }

-    function testGuardCanProtec() public {
+    // TODO: update Fuse withdrawal guard PCV_GUARDIAN address,


Commenting for this todo as a reminder.

thanks can only be fixed after deployment

xklob · 2022-07-12T02:30:01Z

contracts/sentinel/guards/FuseWithdrawalGuard.sol

@@ -22,7 +22,7 @@ contract FuseWithdrawalGuard is IGuard, CoreRef {
    EnumerableSet.AddressSet private fuseDeposits;

    /// @notice the PCV mover contract exposed to guardian role
-    PCVGuardian public constant pcvGuardian = PCVGuardian(0x02435948F84d7465FB71dE45ABa6098Fc6eC2993);
+    PCVGuardian public constant pcvGuardian = PCVGuardian(0x0000000000000000000000000000000000000000); // TODO update


Commenting to remind of todo.

thanks can only be fixed after deployment

This guardian address should be injected in at constructor time no? Otherwise I imagine it's impossible to test this

xklob · 2022-07-12T02:30:32Z

contracts/pcv/PCVGuardian.sol

-            pcvDeposit._pause();
-        }
-
-        if (depositAfter) {


I think we agreed to leave depositAfter in?

yes we kept depositAfter, the diff is displaying very weird 🤔 this logic is now in the modifier

xklob · 2022-07-12T02:30:56Z

contracts/pcv/PCVGuardian.sol

-    ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) {
-        require(isSafeAddress(safeAddress), "Provided address is not a safe address!");
-
-        pcvDeposit._ensureUnpaused();


Do we not need this to withdraw from paused deposits?

we do, that's why before withdrawals the modifier reads the paused state & unpauses if the deposit is paused

xklob · 2022-07-12T02:31:16Z

contracts/pcv/PCVGuardian.sol


-        pcvDeposit._ensureUnpaused();
+        bool paused = pcvDeposit._paused();
+        if (paused) {


Why not just use ensureUnpaused here?

ensureUnpaused reads the pcvDeposit.paused() but does not return it. We need to read the paused state before withdrawal, because after withdrawal we want to re-pause if the deposit was paused in the first place. But yeah essentially it's the same code than what's in the ensureUnpaused function

eswak · 2022-07-13T09:51:13Z

I moved the modifier function to be at the bottom of the file, the diff displays much more nicely now

thomas-waite

Looks good overall to me.

Idea about using _beforeWithdrawal() and _afterWithdrawal() hooks instead of a modifier to reduce control flow switching, but no strong feelings.

Also think the FuseWithdrawalGuard should have the PCVGuardian address injected in through it's constructor, otherwise it's impossible to test new pcvGuardians with it before deploying them.

thomas-waite · 2022-07-13T10:59:58Z

contracts/pcv/PCVGuardian.sol


-        if (pauseAfter) {
+        _;


No strong feelings and fine as is, but not a fan of having the control flow jumping about in this modifier. The modifier runs before the function executes and checks permissions etc., then passes the control flow back to the function on line 223, then will take the control flow back once the function is done.

Might be cleaner and a nicer seperation of before and after logic if this modifier was split out into two hooks: _beforeWithdrawal() and _afterWithdrawal() which then get called in the function body

I think the modifier is better because we want the paused stated read before withdrawal to be used after withdrawal (to re-pause), we can't do that without modifier

thomas-waite · 2022-07-13T11:18:03Z

contracts/sentinel/guards/FuseWithdrawalGuard.sol

@@ -22,7 +22,7 @@ contract FuseWithdrawalGuard is IGuard, CoreRef {
    EnumerableSet.AddressSet private fuseDeposits;

    /// @notice the PCV mover contract exposed to guardian role
-    PCVGuardian public constant pcvGuardian = PCVGuardian(0x02435948F84d7465FB71dE45ABa6098Fc6eC2993);
+    PCVGuardian public constant pcvGuardian = PCVGuardian(0x0000000000000000000000000000000000000000); // TODO update


This guardian address should be injected in at constructor time no? Otherwise I imagine it's impossible to test this

thomas-waite · 2022-07-13T11:20:01Z

contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol.disabled

@@ -54,7 +54,9 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib {
        sentinel.knight(address(guard));
    }

-    function testGuardCanProtec() public {
+    // TODO: update Fuse withdrawal guard PCV_GUARDIAN address,


I think the guardian should be deployed locally in this integration test and then it's address be injected into the FuseWithdrawalGuard through it's constructor, otherwise you can't test the guard without deploying something to mainnet first

thomas-waite · 2022-07-13T11:21:11Z

protocol-configuration/proposalsConfig.ts

+    proposalId: '',
+    affectedContractSignoff: [],
+    deprecatedContractSignoff: [],
+    category: ProposalCategory.DEBUG


Will want to change this proposal category to TC I imagine?

yep, good catch

actually has to be submitted to DAO because it revokes & grant Guardian & PCV Controller

thomas-waite · 2022-07-13T11:21:46Z

proposals/description/pcv_guardian_v3.ts

+import { TemplatedProposalDescription } from '@custom-types/types';
+
+const proposal: TemplatedProposalDescription = {
+  title: 'TIP-XYZ: PCV Guardian v3',


Will need a TIP number

I'll take... TIP-120 !

thomas-waite · 2022-07-13T11:25:21Z

proposals/dao/pcv_guardian_v3.ts

+// Run any validations required on the fip using mocha or console logging
+// IE check balances, check state of contracts, etc.
+const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  // display pcvStats


Being paranoid I would probably make sure that we can move funds here using the newly deployed pcvGuardianV3

Update PCVGuardian to v3 and add forge tests

04c43b8

eswak force-pushed the feat/pcvguardian-v3 branch from 52ef5f1 to 04c43b8 Compare July 4, 2022 14:08

eswak marked this pull request as ready for review July 4, 2022 14:15

eswak requested a review from a team as a code owner July 4, 2022 14:15

thomas-waite reviewed Jul 4, 2022

View reviewed changes

eswak added 3 commits July 5, 2022 17:26

tom's review

5d37dbd

Merge remote-tracking branch 'origin/develop' into feat/pcvguardian-v3

8546f7b

rename fixture addresses & fix podexecutor unit tests

915d275

eswak linked an issue Jul 6, 2022 that may be closed by this pull request

PCVGuardianV3 with ratio movements #954

Open

eswak added the feature label Jul 7, 2022

eswak added 3 commits July 8, 2022 11:48

Update PCVGuardian to v3 and add forge tests

1dbaf4b

tom's review

531415d

PCVGuardian v3: remove pauseAfter param

9428149

eswak force-pushed the feat/pcvguardian-v3 branch from f030292 to 9428149 Compare July 8, 2022 09:49

eswak and others added 5 commits July 8, 2022 12:16

reorder proposals

5508ba7

Merge branch 'feat/pcvguardian-v3' into qol/forge-fixture-rename

15374db

finish resolving merge conflicts

fef615c

Merge pull request #957 from fei-protocol/qol/forge-fixture-rename

283adc1

rename fixture addresses & fix podexecutor unit tests

Merge remote-tracking branch 'origin/develop' into feat/pcvguardian-v3

b074e98

xklob reviewed Jul 12, 2022

View reviewed changes

eswak added 3 commits July 13, 2022 11:34

klob's review

f59d04b

attempt to make diff more friendly

8752fb3

Merge remote-tracking branch 'origin/develop' into feat/pcvguardian-v3

ce69cf3

thomas-waite reviewed Jul 13, 2022

View reviewed changes

eswak added 3 commits July 13, 2022 16:20

tom's review

4787aeb

Merge remote-tracking branch 'origin/develop' into feat/pcvguardian-v3

f0c0444

Update pcv_guardian_v3 to be executed by DAO

a8ee388

eswak changed the title ~~PCVGuardian v3~~ [TIP-120] DAO Vote: PCVGuardian v3 Jul 13, 2022

Joeysantoro closed this Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIP-120] DAO Vote: PCVGuardian v3 #952

[TIP-120] DAO Vote: PCVGuardian v3 #952

eswak commented Jul 4, 2022 •

edited

Loading

thomas-waite left a comment

xklob Jul 12, 2022

eswak Jul 13, 2022

xklob Jul 12, 2022

eswak Jul 13, 2022

xklob Jul 12, 2022

eswak Jul 13, 2022

thomas-waite Jul 13, 2022

xklob Jul 12, 2022

eswak Jul 13, 2022

xklob Jul 12, 2022

eswak Jul 13, 2022

xklob Jul 12, 2022

eswak Jul 13, 2022 •

edited

Loading

eswak commented Jul 13, 2022

thomas-waite left a comment

thomas-waite Jul 13, 2022

eswak Jul 13, 2022

thomas-waite Jul 13, 2022

thomas-waite Jul 13, 2022

eswak Jul 13, 2022

thomas-waite Jul 13, 2022

eswak Jul 13, 2022

eswak Jul 13, 2022

thomas-waite Jul 13, 2022

eswak Jul 13, 2022 •

edited

Loading

thomas-waite Jul 13, 2022

eswak Jul 13, 2022

[TIP-120] DAO Vote: PCVGuardian v3 #952

[TIP-120] DAO Vote: PCVGuardian v3 #952

Conversation

eswak commented Jul 4, 2022 • edited Loading

thomas-waite 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

eswak Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

eswak commented Jul 13, 2022

thomas-waite 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

eswak Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eswak commented Jul 4, 2022 •

edited

Loading

eswak Jul 13, 2022 •

edited

Loading

eswak Jul 13, 2022 •

edited

Loading