Allow a specified address to disable/enable rewards distribution #1828

m-chrzan · 2019-11-21T16:30:21Z

Description

For Baklava, we want the organizer to be able to turn off rewards distribution. This PR uses the previously introduced Freezable interface to make updateTargetVotingYield freezable. Celo-blockchain will, upon reverting on calling the frozen function, produce an epoch block without distributed rewards.

Tested

Unit and e2e tests.

Other changes

Reorganized helper functions in e2e governance test.

Related issues

Fixes Stake off admin SBAT enable/disable rewards and exchange #1431

Backwards compatibility

Not backwards compatible, smart contract has new storage layout.

nategraf

#1431 mentions being able to freeze the exchange, so I've edited the description to make #1431 as related, but not fixed.

nategraf · 2019-11-22T21:16:05Z

packages/protocol/migrations/14_epoch_rewards.ts

  return [
    config.registry.predeployedProxyAddress,
+    network.from,


network.from is validator 0. I think this should be a different key because it's validator key is quite sensitive.

Any ideas what else this could be? validator-0 is fairly convenient as that has been the address we use in our tooling to manage test networks (throughout celotool, I believe that's the address that has faucet balances), but agree that we might need to think differently about baklava.

It could be any key, including one generated on the fly. You could generate it from the same mnemonic and might create a new AccountType.ADMIN https://github.com/celo-org/celo-monorepo/blob/master/packages/celotool/src/lib/generate_utils.ts#L23

What do we currently set other things to? I.e. the approver, reserve spender, etc.? I agree validator 0 isn't the most robust choice, but I believe it's currently the standard choice

packages/protocol/test/governance/epochrewards.ts

packages/celotool/src/e2e-tests/governance_tests.ts

packages/protocol/contracts/governance/EpochRewards.sol

m-chrzan · 2019-11-23T21:14:04Z

@nategraf, thanks for the comments!

#1431 mentions being able to freeze the exchange, so I've edited the description to make #1431 as related, but not fixed.

#1467 already did the Exchange portion, so this PR will close out the issue.

* Appease the linter * beforeEach -> before

codecov · 2019-11-24T17:19:06Z

Codecov Report

Merging #1828 into master will decrease coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1828      +/-   ##
=========================================
- Coverage   74.43%   74.2%   -0.23%     
=========================================
  Files         281     278       -3     
  Lines        7822    7653     -169     
  Branches      974     672     -302     
=========================================
- Hits         5822    5679     -143     
+ Misses       1883    1857      -26     
  Partials      117     117

Flag	Coverage Δ
#mobile	`74.2% <ø> (-0.23%)`	⬇️

Impacted Files	Coverage Δ
packages/mobile/src/app/saga.ts	`58.33% <0%> (-3.78%)`	⬇️
packages/mobile/src/redux/selectors.ts	`76% <0%> (-2.58%)`	⬇️
packages/mobile/src/identity/reducer.ts	`46.15% <0%> (-2%)`	⬇️
packages/mobile/src/home/NotificationBox.tsx	`75% <0%> (-1.67%)`	⬇️
packages/mobile/src/app/actions.ts	`83.87% <0%> (-1.43%)`	⬇️
packages/mobile/src/import/ImportWalletEmpty.tsx	`78.78% <0%> (-0.63%)`	⬇️
packages/mobile/src/account/actions.ts	`91.66% <0%> (-0.44%)`	⬇️
packages/mobile/src/web3/saga.ts	`38.77% <0%> (-0.12%)`	⬇️
packages/mobile/src/firebase/actions.ts	`100% <0%> (ø)`	⬆️
packages/mobile/src/account/types.ts	`100% <0%> (ø)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a330768...21588db. Read the comment docs.

…1840)

nategraf

It would be good to make the key management change I mentioned, but overall LGTM. Thanks for addressing my comments!

asaj · 2019-11-26T19:19:17Z

packages/celotool/src/e2e-tests/governance_tests.ts

@@ -217,6 +217,65 @@ describe('governance tests', () => {
    assertAlmostEqual(currentBalance.minus(previousBalance), expected)
  }

+  const waitForBlock = async (blockNumber: number) => {


Thanks for cleaning this up!

asaj · 2019-11-26T19:20:23Z

packages/protocol/contracts/governance/EpochRewards.sol

@@ -372,6 +379,10 @@ contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry
   * @return The per validator epoch payment and the total rewards to voters.
   */
  function calculateTargetEpochPaymentAndRewards() external view returns (uint256, uint256) {
+    if (frozen) {


nit: This shouldn't strictly be necessary, since if one epoch rewards action fails we won't execute any of them.

Yep. @nategraf recommended this change for robustness.

One nice thing about how this is implemented is that after Baklava we can just delete the baklava/ directory and then remove any code that the compiler complains about.

asaj · 2019-11-26T19:59:05Z

packages/protocol/migrations/14_epoch_rewards.ts

  return [
    config.registry.predeployedProxyAddress,
+    network.from,


What do we currently set other things to? I.e. the approver, reserve spender, etc.? I agree validator 0 isn't the most robust choice, but I believe it's currently the standard choice

* master: (27 commits) Experience Brand Kit 1.0 (#1948) Adjust reference to the rewards app (#2065) [Wallet] Compatibility with exchange rate in string format (#2060) Fix Typo in CI config (#2056) Fix additional attestations instructions (#2057) Allow a specified address to disable/enable rewards distribution (#1828) Aaronmgdr/leaderboard patch (#2055) Move attestation service instructions to main page (#2051) Point To Updated Join Celo Video (#2052) Fix minor issue withe the ordering of instructions changes to docs related to discovery (#2025) [Docs] Fix typos in Running a Validator docs (#2045) Add node flag to celocli to set the target node for a single command (#2020) Fix broken links and spruce up CLI docs for accounts command (#2027) Prevent clipping of arrow component (#2036) Allocates an initial balance to the attestation bot (#2019) gold and dollar flags are required for faucet script (#1943) Clean seed words text area when returns from empty wallet view (#1904) Update validator script (#2026) Docs: PoS, metadata, gateway fee plus cleanup (#2022) ...

* master: (208 commits) Fix potential hard timeouts (#2072) Add checkzero,blockscout,mulit-address support to faucet. Fix broken envType checks (#2068) README added mission (#1972) Add dev-utils README (#2081) Add validator:status command to check if a validator signer is online and producing blocks (#1906) Experience Brand Kit 1.0 (#1948) Adjust reference to the rewards app (#2065) [Wallet] Compatibility with exchange rate in string format (#2060) Fix Typo in CI config (#2056) Fix additional attestations instructions (#2057) Allow a specified address to disable/enable rewards distribution (#1828) Aaronmgdr/leaderboard patch (#2055) Move attestation service instructions to main page (#2051) Point To Updated Join Celo Video (#2052) Fix minor issue withe the ordering of instructions changes to docs related to discovery (#2025) [Docs] Fix typos in Running a Validator docs (#2045) Add node flag to celocli to set the target node for a single command (#2020) Fix broken links and spruce up CLI docs for accounts command (#2027) Prevent clipping of arrow component (#2036) ... # Conflicts: # packages/web/src/layout/BookLayout.tsx # packages/web/src/lib.dom.d.ts

m-chrzan added 3 commits November 21, 2019 10:42

Freeze rewards distribution

3a0acc5

Merge branch 'master' into m-chrzan/toggle-rewards

3df4d2c

Test rewards are not distributed after freezing

1af99f0

m-chrzan requested a review from asaj as a code owner November 21, 2019 16:30

m-chrzan assigned mcortesi, asaj and nategraf Nov 21, 2019

nategraf reviewed Nov 22, 2019

View reviewed changes

nategraf assigned m-chrzan and unassigned nategraf Nov 22, 2019

m-chrzan added 2 commits November 23, 2019 22:34

Refactor

ac428da

* Appease the linter * beforeEach -> before

Merge branch 'master' into m-chrzan/toggle-rewards

e466053

Fix typo

e9639a1

m-chrzan requested review from mcortesi and yerdua as code owners November 25, 2019 10:47

m-chrzan and others added 7 commits November 25, 2019 11:48

Return 0s from rewards calculations when frozen

644441e

Fix elect validators migration, deploy integration (#1847)

a90e692

Update faucet to pull from the reserve when possible (#1844)

8ba374a

Serialize to hex string (#1848)

51f61b6

Various improvements to the CLI, allow voters to revote for a group (#…

a3fdf21

…1840)

Validator UX docs for baklava network (#1688) (#1849)

d71a75b

[Docs] Correct typo

a6d0b69

m-chrzan assigned nategraf and unassigned m-chrzan Nov 25, 2019

Merge branch 'master' into m-chrzan/toggle-rewards

51da099

nategraf approved these changes Nov 26, 2019

View reviewed changes

nategraf assigned m-chrzan and unassigned nategraf Nov 26, 2019

asaj approved these changes Nov 26, 2019

View reviewed changes

asaj unassigned mcortesi and asaj Nov 26, 2019

m-chrzan added the automerge Have PR merge automatically when checks pass label Dec 5, 2019

m-chrzan added 2 commits December 5, 2019 10:25

Merge branch 'master' into m-chrzan/toggle-rewards

8e8f18a

Merge branch 'master' into m-chrzan/toggle-rewards

21588db

celo-ci-bot-user merged commit 81d644c into master Dec 5, 2019

celo-ci-bot-user deleted the m-chrzan/toggle-rewards branch December 5, 2019 09:59

leojoy95 mentioned this pull request Oct 24, 2024

[Snyk] Fix for 58 vulnerabilities leojoy95/celo-monorepo#1271

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a specified address to disable/enable rewards distribution #1828

Allow a specified address to disable/enable rewards distribution #1828

m-chrzan commented Nov 21, 2019 •

edited

Loading

nategraf left a comment

nategraf Nov 22, 2019

m-chrzan Nov 23, 2019

nategraf Nov 26, 2019

asaj Nov 26, 2019

m-chrzan commented Nov 23, 2019

codecov bot commented Nov 24, 2019 •

edited

Loading

nategraf left a comment

asaj Nov 26, 2019

asaj Nov 26, 2019

m-chrzan Dec 4, 2019

asaj Nov 26, 2019

Allow a specified address to disable/enable rewards distribution #1828

Allow a specified address to disable/enable rewards distribution #1828

Conversation

m-chrzan commented Nov 21, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

nategraf 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

m-chrzan commented Nov 23, 2019

codecov bot commented Nov 24, 2019 • edited Loading

Codecov Report

nategraf 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

m-chrzan commented Nov 21, 2019 •

edited

Loading

codecov bot commented Nov 24, 2019 •

edited

Loading