bonding: Implement active stake checkpointing #614

victorges · 2023-06-17T00:13:53Z

What does this pull request do? Explain your changes. (required)
This is to introduce a checkpointing logic in bonding manager so that we can lookup historical information about any point in time. We need this in order to support an on-chain governance system with support for OpenZeppelin Governor abstractions. That is why it also implements the IERC5808 interface to plug into OZ Governor framework.

It is implemented as "external" to BondingManager as possible in a separate BondingVotes contract, but there were a couple places where BondingManager needed changes to start checkpointing its state.

Specific updates (required)

Implement historical active stake snapshotting logic in the BondingVotes contract
Make BondingManager call it to checkpoint the state where it needs to
Tests for the checkpointing logic (unit / integration / fork)

How did you test each of these updates (required)

Unit and integration tests with yarn test
Fork test with forge
Devnet on Goerli

Does this pull request close any open issues?
Implements PRO-28

Checklist:

README and other documentation updated
All tests using yarn test pass

contracts/bonding/BondingManager.sol

test/unit/BondingCheckpoints.js

contracts/bonding/BondingCheckpoints.sol

Will be required for new checkpoints code and later governor implementation.

Used for checkpointing logic in bonding state checkpoints

Handles historic checkpointing ("snapshotting") and lookup of the bonding state.

- unit tests - integration tests - gas-report - fork test for upgrade

victorges · 2023-07-14T06:49:06Z

@yondonfu @0xcadams Ok I think NOW this one is ready to go! :)

This will provide a cleaner experience using governance tools, since users that don't have any stake won't get errors when querying their voting power. For users that do have stake, we will make sure to checkpoint them on first deploy.

victorges · 2023-07-17T17:23:12Z

Ran the gas report on confluence branch (current) compared to this (checkpoints). Here are the reports, it seems to have increased around 200k gas on the bond/unbond functions due to the checkpoints that have been made. The checkpoint initialization itself also costs around 170k gas, which should still be relatively cheap on Arbitrum, but I haven't found a way to reliably calculate how it would cost considering the calldata cost on L1.

gas-report-checkpoints.txt
gas-report-current.txt

contracts/bonding/BondingCheckpoints.sol

0xcadams

Preliminary review of code quality & gas improvements, no deep checks of logic

contracts/bonding/BondingManager.sol

contracts/bonding/BondingCheckpoints.sol

contracts/bonding/libraries/SortedArrays.sol

contracts/bonding/BondingCheckpoints.sol

contracts/bonding/BondingManager.sol

contracts/bonding/BondingCheckpoints.sol

contracts/bonding/BondingManager.sol

contracts/bonding/BondingCheckpoints.sol

contracts/bonding/BondingManager.sol

contracts/bonding/BondingCheckpoints.sol

Meaning the start of next round instead of the current round. This is more compatible with the way OZ expects the timepoints to work in the clock and snapshots on the Governor framework.

This changes the BondingVotes implementation to stop having so many reverts on supposedly invalid states. Instead, redefine the voting power (and checkpointed stake) as being zero before the first round to be checkpointed. This had other implications in the code like removing changes in BondingManager to make the state complete (always having an earnings pool on lastClaimRound). Instead, the BondingVotes implementaiton is resilient to that as well and just assumes reward() had never been called. This also fixed the redundant reverts between BondingVotes and SortedArrays, as now there are almost no reverts.

victorges · 2023-08-17T16:49:21Z

OK @yondonfu @0xcadams I've addressed the feedback on your comments and this is ready for another review round. I have included some other changes as well that seemed necessary after the rounds of testing with the testnet and some discussions.

I recommend reviewing commit by commit, I avoided making a force push here not to lose history, and also isolated the less trivial changes in their own commits. As an overview of each:

