Court: Polish main court contract functionality #103

facuspagnuolo · 2019-08-15T13:46:30Z

Fixes #83 and #16

Lots of polishing stuff, inline documentation and bunch of new tests happening on this PR, important pieces to take a look carefully are:

⚠️ Ensure first term is in the future (53ff9e3#diff-4bb317b489994cf6f4c4b7ed11c18590R235)
⚠️ Ensure term is not zero when creating disputes (79abd26#diff-4bb317b489994cf6f4c4b7ed11c18590R297)
⚠️ Ensure appealer cannot confirm an appeal (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R456)
⚠️ Use start term id of the final round to fetch the total active balance of the jurors' registry when computing the jurors number (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R1294)
Unify appeal struct (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R172-R177)
Handle Pre-Draft and Executed dispute states when querying an adjudication round state (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R1362-R1371)
Add round getter (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R760)
Add appeal getter (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R796)
Update settledJurors even for the last batch to ensure consistency (2e3cbbd#diff-4bb317b489994cf6f4c4b7ed11c18590R1008)
New thousands percentage lib (0cb4d62?file-filters%5B%5D=.sol#diff-7c2369ae5667d560b954f85000fff662R6-R22)

izqui

Looking really good so far, some minor comments

contracts/Court.sol

izqui

~~Some minor comments that I forgot to submit~~

Edit: moving them to #104

bingen

Still reviewing, but here you have another batch.
Before I forget, I realized there are some lint errors. I will push a small PR to add linter to CI, ok?

contracts/Court.sol

bingen · 2019-09-09T16:06:53Z

contracts/Court.sol

+    * @dev Tell the last ensured term identification number
+    * @return Identification number of the last ensured term
+    */
+    function getLastEnsuredTermId() external view returns (uint64) {


I wouldn't call this Ensured, as it can be misunderstood as if we somehow know that we have already called ensureTerm and it's up to date.

Not sure I follow the scenario you are describing. IMO this is what termId actually means, it's the last ensured term since it may be outdated.

Ok, again, proceed as you want, it's just a name, but to make my point clear: if termId means "the last ensured term", I don't see the need for adding Ensured as it would be just confusing, like there are 2 different concepts, termId and ensuredTermId.
But I don't think termId is "ensured" at all, as we don't know if all needed term transitions have taken place.
Anyway, to me this is just and old plain getter:

function getLastEnsuredTermId() external view returns (uint64) { return termId; }

And it's a common practice to name getters as get + variable name. So if you really want to call this with that (IMO) unnecessary long name instead of the classic getTermId, I would then rename the variable to lastEnsuredTermId.

[Suggestion for our style guide, section naming conventions: plain getters are called get + variable name]

This is definitely a bit of bikeshedding at this point, but what about getLastTransitionedTermId?

I agree it's bikeshedding. Actually I don't think we even need this function at all, as we could have kept termId public and use the automatically generated getter.
[Another thing to discuss for our style guide: do we use public state variables and auto-generated getters? Or do we declare them all internal and explicitly write getters?]

I like getLastTransitionedTermId and also agree about the getters pattern, I think the whole problem comes as a consequence of the state variable being called simply termId which is not always up-to-date. What about renaming the variable to lastTransitionedTermId as well?

contracts/Court.sol

izqui

Outstanding work on this @facuspagnuolo!

Some minor comments, but good to go on my end to merge 🔥

contracts/Court.sol

test/helpers/court.js

test/court/court-appeal.js

test/court/court-confirm-appeal.js

test/court/court-settle.js

Co-Authored-By: Jorge Izquierdo <[email protected]>

facuspagnuolo · 2019-09-19T03:40:15Z

@izqui I already addressed all your comments. Please take a look at the related changes, I'd recommend reviewing them by commit.

⚠️ I improved and added some other appeal tests in e2cac0b, and also noted that we may be missing the final round reduction here. LMKYT

izqui

LGTM

izqui · 2019-09-19T07:25:59Z

contracts/Court.sol

@@ -1235,6 +1235,7 @@ contract Court is IJurorsRegistryOwner, ICRVotingOwner, ISubscriptionsOwner, Tim

        // Otherwise, return the times the active balance of the juror fits in the min active balance, multiplying
        // it by a round factor to ensure a better precision rounding.
+        // TODO: review, we are not using the final round discount here


Good catch. I don't think we should have it here though, but it should definitely be in settleReward when assigning fees for coherent jurors in the final round

aragon-court/contracts/Court.sol

Line 591 in e2cac0b

accounting.assign(config.feeToken, _juror, jurorFee);

And we should definitely have a test for it. Maybe we should work on it on a specific test file for accounting?

Hm, I don't get it. round.jurorFees already has the discount applied, right?

True, I was confused. The problem was that we are not testing it 😂

facuspagnuolo added wip Work in progress issue component:contracts Contracts related requirements labels Aug 15, 2019

facuspagnuolo requested a review from izqui August 15, 2019 13:46

facuspagnuolo self-assigned this Aug 15, 2019

facuspagnuolo force-pushed the polish_main_court_contract branch from 6a5b041 to 6618c22 Compare August 15, 2019 13:51

izqui reviewed Aug 15, 2019

View reviewed changes

contracts/Court.sol Outdated Show resolved Hide resolved

contracts/Court.sol Show resolved Hide resolved

facuspagnuolo force-pushed the polish_main_court_contract branch 3 times, most recently from 4411b03 to d877efd Compare August 23, 2019 15:12

facuspagnuolo added component:tests Tests related requirements documentation Documentation related requirements labels Aug 23, 2019

facuspagnuolo force-pushed the polish_main_court_contract branch 2 times, most recently from ad53add to 99ac098 Compare August 26, 2019 12:02

facuspagnuolo changed the base branch from development to add_drafting_tests August 26, 2019 12:03

facuspagnuolo force-pushed the polish_main_court_contract branch from 99ac098 to 5e0b779 Compare August 26, 2019 12:34

izqui reviewed Aug 26, 2019

View reviewed changes

facuspagnuolo force-pushed the polish_main_court_contract branch 6 times, most recently from 8629ab5 to 95124f4 Compare August 28, 2019 11:18

facuspagnuolo changed the title ~~[WIP] Polish main court functionality~~ Polish main court functionality Aug 28, 2019

facuspagnuolo force-pushed the polish_main_court_contract branch 2 times, most recently from b138838 to da99eeb Compare August 29, 2019 12:59

facuspagnuolo added 5 commits August 30, 2019 21:39

agreements: fix contract warnings

8221815

court: rename appealConfirm to confirmAppeal

f6dc198

court: rename draftAjudicationRound to draft

3c4320e

court: organize functions

8d07709

court: drop redundant areAllJurorsSettled function

c02593a

bingen reviewed Sep 6, 2019

View reviewed changes

facuspagnuolo added 4 commits September 6, 2019 16:48

court: improve adjudication round checks

a81c2d1

court: draft sloads optimization

66194cc

court: fix typo

3480bc3

court: improve uint256 castings

1d84975

bingen reviewed Sep 6, 2019

View reviewed changes

contracts/Court.sol Show resolved Hide resolved

contracts/Court.sol Show resolved Hide resolved

contracts/Court.sol Show resolved Hide resolved

contracts/Court.sol Show resolved Hide resolved

bingen reviewed Sep 6, 2019

View reviewed changes

contracts/Court.sol Outdated Show resolved Hide resolved

contracts/Court.sol Outdated Show resolved Hide resolved

court: optimize adjudication round validations

8bb0168

bingen reviewed Sep 9, 2019

View reviewed changes

This was referenced Sep 16, 2019

Voting: Improve implementation and document #96

Merged

Chore: Upgrade contracts to Solidity 0.5.8 #109

Merged

facuspagnuolo added 3 commits September 17, 2019 16:43

court: fix heartbeat court config fetch

d71c47a

court: rename _ensureAdjudicationState to use check instead

87c7b3b

court: remove useless memoization in appeal settle

12cb8ea

izqui approved these changes Sep 18, 2019

View reviewed changes

facuspagnuolo and others added 4 commits September 18, 2019 13:37

tests: improve wording and fix typos

0f31a59

Co-Authored-By: Jorge Izquierdo <[email protected]>

tests: minor enhancements

cce6087

court: undo appeal skipped tests

b953158

court: minor enhacements and optimizations

f616da3

facuspagnuolo force-pushed the polish_main_court_contract branch from 37ca5d6 to f616da3 Compare September 18, 2019 12:47

facuspagnuolo added 4 commits September 18, 2019 12:43

court: optimize drafting and improve tests

e470083

court: add appeal details getter

14da657

court: improve appeals tests

e2cac0b

court: add getter to tell dispute fees

234267f

izqui approved these changes Sep 19, 2019

View reviewed changes

facuspagnuolo merged commit 36679bf into development Sep 19, 2019

delete-merged-branch bot deleted the polish_main_court_contract branch September 19, 2019 11:15

bingen mentioned this pull request Sep 25, 2019

Court: Implement new config params #146

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Court: Polish main court contract functionality #103

Court: Polish main court contract functionality #103

facuspagnuolo commented Aug 15, 2019 •

edited

Loading

izqui left a comment

izqui left a comment •

edited

Loading

bingen left a comment

bingen Sep 9, 2019

facuspagnuolo Sep 17, 2019

bingen Sep 18, 2019 •

edited

Loading

izqui Sep 18, 2019

bingen Sep 18, 2019

facuspagnuolo Sep 18, 2019

izqui left a comment

facuspagnuolo commented Sep 19, 2019 •

edited

Loading

izqui left a comment

izqui Sep 19, 2019

bingen Sep 19, 2019

facuspagnuolo Sep 19, 2019

Court: Polish main court contract functionality #103

Court: Polish main court contract functionality #103

Conversation

facuspagnuolo commented Aug 15, 2019 • edited Loading

izqui left a comment

Choose a reason for hiding this comment

izqui left a comment • edited Loading

Choose a reason for hiding this comment

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen Sep 18, 2019 • edited Loading

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

facuspagnuolo commented Sep 19, 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

facuspagnuolo commented Aug 15, 2019 •

edited

Loading

izqui left a comment •

edited

Loading

bingen Sep 18, 2019 •

edited

Loading

facuspagnuolo commented Sep 19, 2019 •

edited

Loading