bonding: Treasury rewards contribution #616

victorges · 2023-07-20T23:21:40Z

What does this pull request do? Explain your changes. (required)
This implements the behavior described in LIP-92, for a logic in BondingManager
to send a percentage of rewards to a treasury contract. This treasury contract
was implemented in #615.

It also supports a balance ceiling which, if reached, will automatically halt treasury contributions until
either the ceiling or the cut rate parameters (or both) are updated again.

Changing the treasuryCutRate also takes place only on the next round, to safeguard againsts
possible games about when to execute a transaction in a round.

Specific updates (required)

Add updateable parameter on BondingManager
Add logic on rewardWithHint and updateTranscoderWithFees to consider treasury contribution
Increment unit tests for the new logic

How did you test each of these updates (required)

yarn test
Devnet on Goerli

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

Checklist:

README and other documentation updated
All tests using yarn test pass

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

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.

100% coverage 😎

"Harness" seems to make more sense, I could only think of that now.

contracts/bonding/BondingManager.sol

victorges · 2023-07-21T19:00:25Z

Btw, not sure what is wrong with the tests. If I run the unit tests by themselves they all work, but they are failing under coverage for some reason. I didn't make changes to the tests that are failing (RoundsManager). Any idea of what could be causing this?

Edit: Found out a flaky test that wasn't awaiting and fixed it later on

Co-authored-by: Chase Adams <[email protected]>

It is a little less exact (might overmint on the last reward call), but the simpler logic might be just worth it.

contracts/bonding/BondingManager.sol

yondonfu · 2023-08-10T16:08:02Z

Btw, not sure what is wrong with the tests.

Not immediately sure, but looks like coverage tests actually started failing in #615.

yarn test:coverage does result in failures locally as well so its not just a CI thing. I tried upgrading to latest hardhat & solidity-coverage locally, but still seeing failures with a couple different ones as well:

1) Rewards
       correctly calculates reward shares for delegators and transcoders:
     Error: Timeout of 100000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/yondonfu/Development/livepeer/protocol/test/integration/Rewards.js)
      at listOnTimeout (node:internal/timers:564:17)
      at processTimers (node:internal/timers:507:7)

  2) Minter
       migrateToNewMinter
         should transfer current token balance and current ETH balance to new minter:
     TypeError: vm.on is not a function
      at Function.fromEvent (node_modules/@defi-wonderland/smock/src/observable-vm.ts:42:8)
      at new ObservableVM (node_modules/@defi-wonderland/smock/src/observable-vm.ts:19:35)
      at new Sandbox (node_modules/@defi-wonderland/smock/src/sandbox.ts:36:15)
      at Function.create (node_modules/@defi-wonderland/smock/src/sandbox.ts:82:12)
      at async init (node_modules/@defi-wonderland/smock/src/index.ts:27:13)
      at async Object.fake (node_modules/@defi-wonderland/smock/src/index.ts:13:17)

  3) Minter
       trustedBurnTokens
         should burn tokens:
     TypeError: vm.on is not a function
      at Function.fromEvent (node_modules/@defi-wonderland/smock/src/observable-vm.ts:42:8)
      at new ObservableVM (node_modules/@defi-wonderland/smock/src/observable-vm.ts:19:35)
      at new Sandbox (node_modules/@defi-wonderland/smock/src/sandbox.ts:36:15)
      at Function.create (node_modules/@defi-wonderland/smock/src/sandbox.ts:82:12)
      at async init (node_modules/@defi-wonderland/smock/src/index.ts:27:13)
      at async Object.fake (node_modules/@defi-wonderland/smock/src/index.ts:13:17)

  4) PollCreator
       "before all" hook in "PollCreator":
     TypeError: vm.on is not a function
      at Function.fromEvent (node_modules/@defi-wonderland/smock/src/observable-vm.ts:42:8)
      at new ObservableVM (node_modules/@defi-wonderland/smock/src/observable-vm.ts:19:35)
      at new Sandbox (node_modules/@defi-wonderland/smock/src/sandbox.ts:36:15)
      at Function.create (node_modules/@defi-wonderland/smock/src/sandbox.ts:82:12)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async init (node_modules/@defi-wonderland/smock/src/index.ts:27:13)
      at async Object.fake (node_modules/@defi-wonderland/smock/src/index.ts:13:17)

  5) RoundsManager
       setRoundLength
         should set roundLength before lastRoundLengthUpdateRound and lastRoundLengthUpdateStartBlock when roundLength = 0:
     AssertionError: wrong lastRoundLengthUpdateRound: expected { Object (_hex, _isBigNumber) } to equal 8
      at /Users/yondonfu/Development/livepeer/protocol/test/unit/RoundsManager.js:94:13
      at step (test/unit/RoundsManager.js:52:23)
      at Object.next (test/unit/RoundsManager.js:33:53)
      at fulfilled (test/unit/RoundsManager.js:24:58)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at runNextTicks (node:internal/process/task_queues:64:3)
      at listOnTimeout (node:internal/timers:533:9)
      at processTimers (node:internal/timers:507:7)

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