0c41f78: This addresses the comments about multiple checkpoints being made for the same account in the same transaction. It makes sure we only call it once in the end of all mutations instead on all code paths.
d518cd6: This merges the BondingCheckpointsVotes contract from treasury: Delta Governor v1 #615 into the BondingCheckpoints contract here (renamed to just BondingVotes). This is mainly to allow it to send the ERC5805 events on changes.
ccc3ddf: This is a change on the way that we assign voting power to accounts, to use their active stake at the end of the round instead of start. I didn't change any of the core checkpointing logic, just changed the voting power functions to read state from the next round instead, also using the nextRoundTotalActiveStake when querying for the next round to the current one.
- After further testing with the OZ governor, I came to understand that their definition of "voting power at a timepoint" actually refers to the voting power at the end of that timepoint. This makes sense if you think about using block numbers as the clock, and is also reflected in the way that the OZ Governor works for some things like vote snapshotting: voting starts only after the "snapshot round", not on it.
- A practical effect of this is that, in the previous implementation, if we had a votingDelay of 1 round the voting would be delayed in 1-2 rounds, but users would only really have 0-1 rounds to make any staking changes. Any staking changes made on the round before the voting actually starts would not be effective, since we would take the active stake from the start of that round for the voting power. Now we take it from the end (start of next round), and things work a bit more as expected.
d5062fe: This changes the behavior of most of the "read" logic in the BondingVotes contract to avoid reverting queries that could simply return a 0 value. There is only 1 specific case now where we will revert instead of returning zero, which is when there is no earnings pool on a transcoder's last reward round, which is an illegal state from the way that BondingManager works. This also fixed a lot of the redundant reverts with the SortedArrays implementation.
292f954: This one addresses the other review comments which were simpler so I just combined them in this commit.

contracts/test/mocks/BondingVotesERC5805Harness.sol

Just for consistency with other contracts. Some docs as a bonus

This is to increase compatibility with some tools out there like Tally, which require only these functions from the ERC20 spec, not the full implementation. So we can have these from the get-go to make things easier if we want to make something with them.

yondonfu

Still need to do the follow up for BondingVotes.sol.

contracts/bonding/BondingManager.sol

contracts/bonding/BondingVotes.sol

contracts/bonding/IBondingVotes.sol

contracts/bonding/BondingVotes.sol

src/test/BondingVotesStateInitialization.sol

contracts/bonding/BondingVotes.sol

yondonfu

LGTM

* package.json: Update @OpenZeppelin libraries Will be required for new checkpoints code and later governor implementation. * bonding: Create SortedArrays library Used for checkpointing logic in bonding state checkpoints * bonding: Create BondingCheckpoints contract Handles historic checkpointing ("snapshotting") and lookup of the bonding state. * bonding: Checkpoint bonding state on changes * test/bonding: Test BondingManager and Checkpoints - unit tests - integration tests - gas-report - fork test for upgrade * bonding: Migrate to custom error types * bonding: Allow querying unbonded+uncheckpointed accounts This will provide a cleaner experience using governance tools, since users that don't have any stake won't get errors when querying their voting power. For users that do have stake, we will make sure to checkpoint them on first deploy. * bonding: Make sure we checkpoint up to once per op * bonding: Make bonding checkpoints implement IVotes * bonding: Read votes from the end of the round Meaning the start of next round instead of the current round. This is more compatible with the way OZ expects the timepoints to work in the clock and snapshots on the Governor framework. * bonding: Make checkpoint reading revert-free This changes the BondingVotes implementation to stop having so many reverts on supposedly invalid states. Instead, redefine the voting power (and checkpointed stake) as being zero before the first round to be checkpointed. This had other implications in the code like removing changes in BondingManager to make the state complete (always having an earnings pool on lastClaimRound). Instead, the BondingVotes implementaiton is resilient to that as well and just assumes reward() had never been called. This also fixed the redundant reverts between BondingVotes and SortedArrays, as now there are almost no reverts. * bonding: Address minor code review comments * bonding: Move constructor to after modifiers Just for consistency with other contracts. Some docs as a bonus * bonding: Implement ERC20 metadata on votes This is to increase compatibility with some tools out there like Tally, which require only these functions from the ERC20 spec, not the full implementation. So we can have these from the get-go to make things easier if we want to make something with them. * bonding: Address PR comments * bonding: Address BondingVotes review comments * test/bonding: Document IVotes layer tests

victorges requested review from yondonfu and 0xcadams June 17, 2023 00:13

victorges force-pushed the vg/feat/bonding-checkpoints branch 6 times, most recently from b6de577 to 91547a8 Compare June 17, 2023 01:28

victorges commented Jun 19, 2023

View reviewed changes

contracts/bonding/BondingManager.sol Outdated Show resolved Hide resolved

victorges force-pushed the vg/feat/bonding-checkpoints branch 2 times, most recently from b82f36d to 3b49303 Compare June 20, 2023 15:29

victorges marked this pull request as ready for review June 20, 2023 16:48

victorges commented Jun 20, 2023

View reviewed changes

test/unit/BondingCheckpoints.js Outdated Show resolved Hide resolved

victorges commented Jun 20, 2023

View reviewed changes

contracts/bonding/BondingCheckpoints.sol Outdated Show resolved Hide resolved

victorges marked this pull request as draft June 28, 2023 23:11

victorges added 4 commits July 7, 2023 17:51

package.json: Update @OpenZeppelin libraries

