Adapt Court to MultiSortition #41

bingen · 2019-05-16T22:06:28Z

Add batches
Remove queues
Split voting
Split Sum Tree

Pull requests:
Replaces #37, #38 and #39
Closes: #27

Issues:
Fixes: #8, #12, #23
Closes: #17, #26

Add baches.

- Remove queues. - Make activations and deactivations immediate, instead of scheduled (on next term). - Allow jurors to re-activate, simpliflying this logic. There's no `PastJuror` state anymore.

If a juror was selected more than once in a round, the voting contract and the rewarding system would only count it once.

Shrink jurors array in round struct too in case of repeated jurors.

Add tests for batches.

izqui

Just some initial comments focused on drafting, will continue reviewing later

izqui · 2019-05-17T15:29:10Z

contracts/Court.sol

@@ -254,9 +231,11 @@ contract Court is ERC900, ApproveAndCallFallBack {
    ) public {
        termDuration = _termDuration;
        jurorToken = _jurorToken;
+        voting = _voting;
+        sumTree = _sumTree;


If we go the owner route for voting and sumTree we should check that the Court is the owner and that they are in the initial state

contracts/Court.sol

izqui · 2019-05-17T16:32:18Z

contracts/Court.sol

+                    if (round.nextJurorIndex > 0 && round.jurors[round.nextJurorIndex - 1] == juror) {
+                        round.jurors.length--;
+                    } else {
+                        round.jurors[round.nextJurorIndex] = juror;


We should be able to update round.nextJurorIndex only after adding all the jurors which would save a lot of gas in extra SSTORE and SLOAD

Same comment as above, actually, see here: https://github.com/aragon/aragon-court/blob/split_sumtree/contracts/Court.sol#L472

Moved to a new issue: #43

contracts/Court.sol

Simplify activation/deactivation logic. Address PR #41 comments.

and that it does from the very beginning. Address PR #41 comments.

Needed for settleAppealDeposit in PR #40.

Closes #48.

In case not all batches were fullfilled. Closes issue #14.

izqui

Still in the process of review this PR, but some more comments:

izqui · 2019-05-27T13:08:05Z

contracts/Court.sol

+                round.jurorNumber,
+                sortitionIteration
+            );
+            require(jurorKeys.length == stakes.length, ERROR_SORTITION_LENGTHS_MIMATCH);


I'd say we can use the same error for these two

We would definitely save some bytecode. My idea was to make it easier to spot the mismatch, but happy to join them.

izqui · 2019-06-03T14:40:24Z

contracts/CRVoting.sol

+        _;
+    }
+
+    constructor(address _preOwner) public {


The idea of the preOwner doesn't really solve the frontrunning problem for initialization (as the initcode could be fetched from a pending transaction) and adds some complexity that feels hacky.

I think we can just not set an owner in the constructor and allow the Court contract to 'claim ownership' which will revert if the ownership has already been claimed. This can be frontrun as well, but the frontrunning can be detected as the Court constructor will revert.

We should acknowledge that this is vulnerable to frontrunning at deployment time but in a way that is easily detectable.

izqui · 2019-06-03T14:50:56Z

contracts/HexSumTreeWrapper.sol

+    function init(address _owner, bytes32 _initCode) external {
+        require(address(owner) == address(keccak256(abi.encodePacked(_initCode))),  ERROR_WRONG_INIT_CODE);
+        owner = _owner;
+        tree.init();


Before initialization we could check that tree.rootDepth == 0 to ensure that the tree cannot be reinitialized for whatever reason.

izqui · 2019-06-03T14:51:39Z

contracts/HexSumTreeWrapper.sol

+        _;
+    }
+
+    constructor(address _preOwner) public {


Same comment about preOwner as in CRVoting

izqui · 2019-06-03T15:11:14Z

contracts/Court.sol

    mapping (address => Account) public accounts;
    mapping (uint256 => address) public jurorsByTreeId;
    mapping (uint64 => Term) public terms;
-    HexSumTree.Tree internal sumTree;
+    ISumTree internal sumTree;


Should be moved up next to voting as it is not strictly court state anymore.

izqui · 2019-06-03T16:07:36Z

contracts/Court.sol

+    }
+
+    function _canPerformVotingAction(uint256 _voteId, address _voter, AdjudicationState _state) internal returns (uint256) {
+        require(_voteId <= MAX_UINT32, ERROR_OVERFLOW);


Given that canCommit and canReveal aren't view functions, we should ensure that only voting can call these two functions for extra security

Yes, you are right, but the only thing that will modify state after this commit will be ensureTerm, that doesn't seem harmful. Do you think we can then keep it as is, or do you still want to add that check?

I think I'd still add the check

izqui · 2019-06-03T16:08:16Z

contracts/Court.sol

@@ -847,56 +777,42 @@ contract Court is ERC900, ApproveAndCallFallBack {
        nC = pA == pC ? nC : -nC;


With these changes we can now remove the _signedSum function which is no longer used

izqui · 2019-06-03T16:12:43Z

contracts/Court.sol

-
-        uint256 rulingVotes = round.rulingVotes[_ruling] + 1;
-        round.rulingVotes[_ruling] = rulingVotes;
+        votes[voteId] = Vote(uint128(_disputeId), uint128(roundId));


A nice optimization could be that the Court sets the id for votes (rather than being consecutive), so the vote ids could be _disputeId << 128 + roundId in a way that we wouldn't need a mapping between vote ids and the specific round, as it could be derived directly.

It would save a bunch of storage writes and reads. We don't necessarily need to implement it right away, but we can consider it later.

Great idea!

With batching, it doesn't make sense, as it could run out of gas. It must be done manually before.

Court: allow to continue draft at a later term

Court: remove dismiss dispute

Drafting will have to wait for a later term where randomness seed is available. Closes #15.

Address PR #41 comments.

Address PR #58 comments.

Court: prevents drafting without randomness seed

Plus make _getVoteId and _decodeVoteId pure. Missing changes in commit d4234fc: "Court: build voteId from disputeId and roundId".

facuspagnuolo · 2019-08-01T20:35:43Z

Closing this PR since development branch was already updated with these commits manually

* Add confirmation date to Appeal entity * Update confirm date only when opposed ruling is not zero Address PR #41. * Update schema.graphql Co-Authored-By: Fabrizio Vigevani <fabriziovigevani@gmail.com> * Change BigInt constructor Address PR #41 comments. Co-authored-by: Fabrizio Vigevani <fabriziovigevani@gmail.com>

ßingen added 19 commits May 2, 2019 22:16

Court: adapt to multiSortition

2fd9304

Add baches.

tmp: Court: adapt to multiSortition

ad7f264

- Remove queues. - Make activations and deactivations immediate, instead of scheduled (on next term). - Allow jurors to re-activate, simpliflying this logic. There's no `PastJuror` state anymore.

tmp: Split Voting

f6cfd15

Court Voting split: fix bug for repeated jurors

4cc673b

If a juror was selected more than once in a round, the voting contract and the rewarding system would only count it once.

Court split voting: add missing pieces

42b5327

Court, split voting: group juror's votes, adding weights

a2906af

Court, split voting: group juror's votes, adding weights

440bff7

Shrink jurors array in round struct too in case of repeated jurors.

Split HexSumTree library from Court

9e67418

tmp: Fix tests

5852cb7

Fix tests

350127b

Add lint and coverage

37b8d8f

Remove warnings: visibility

fb128ea

Fix court dispute tests

4ab7ca4

Add re-activation test

925edc8

Fix and refactor draftAdjudicationRound

60f66e6

Add tests for batches.

Protect HexSumTreeWrapper

a3a85e5

Add out of bounds error to CRVoting

1fe4227

Fix lint errors

d3615ab

Add tests

052377e

bingen requested a review from izqui May 16, 2019 22:06

bingen self-assigned this May 16, 2019

ßingen added 3 commits May 17, 2019 10:09

Fix leak vote

defa1f7

Add tests

7eaf6d4

Add tests. Remove some assert error messages for coverage

235cb47

izqui reviewed May 17, 2019

View reviewed changes

contracts/Court.sol Show resolved Hide resolved

Court: remove unnecessary incomingBalance

929fb19

Simplify activation/deactivation logic. Address PR #41 comments.

This was referenced May 21, 2019

Court: Allow slashing jurors in batches #42

Closed

Contracts: Stack too deep issues #43

Closed

Make sure Court owns Voting and SumTree contracts

7994c11

and that it does from the very beginning. Address PR #41 comments.

Refactor _payFees function

9f1c5f6

bingen force-pushed the split_sumtree branch from a3c9f18 to 66bf9ff Compare May 22, 2019 21:41

ßingen added 4 commits May 23, 2019 19:28

Restore _getWinningRuling

0ad05d9

Needed for settleAppealDeposit in PR #40.

Modify comments

3bee3de

Update obsolete block.blockhash to blockhash

9578683

Court: rename term to termId

3d490a1

bingen force-pushed the split_sumtree branch from 66bf9ff to 3d490a1 Compare May 23, 2019 17:37

ßingen added 5 commits May 31, 2019 10:34

Fix event arg bug in tests

fa49968

Rename getPastItem to getItemPast

2e813f5

CRVoting: fix ruling bug

b17ec56

Court: remove dismiss dispute

6d18937

Closes #48.

Court: allow to continue draft at a later term

9501706

In case not all batches were fullfilled. Closes issue #14.

izqui reviewed Jun 3, 2019

View reviewed changes

ßingen added 12 commits June 3, 2019 23:21

Court: address PR #41 comments

9222ea9

Court: don't modify dispute state on _checkAdjudicationState

e82d0ab

With batching, it doesn't make sense, as it could run out of gas. It must be done manually before.

Court: address PR #44 comments

a554e89

Court: address PR #44 comments

dec58f4

Merge pull request #44 from aragon/issue_14_missing_batches

c8c18d8

Court: allow to continue draft at a later term

Merge pull request #49 from aragon/issue_48_remove_dismiss

7af8d5f

Court: remove dismiss dispute

Court: address PR #41 comments

0e36f36

Court: prevents drafting without randomness seed

abda393

Drafting will have to wait for a later term where randomness seed is available. Closes #15.

Court: build voteId from disputeId and roundId

d4234fc

Address PR #41 comments.

Court: remove unused variables in test

5459193

Address PR #58 comments.

Merge pull request #58 from aragon/issue_15_missing_randomness_seed

305827c

Court: prevents drafting without randomness seed

Court: fix voting tests

1b31563

Plus make _getVoteId and _decodeVoteId pure. Missing changes in commit d4234fc: "Court: build voteId from disputeId and roundId".

facuspagnuolo closed this Aug 1, 2019

facuspagnuolo deleted the split_sumtree branch August 1, 2019 20:36

izqui added this to the Freeze #1 milestone Oct 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt Court to MultiSortition #41

Adapt Court to MultiSortition #41

bingen commented May 16, 2019 •

edited

Loading

izqui left a comment

izqui May 17, 2019

izqui May 17, 2019

bingen May 20, 2019

bingen May 21, 2019

izqui left a comment

izqui May 27, 2019

bingen Jun 3, 2019

izqui Jun 3, 2019

izqui Jun 3, 2019

izqui Jun 3, 2019

izqui Jun 3, 2019

izqui Jun 3, 2019

bingen Jun 4, 2019

izqui Jun 5, 2019

izqui Jun 3, 2019

izqui Jun 3, 2019

bingen Jun 3, 2019

facuspagnuolo commented Aug 1, 2019

		@@ -847,56 +777,42 @@ contract Court is ERC900, ApproveAndCallFallBack {
		nC = pA == pC ? nC : -nC;

Adapt Court to MultiSortition #41

Adapt Court to MultiSortition #41

Conversation

bingen commented May 16, 2019 • edited Loading

izqui 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

izqui 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Aug 1, 2019

bingen commented May 16, 2019 •

edited

Loading