…ution

contracts/bonding/BondingManager.sol

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.

contracts/bonding/BondingManager.sol

…ution

Also add docs

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. * treasury: Create treasury governance contracts * test/treasury: Add unit test fro BondingCheckpointsVotes * test/treasury: Test GovernorCountingOverridable * test/treasury: Test LivepeerGovernor * test/treasury: A couple additional Governor tests 100% coverage 😎 * test/treasury: Rename Counting unit test mock "Harness" seems to make more sense, I could only think of that now. * Apply suggestions from code review Co-authored-by: Chase Adams <[email protected]> * treasury: Fix storage layout situation * treasury: Move governor initial params to configs * bonding: Implement treasury contribution * test/bonding: Add tests for treasury contribution * bonding: Update reward cut logic to match LIP It is a little less exact (might overmint on the last reward call), but the simpler logic might be just worth it. * 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 * treasury: Migrate to the new BondingVotes contract * treasury: Address PR comments * bonding: Move constructor to after modifiers Just for consistency with other contracts. Some docs as a bonus * bonding: Avoid returning payable treasury address * bonding: Update treasury cut rate only on the next round * test: Fix TicketBroker tests flakiness! * test/mocks: Remove mock functions that moved to other mock * 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 * treasury: Merge BondingCheckpoints and nits * bonding: Move internal func to the right section Also add docs --------- Co-authored-by: Chase Adams <[email protected]>

victorges added 13 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

test/bonding: Test BondingManager and Checkpoints

078f348

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

bonding: Migrate to custom error types

22ee2db

treasury: Create treasury governance contracts

06b7100

test/treasury: Add unit test fro BondingCheckpointsVotes

0aa6cd0

test/treasury: Test GovernorCountingOverridable

9f007ba

test/treasury: Test LivepeerGovernor

2fd7abb

test/treasury: A couple additional Governor tests

34cb455

100% coverage 😎

test/treasury: Rename Counting unit test mock

e970472

"Harness" seems to make more sense, I could only think of that now.

victorges changed the title ~~bonding: Implement treasury contribution~~ bonding: Treasury rewards contribution Jul 20, 2023

victorges force-pushed the vg/feat/treasury-contribution branch from 5643596 to f580f29 Compare July 21, 2023 01:02

victorges marked this pull request as ready for review July 21, 2023 01:07

victorges requested review from yondonfu and 0xcadams July 21, 2023 01:07

victorges commented Jul 21, 2023

View reviewed changes

contracts/bonding/BondingManager.sol Show resolved Hide resolved

victorges and others added 6 commits July 26, 2023 15:56

Apply suggestions from code review

a6db562

Co-authored-by: Chase Adams <[email protected]>

treasury: Fix storage layout situation

0ad3a8f

treasury: Move governor initial params to configs

bb9ff45

bonding: Implement treasury contribution

6984ede

test/bonding: Add tests for treasury contribution

9aad9e2

bonding: Update reward cut logic to match LIP

cd6d147

It is a little less exact (might overmint on the last reward call), but the simpler logic might be just worth it.

victorges force-pushed the vg/feat/treasury-contribution branch from 90f63e5 to cd6d147 Compare August 4, 2023 16:00

yondonfu reviewed Aug 10, 2023

View reviewed changes

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

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

contracts/bonding/BondingManager.sol Show resolved Hide resolved

bonding: Make sure we checkpoint up to once per op

0c41f78

victorges added 5 commits August 17, 2023 17:01

Merge branch 'vg/feat/bonding-checkpoints' into vg/feat/onchain-governor

4f12ce9

treasury: Migrate to the new BondingVotes contract

e9d3339

treasury: Address PR comments

f0fb8ee

bonding: Move constructor to after modifiers

9dbb0ae

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

Merge branch 'vg/feat/onchain-governor' into vg/feat/treasury-contrib…

cf12cb4

…ution

victorges force-pushed the vg/feat/treasury-contribution branch from 157fe9c to cf12cb4 Compare August 18, 2023 00:09

victorges added 2 commits August 17, 2023 22:46

bonding: Avoid returning payable treasury address

27f8930

bonding: Update treasury cut rate only on the next round

ead1edb

victorges commented Aug 18, 2023

View reviewed changes

contracts/bonding/BondingManager.sol Show resolved Hide resolved

test: Fix TicketBroker tests flakiness!

391e444

victorges force-pushed the vg/feat/treasury-contribution branch from 97ff8eb to 391e444 Compare August 18, 2023 17:12

victorges added 2 commits August 18, 2023 14:32

test/mocks: Remove mock functions that moved to other mock

b59a538

victorges mentioned this pull request Aug 22, 2023

bonding: Implement active stake checkpointing #614

Merged

2 tasks

bonding: Address PR comments

fecf5a2

yondonfu reviewed Aug 23, 2023

View reviewed changes

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

contracts/bonding/BondingManager.sol Show resolved Hide resolved

contracts/bonding/BondingManager.sol Show resolved Hide resolved

victorges added 5 commits August 23, 2023 17:03

bonding: Address BondingVotes review comments

4246474

Merge branch 'vg/feat/bonding-checkpoints' into vg/feat/onchain-governor

63d1d19

treasury: Merge BondingCheckpoints and nits

8c57a4d

Merge branch 'vg/feat/onchain-governor' into vg/feat/treasury-contrib…

e2a2ee7

…ution

bonding: Move internal func to the right section

a65a5b0

Also add docs

yondonfu approved these changes Aug 25, 2023

View reviewed changes

Base automatically changed from vg/feat/onchain-governor to delta August 25, 2023 22:11

Merge branch 'delta' into vg/feat/treasury-contribution

48a9230

victorges merged commit 5c4a369 into delta Aug 25, 2023

victorges deleted the vg/feat/treasury-contribution branch August 25, 2023 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bonding: Treasury rewards contribution #616

bonding: Treasury rewards contribution #616

victorges commented Jul 20, 2023 •

edited

Loading

victorges commented Jul 21, 2023 •

edited

Loading

yondonfu commented Aug 10, 2023

yondonfu left a comment

bonding: Treasury rewards contribution #616

bonding: Treasury rewards contribution #616

Conversation

victorges commented Jul 20, 2023 • edited Loading

victorges commented Jul 21, 2023 • edited Loading

yondonfu commented Aug 10, 2023

yondonfu left a comment

Choose a reason for hiding this comment

victorges commented Jul 20, 2023 •

edited

Loading

victorges commented Jul 21, 2023 •

edited

Loading