Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIP-78a: Metagovernance - ANGLE #529

Merged
merged 49 commits into from Mar 7, 2022
Merged

FIP-78a: Metagovernance - ANGLE #529

merged 49 commits into from Mar 7, 2022

Conversation

eswak
Copy link
Contributor

@eswak eswak commented Feb 8, 2022

Forum: https://tribe.fei.money/t/fip-78a-meta-governance-angle-angle/3924
Snapshot: https://snapshot.org/#/fei.eth/proposal/0x6fb9e4bc23247f5f65da9a57577bee83b8dc748a21c5102ed1401bbcde720908

The purpose of this PR is to enable meta-governance with the protocol-owned governance tokens :

  • ANGLE
  • INDEX (already live)

SnapshotDelegatorPCVDeposit

This contract existed already, I just moved it to a new metagov folder. It is now using the new ACL system with METAGOVERNANCE_VOTE_ADMIN role.

Used for INDEX (already deployed on-chain)

AngleDelegatorPCVDeposit

This contract is a SnapshotDelegatorPCVDeposit that can also vote-escrow tokens (VoteEscrowTokenManager), vote and propose on an OZ governor (OZGovernorVoter), and stake tokens in gauges and claim their rewards (LiquidityGaugeManager).

Angle deprecated their OZ governor, but I wanted to include that feature in the immutable contract, because they may reinstate a governor in the future.

Used for ANGLE.

GaugeLens

Very simple "lens" that reports the balance of a contract staked in a gauge

UniswapLens

Manipulation-resistant balance and FEI contained within LP tokens. Uses the same formula as in UniswapPCVDeposit.

ACL

This PR uses the new ACL system with TribeRoles.sol and the onlyRole modifier. New roles:
image

FEI can be pre-minted on PCVDeposit before calling deposit()
Also set METAGOV_ADMIN_ROLE as the default contract admin role
/// @dev permissionless: anyone can lock tokens and extend the lock
/// duration at anytime. The only way to withdraw tokens will be to
/// pause this contract for MAXTIME and then call exitLock().
function lock() external whenNotPaused {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be behind a low-clearance VOTE_ESCROW role imo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a METAGOVERNANCE_TOKEN_STAKING role, that we'll use for AAVE/stkAAVE and CVX/vlCVX too. The mechanisms are very similar.

) VoteEscrowTokenManager(
IERC20(0x31429d1856aD1377A8A0079410B297e1a9e214c2), // ANGLE
IVeToken(0x0C462Dbb9EC8cD1630f1728B2CFD2769d09f0dd5), // veANGLE
4 * 365 * 86400 // 4 years
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use solidity keyword day instead of putting 86,400

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep the same exact wording as in the veANGLE token's code if you don't mind

@ElliotFriedman
Copy link
Contributor

It would be great to see unit tests on all this new solidity

@eswak
Copy link
Contributor Author

eswak commented Feb 14, 2022

It would be great to see unit tests on all this new solidity

I added a lot of e2e tests for the various new contracts, they interact with real on-chain governors, gauges, and veTokens, to validate the behavior

Copy link
Contributor

@thomas-waite thomas-waite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main comments:

  • Question about whether we are effectively going to be continuously locking tokens with this implementation
  • Would break out the parser/debug mode you've added into the e2e tests to a seperate PR. Will reduce this PR size + make it easier to go through the parser/debug mode. Not sure adding a new ProposalCategory of DEBUG is the optimal way to do it
  • Would break out the metagov.ts tests into several different test files with a parent metagov directory. Will reduce the file size and make it easier to review

On a seperate note, I know the e2e tests for this work have been taking a long time to run and made development painful. In the Backup Oracles PR, I've added forge based integration tests for development. They run significantly faster than our TypeScript e2e tests, and will likely be a better way to test contracts that interact with other protocols as a pose to e2e testing our whole protocol

contracts/metagov/AngleDelegatorPCVDeposit.sol Outdated Show resolved Hide resolved
contracts/metagov/utils/LiquidityGaugeManager.sol Outdated Show resolved Hide resolved
contracts/metagov/utils/LiquidityGaugeManager.sol Outdated Show resolved Hide resolved
contracts/metagov/utils/VoteEscrowTokenManager.sol Outdated Show resolved Hide resolved
test/integration/setup/index.ts Outdated Show resolved Hide resolved
test/integration/setup/index.ts Outdated Show resolved Hide resolved
test/integration/tests/metagov.ts Outdated Show resolved Hide resolved
test/integration/tests/metagov.ts Outdated Show resolved Hide resolved
test/integration/tests/metagov.ts Outdated Show resolved Hide resolved
Comment on lines 34 to 37
METAGOVERNANCE_VOTE_ADMIN: ['feiDAOTimelock', 'opsOptimisticTimelock'],
METAGOVERNANCE_TOKEN_STAKING: ['feiDAOTimelock', 'opsOptimisticTimelock'],
METAGOVERNANCE_GAUGE_ADMIN: ['feiDAOTimelock', 'opsOptimisticTimelock'],
METAGOVERNANCE_GAUGE_STAKING: ['feiDAOTimelock', 'opsOptimisticTimelock']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are any of these higher clearance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arguably METAGOVERNANCE_GAUGE_ADMIN is higher risk because it can stake the protocol's LP tokens held on the contract on an arbitrary contract (it can whitelist a gauge & then stake in it), so I'll move it to the optimisticTimelock that has a longer duration

VoteEscrowTokenManager: rename _totalTokens() to _totalTokensManaged()
Merge METAGOVERNANCE_GAUGE_STAKING and METAGOVERNANCE_GAUGE_ADMIN roles, only keep METAGOVERNANCE_GAUGE_ADMIN.

Change METAGOVERNANCE_GAUGE_ADMIN reciption to OA timelock (not OPS OA timelock)
@eswak eswak self-assigned this Feb 27, 2022
bytes32 internal constant METAGOVERNANCE_GAUGE_ADMIN = keccak256("METAGOVERNANCE_GAUGE_ADMIN");

/// @notice manages staking to & unstaking from gauges of the protocol's tokens
bytes32 internal constant METAGOVERNANCE_GAUGE_STAKING = keccak256("METAGOVERNANCE_GAUGE_STAKING");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this role can be deleted

@fei-protocol fei-protocol deleted a comment from Joeysantoro Feb 28, 2022
@eswak eswak requested a review from a team as a code owner March 7, 2022 14:24
Copy link
Contributor

@thomas-waite thomas-waite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@eswak eswak merged commit 6f211da into develop Mar 7, 2022
@eswak eswak deleted the feat/metagov-angle branch March 7, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants