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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 16 commits into from
Dec 5, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3a0acc5
Freeze rewards distribution
m-chrzan Nov 21, 2019
3df4d2c
Merge branch 'master' into m-chrzan/toggle-rewards
m-chrzan Nov 21, 2019
1af99f0
Test rewards are not distributed after freezing
m-chrzan Nov 21, 2019
ac428da
Refactor
m-chrzan Nov 23, 2019
e466053
Merge branch 'master' into m-chrzan/toggle-rewards
m-chrzan Nov 23, 2019
e9639a1
Fix typo
m-chrzan Nov 24, 2019
644441e
Return 0s from rewards calculations when frozen
m-chrzan Nov 25, 2019
a90e692
Fix elect validators migration, deploy integration (#1847)
Nov 25, 2019
8ba374a
Update faucet to pull from the reserve when possible (#1844)
Nov 25, 2019
51f61b6
Serialize to hex string (#1848)
nambrot Nov 25, 2019
a3fdf21
Various improvements to the CLI, allow voters to revote for a group (…
Nov 25, 2019
d71a75b
Validator UX docs for baklava network (#1688) (#1849)
Nov 25, 2019
a6d0b69
[Docs] Correct typo
jeanregisser Nov 25, 2019
51da099
Merge branch 'master' into m-chrzan/toggle-rewards
m-chrzan Nov 25, 2019
8e8f18a
Merge branch 'master' into m-chrzan/toggle-rewards
m-chrzan Dec 5, 2019
21588db
Merge branch 'master' into m-chrzan/toggle-rewards
m-chrzan Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 104 additions & 67 deletions packages/celotool/src/e2e-tests/governance_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,74 @@ describe('governance tests', () => {
assertAlmostEqual(currentBalance.minus(previousBalance), expected)
}

const waitForBlock = async (blockNumber: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

// const epoch = new BigNumber(await validators.methods.getEpochSize().call()).toNumber()
let currentBlock: number
do {
currentBlock = await web3.eth.getBlockNumber()
await sleep(0.1)
} while (currentBlock < blockNumber)
}

const waitForEpochTransition = async (epoch: number) => {
// const epoch = new BigNumber(await validators.methods.getEpochSize().call()).toNumber()
let blockNumber: number
do {
blockNumber = await web3.eth.getBlockNumber()
await sleep(0.1)
} while (blockNumber % epoch !== 1)
}

const assertTargetVotingYieldChanged = async (blockNumber: number, expected: BigNumber) => {
const currentTarget = new BigNumber(
(await epochRewards.methods.getTargetVotingYieldParameters().call({}, blockNumber))[0]
)
const previousTarget = new BigNumber(
(await epochRewards.methods.getTargetVotingYieldParameters().call({}, blockNumber - 1))[0]
)
const max = new BigNumber(
(await epochRewards.methods.getTargetVotingYieldParameters().call({}, blockNumber))[1]
)
const expectedTarget = previousTarget.plus(expected)
if (expectedTarget.isGreaterThanOrEqualTo(max)) {
assert.equal(currentTarget.toFixed(), max.toFixed())
} else if (expectedTarget.isLessThanOrEqualTo(0)) {
assert.isTrue(currentTarget.isZero())
} else {
const difference = currentTarget.minus(previousTarget)
// Assert equal to 9 decimal places due to rounding errors.
assert.equal(
fromFixed(difference)
.dp(9)
.toFixed(),
fromFixed(expected)
.dp(9)
.toFixed()
)
}
}

const assertTargetVotingYieldUnchanged = async (blockNumber: number) => {
await assertTargetVotingYieldChanged(blockNumber, new BigNumber(0))
}

const getLastEpochBlock = (blockNumber: number, epoch: number) => {
const epochNumber = Math.floor((blockNumber - 1) / epoch)
return epochNumber * epoch
}

const assertGoldTokenTotalSupplyUnchanged = async (blockNumber: number) => {
await assertGoldTokenTotalSupplyChanged(blockNumber, new BigNumber(0))
}

const assertGoldTokenTotalSupplyChanged = async (blockNumber: number, expected: BigNumber) => {
const currentSupply = new BigNumber(await goldToken.methods.totalSupply().call({}, blockNumber))
const previousSupply = new BigNumber(
await goldToken.methods.totalSupply().call({}, blockNumber - 1)
)
assertAlmostEqual(currentSupply.minus(previousSupply), expected)
}

describe('when the validator set is changing', () => {
let epoch: number
const blockNumbers: number[] = []
Expand Down Expand Up @@ -281,16 +349,9 @@ describe('governance tests', () => {
assert.equal(epoch, 10)

// Wait for an epoch transition so we can activate our vote.
let blockNumber: number
do {
blockNumber = await web3.eth.getBlockNumber()
await sleep(0.1)
} while (blockNumber % epoch !== 1)
await waitForEpochTransition(epoch)
// Wait for an extra epoch transition to ensure everyone is connected to one another.
do {
blockNumber = await web3.eth.getBlockNumber()
await sleep(0.1)
} while (blockNumber % epoch !== 1)
await waitForEpochTransition(epoch)

// Prepare for member swapping.
const groupWeb3 = new Web3('ws://localhost:8555')
Expand Down Expand Up @@ -357,14 +418,9 @@ describe('governance tests', () => {
)
}

const getLastEpochBlock = (blockNumber: number) => {
const epochNumber = Math.floor((blockNumber - 1) / epoch)
return epochNumber * epoch
}

it('should always return a validator set size equal to the number of group members at the end of the last epoch', async () => {
for (const blockNumber of blockNumbers) {
const lastEpochBlock = getLastEpochBlock(blockNumber)
const lastEpochBlock = getLastEpochBlock(blockNumber, epoch)
const validatorSetSize = await election.methods
.numberValidatorsInCurrentSet()
.call({}, blockNumber)
Expand All @@ -376,7 +432,7 @@ describe('governance tests', () => {
it('should always return a validator set equal to the signing keys of the group members at the end of the last epoch', async function(this: any) {
this.timeout(0)
for (const blockNumber of blockNumbers) {
const lastEpochBlock = getLastEpochBlock(blockNumber)
const lastEpochBlock = getLastEpochBlock(blockNumber, epoch)
const memberAccounts = await getValidatorGroupMembers(lastEpochBlock)
const memberSigners = await Promise.all(
memberAccounts.map((v: string) => getValidatorSigner(v, lastEpochBlock))
Expand All @@ -391,7 +447,7 @@ describe('governance tests', () => {
it('should block propose in a round robin fashion', async () => {
let roundRobinOrder: string[] = []
for (const blockNumber of blockNumbers) {
const lastEpochBlock = getLastEpochBlock(blockNumber)
const lastEpochBlock = getLastEpochBlock(blockNumber, epoch)
// Fetch the round robin order if it hasn't already been set for this epoch.
if (roundRobinOrder.length === 0 || blockNumber === lastEpochBlock + 1) {
const validatorSet = await getValidatorSetSignersAtBlock(blockNumber)
Expand Down Expand Up @@ -548,19 +604,6 @@ describe('governance tests', () => {
return new BigNumber(gpm).times(new BigNumber(gas))
}

const assertGoldTokenTotalSupplyChanged = async (
blockNumber: number,
expected: BigNumber
) => {
const currentSupply = new BigNumber(
await goldToken.methods.totalSupply().call({}, blockNumber)
)
const previousSupply = new BigNumber(
await goldToken.methods.totalSupply().call({}, blockNumber - 1)
)
assertAlmostEqual(currentSupply.minus(previousSupply), expected)
}

const assertLockedGoldBalanceChanged = async (blockNumber: number, expected: BigNumber) => {
await assertBalanceChanged(lockedGold.options.address, blockNumber, expected, goldToken)
}
Expand All @@ -573,10 +616,6 @@ describe('governance tests', () => {
await assertVotesChanged(blockNumber, new BigNumber(0))
}

const assertGoldTokenTotalSupplyUnchanged = async (blockNumber: number) => {
await assertGoldTokenTotalSupplyChanged(blockNumber, new BigNumber(0))
}

const assertLockedGoldBalanceUnchanged = async (blockNumber: number) => {
await assertLockedGoldBalanceChanged(blockNumber, new BigNumber(0))
}
Expand Down Expand Up @@ -641,39 +680,6 @@ describe('governance tests', () => {
})

it('should update the target voting yield', async () => {
const assertTargetVotingYieldChanged = async (blockNumber: number, expected: BigNumber) => {
const currentTarget = new BigNumber(
(await epochRewards.methods.getTargetVotingYieldParameters().call({}, blockNumber))[0]
)
const previousTarget = new BigNumber(
(await epochRewards.methods.getTargetVotingYieldParameters().call({}, blockNumber - 1))[0]
)
const max = new BigNumber(
(await epochRewards.methods.getTargetVotingYieldParameters().call({}, blockNumber))[1]
)
const expectedTarget = previousTarget.plus(expected)
if (expectedTarget.isGreaterThanOrEqualTo(max)) {
assert.equal(currentTarget.toFixed(), max.toFixed())
} else if (expectedTarget.isLessThanOrEqualTo(0)) {
assert.isTrue(currentTarget.isZero())
} else {
const difference = currentTarget.minus(previousTarget)
// Assert equal to 9 decimal places due to rounding errors.
assert.equal(
fromFixed(difference)
.dp(9)
.toFixed(),
fromFixed(expected)
.dp(9)
.toFixed()
)
}
}

const assertTargetVotingYieldUnchanged = async (blockNumber: number) => {
await assertTargetVotingYieldChanged(blockNumber, new BigNumber(0))
}

for (const blockNumber of blockNumbers) {
if (isLastBlockOfEpoch(blockNumber, epoch)) {
// We use the voting gold fraction from before the rewards are granted.
Expand All @@ -698,6 +704,37 @@ describe('governance tests', () => {
})
})

describe('when rewards distribution is frozen', () => {
before(restart)

let epoch: number
let blockFrozen: number
let latestBlock: number

before(async function(this: any) {
this.timeout(0)
const validator = (await kit.web3.eth.getAccounts())[0]
await kit.web3.eth.personal.unlockAccount(validator, '', 1000000)
await epochRewards.methods.freeze().send({ from: validator })
blockFrozen = await web3.eth.getBlockNumber()
epoch = new BigNumber(await validators.methods.getEpochSize().call()).toNumber()
await waitForBlock(blockFrozen + epoch * 2)
latestBlock = await web3.eth.getBlockNumber()
})

it('should not update the target voing yield', async () => {
for (let blockNumber = blockFrozen; blockNumber < latestBlock; blockNumber++) {
await assertTargetVotingYieldUnchanged(blockNumber)
}
})

it('should not mint new Celo Gold', async () => {
for (let blockNumber = blockFrozen; blockNumber < latestBlock; blockNumber++) {
await assertGoldTokenTotalSupplyUnchanged(blockNumber)
}
})
})

describe('after the gold token smart contract is registered', () => {
let goldGenesisSupply = new BigNumber(0)
beforeEach(async function(this: any) {
Expand Down
15 changes: 13 additions & 2 deletions packages/protocol/contracts/governance/EpochRewards.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.5.3;
import "openzeppelin-solidity/contracts/math/SafeMath.sol";
import "openzeppelin-solidity/contracts/ownership/Ownable.sol";

import "../baklava/Freezable.sol";
import "../common/FixidityLib.sol";
import "../common/Initializable.sol";
import "../common/UsingRegistry.sol";
Expand All @@ -11,7 +12,7 @@ import "../common/UsingPrecompiles.sol";
/**
* @title Contract for calculating epoch rewards.
*/
contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry {
contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry, Freezable {
using FixidityLib for FixidityLib.Fraction;
using SafeMath for uint256;

Expand Down Expand Up @@ -82,6 +83,7 @@ contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry
*/
function initialize(
address registryAddress,
address _freezer,
uint256 targetVotingYieldInitial,
uint256 targetVotingYieldMax,
uint256 targetVotingYieldAdjustmentFactor,
Expand All @@ -92,6 +94,7 @@ contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry
uint256 _targetValidatorEpochPayment
) external initializer {
_transferOwnership(msg.sender);
setFreezer(_freezer);
setRegistry(registryAddress);
setTargetVotingYieldParameters(targetVotingYieldMax, targetVotingYieldAdjustmentFactor);
setRewardsMultiplierParameters(
Expand Down Expand Up @@ -127,6 +130,10 @@ contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry
);
}

function setFreezer(address freezer) public onlyOwner {
_setFreezer(freezer);
}

/**
* @notice Sets the target voting Gold fraction.
* @param value The percentage of floating Gold voting to target.
Expand Down Expand Up @@ -362,7 +369,7 @@ contract EpochRewards is Ownable, Initializable, UsingPrecompiles, UsingRegistry
* voting Gold fraction.
* @dev Only called directly by the protocol.
*/
function updateTargetVotingYield() external {
function updateTargetVotingYield() external onlyWhenNotFrozen {
m-chrzan marked this conversation as resolved.
Show resolved Hide resolved
require(msg.sender == address(0));
_updateTargetVotingYield();
}
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return (0, 0);
}

uint256 targetEpochRewards = getTargetEpochRewards();
uint256 targetTotalEpochPaymentsInGold = getTargetTotalEpochPaymentsInGold();
uint256 targetGoldSupplyIncrease = targetEpochRewards.add(targetTotalEpochPaymentsInGold);
Expand Down
5 changes: 4 additions & 1 deletion packages/protocol/migrations/14_epoch_rewards.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ import { deploymentForCoreContract } from '@celo/protocol/lib/web3-utils'
import { config } from '@celo/protocol/migrationsConfig'
import { toFixed } from '@celo/utils/lib/fixidity'
import { EpochRewardsInstance } from 'types'
const truffle = require('@celo/protocol/truffle-config.js')

const initializeArgs = async (): Promise<any[]> => {
const initializeArgs = async (networkName: string): Promise<any[]> => {
const network: any = truffle.networks[networkName]
return [
config.registry.predeployedProxyAddress,
network.from,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

toFixed(config.epochRewards.targetVotingYieldParameters.initial).toFixed(),
toFixed(config.epochRewards.targetVotingYieldParameters.max).toFixed(),
toFixed(config.epochRewards.targetVotingYieldParameters.adjustmentFactor).toFixed(),
Expand Down
17 changes: 17 additions & 0 deletions packages/protocol/test/governance/epochrewards.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ contract('EpochRewards', (accounts: string[]) => {

m-chrzan marked this conversation as resolved.
Show resolved Hide resolved
await epochRewards.initialize(
registry.address,
accounts[0],
targetVotingYieldParams.initial,
targetVotingYieldParams.max,
targetVotingYieldParams.adjustmentFactor,
Expand Down Expand Up @@ -130,6 +131,7 @@ contract('EpochRewards', (accounts: string[]) => {
await assertRevert(
epochRewards.initialize(
registry.address,
accounts[0],
targetVotingYieldParams.initial,
targetVotingYieldParams.max,
targetVotingYieldParams.adjustmentFactor,
Expand Down Expand Up @@ -578,4 +580,19 @@ contract('EpochRewards', (accounts: string[]) => {
})
})
})

describe('when the contract is frozen', () => {
beforeEach(async () => {
await epochRewards.freeze()
})

it('should make calculateTargetEpochPaymentAndRewards return zeroes', async () => {
const [
validatorPayment,
voterRewards,
] = await epochRewards.calculateTargetEpochPaymentAndRewards()
assertEqualBN(validatorPayment, 0)
assertEqualBN(voterRewards, 0)
})
})
})