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

[Slashing] Allow voting gold to be slashable #2302

Merged
merged 9 commits into from
Jan 2, 2020

Conversation

lucasege
Copy link
Contributor

@lucasege lucasege commented Dec 19, 2019

Description

Adding function slashVotes within Election.sol that relies on another new function revokeVotes which combines revokePending and revokeActive in order to only sort internal group lists once.

Points of focus:

  • Cannot make the contract onlyRegisteredContract(LOCKED_GOLD_REGISTRY_ID) because if I override mockLockedGold's address the tests revert because that contract cannot be properly called, but I also can't use mockLockedGold's address as the caller since that address is unknown to truffle/ganache. Any guidance on this would be helpful, there don't appear to be any examples of doing this anywhere else.

Tested

Added unit tests

Related issues

Backwards compatibility

yes

packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
})
})

describe('when the account has voted for more than one group equally', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a decent amount of duplicated code in the following 3 test cases. Would it be possible to define something that parses objects like the following, sets up the test case, and then asserts the expected values?

const testCases = [
  {
    votesBefore: [
       {
          group: 0x1          
          pending: 2000
          active: 500
       }, {
          group: 0x2         
          pending: 1000
          active: 0
       }
    ],
    slash: 2700
    votesAfter: [
       {
          group: 0x1          
          pending: 0
          active: 0
       }, {
          group: 0x2         
          pending: 800
          active: 0
       }
    ]
  }]
}  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to you if you want to delay merging for this, I'll get started on it once I finish addressing reviews for #2317 since it seems lower priority, but still worth adding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not worth blocking the PR but something I suggest we do after since it will clean up the testing code quite a bit

packages/protocol/test/governance/election.ts Show resolved Hide resolved
@asaj asaj assigned lucasege and unassigned asaj Dec 19, 2019
@asaj asaj added the audit label Dec 20, 2019
@lucasege lucasege force-pushed the lucasege/slashing-voting-gold branch from 5f0e71d to a37bbfb Compare December 20, 2019 04:30
@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #2302 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2302   +/-   ##
=======================================
  Coverage   74.55%   74.55%           
=======================================
  Files         278      278           
  Lines        7777     7777           
  Branches      991      991           
=======================================
  Hits         5798     5798           
  Misses       1868     1868           
  Partials      111      111
Flag Coverage Δ
#mobile 74.55% <ø> (ø) ⬆️

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 23ec61f...0e4cb67. Read the comment docs.

@lucasege lucasege assigned asaj and unassigned lucasege Dec 20, 2019
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/governance/Election.sol Outdated Show resolved Hide resolved
* @param index The index of the group in the account's voting list.
* @return uint256 Number of votes successfully revoked, with a max of `value`.
*/
function _revokeVotes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Emit an event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An event ValidatorGroupVoteRevoked is emitted if any votes are successfully decremented. This is the same event as in revokePending and revokeActive. I don't think we need an additional event, let me know if you think otherwise or you just didn't see the existing event

packages/protocol/test/governance/election.ts Outdated Show resolved Hide resolved
@asaj asaj assigned lucasege and unassigned asaj Dec 21, 2019
@lucasege lucasege force-pushed the lucasege/slashing-voting-gold branch from a37bbfb to 2d15ef7 Compare December 26, 2019 18:55
@lucasege lucasege assigned mcortesi and unassigned lucasege Dec 30, 2019
@mcortesi mcortesi assigned lucasege and unassigned mcortesi Dec 30, 2019
@lucasege lucasege force-pushed the lucasege/slashing-voting-gold branch from 2d15ef7 to f42c864 Compare December 31, 2019 04:39
@lucasege lucasege assigned mcortesi and unassigned lucasege Dec 31, 2019
@mcortesi mcortesi added the automerge Have PR merge automatically when checks pass label Jan 2, 2020
@celo-ci-bot-user celo-ci-bot-user merged commit f7ca165 into master Jan 2, 2020
@celo-ci-bot-user celo-ci-bot-user deleted the lucasege/slashing-voting-gold branch January 2, 2020 13:56
aaronmgdr added a commit that referenced this pull request Jan 2, 2020
* master: (26 commits)
  Added new lint rule (#2349)
  [Slashing] Slash locked gold (#2317)
  [Slashing] Allow voting gold to be slashable (#2302)
  1666 precompiles assembly rewrite (#2324)
  Small fixes to deploy web (#2343)
  Governance, downtime and double signing slasher contracts (#2267)
  Added daily limit for reserve spending (#2303)
  Fixing multisig tests (#2342)
  [Wallet] Implement new send & import flow (#2332)
  Limechain/16xx (#1982)
  Add infinite pagination to Leaderboard (#2339)
  cli: Add rewards:show (#2160)
  Correct broken link formatting
  Celotool command for deploying hotfixes (#2142)
  Governance ContractKit + CLI changes (#2139)
  Complete codependent slashing precompile PRs (#2333)
  Add new and modified precompiles to UsingPrecompiles.sol (#2318)
  Adding error messages to contracts (#1182)
  Upgrade i18next and react-i18next to latest across monorepo (#2311)
  [Wallet] Fix exchange input on iOS and feed item display (#2319)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Jan 2, 2020
* master: (35 commits)
  Remove rep sentence from brand kit page (#2350)
  Added new lint rule (#2349)
  [Slashing] Slash locked gold (#2317)
  [Slashing] Allow voting gold to be slashable (#2302)
  1666 precompiles assembly rewrite (#2324)
  Small fixes to deploy web (#2343)
  Governance, downtime and double signing slasher contracts (#2267)
  Added daily limit for reserve spending (#2303)
  Fixing multisig tests (#2342)
  [Wallet] Implement new send & import flow (#2332)
  Limechain/16xx (#1982)
  Add infinite pagination to Leaderboard (#2339)
  cli: Add rewards:show (#2160)
  Correct broken link formatting
  Celotool command for deploying hotfixes (#2142)
  Governance ContractKit + CLI changes (#2139)
  Complete codependent slashing precompile PRs (#2333)
  Add new and modified precompiles to UsingPrecompiles.sol (#2318)
  Adding error messages to contracts (#1182)
  Upgrade i18next and react-i18next to latest across monorepo (#2311)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Locked gold voting in validator elections slashable
5 participants