cc315d1

Will be required for new checkpoints code and later governor implementation.

bonding: Create SortedArrays library

d42e92c

Used for checkpointing logic in bonding state checkpoints

bonding: Create BondingCheckpoints contract

2abf207

Handles historic checkpointing ("snapshotting") and lookup of the bonding state.

bonding: Checkpoint bonding state on changes

a9dde9d

victorges force-pushed the vg/feat/bonding-checkpoints branch 2 times, most recently from 4915ecb to 09a2d51 Compare July 13, 2023 05:02

test/bonding: Test BondingManager and Checkpoints

078f348

- unit tests - integration tests - gas-report - fork test for upgrade

victorges force-pushed the vg/feat/bonding-checkpoints branch from 09a2d51 to 078f348 Compare July 14, 2023 06:48

victorges marked this pull request as ready for review July 14, 2023 06:48

bonding: Migrate to custom error types

22ee2db

victorges force-pushed the vg/feat/bonding-checkpoints branch from bc23af0 to 22ee2db Compare July 17, 2023 03:18

victorges mentioned this pull request Jul 20, 2023

treasury: Delta Governor v1 #615

Merged

2 tasks

victorges commented Jul 25, 2023

View reviewed changes

contracts/bonding/BondingCheckpoints.sol Outdated Show resolved Hide resolved

0xcadams reviewed Jul 26, 2023

View reviewed changes

yondonfu requested changes Aug 1, 2023

View reviewed changes

victorges added 5 commits August 15, 2023 17:54

bonding: Make sure we checkpoint up to once per op

0c41f78

bonding: Make bonding checkpoints implement IVotes

d518cd6

bonding: Read votes from the end of the round

ccc3ddf

Meaning the start of next round instead of the current round. This is more compatible with the way OZ expects the timepoints to work in the clock and snapshots on the Governor framework.

bonding: Address minor code review comments

292f954

victorges force-pushed the vg/feat/bonding-checkpoints branch from 13c4caf to 292f954 Compare August 17, 2023 15:22

victorges commented Aug 17, 2023

View reviewed changes

contracts/test/mocks/BondingVotesERC5805Harness.sol Show resolved Hide resolved

victorges added 2 commits August 17, 2023 20:31

bonding: Move constructor to after modifiers

9dbb0ae

Just for consistency with other contracts. Some docs as a bonus

yondonfu reviewed Aug 22, 2023

View reviewed changes

bonding: Address PR comments

fecf5a2

victorges commented Aug 23, 2023

View reviewed changes

contracts/bonding/IBondingVotes.sol Outdated Show resolved Hide resolved

yondonfu reviewed Aug 23, 2023

View reviewed changes

victorges commented Aug 23, 2023

View reviewed changes

contracts/bonding/BondingVotes.sol Outdated Show resolved Hide resolved

victorges commented Aug 23, 2023

View reviewed changes

src/test/BondingVotesStateInitialization.sol Outdated Show resolved Hide resolved

victorges commented Aug 23, 2023

View reviewed changes

contracts/bonding/BondingVotes.sol Outdated Show resolved Hide resolved

bonding: Address BondingVotes review comments

4246474

yondonfu reviewed Aug 25, 2023

View reviewed changes

contracts/bonding/BondingVotes.sol Show resolved Hide resolved

yondonfu approved these changes Aug 25, 2023

View reviewed changes

victorges changed the base branch from confluence to delta August 25, 2023 21:39

test/bonding: Document IVotes layer tests

e602ab9

victorges merged commit 4d720f0 into delta Aug 25, 2023
2 checks passed

victorges deleted the vg/feat/bonding-checkpoints branch August 25, 2023 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bonding: Implement active stake checkpointing #614

bonding: Implement active stake checkpointing #614

victorges commented Jun 17, 2023 •

edited

Loading

victorges commented Jul 14, 2023

victorges commented Jul 17, 2023 •

edited

Loading

0xcadams left a comment

victorges commented Aug 17, 2023

yondonfu left a comment

yondonfu left a comment

bonding: Implement active stake checkpointing #614

bonding: Implement active stake checkpointing #614

Conversation

victorges commented Jun 17, 2023 • edited Loading

victorges commented Jul 14, 2023

victorges commented Jul 17, 2023 • edited Loading

0xcadams left a comment

Choose a reason for hiding this comment

victorges commented Aug 17, 2023

yondonfu left a comment

Choose a reason for hiding this comment

yondonfu left a comment

Choose a reason for hiding this comment

victorges commented Jun 17, 2023 •

edited

Loading

victorges commented Jul 17, 2023 •

edited

Loading