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

Final appeal round #45

Closed
wants to merge 5 commits into from
Closed

Conversation

bingen
Copy link
Contributor

@bingen bingen commented May 27, 2019

Closes #18
Closes #19

TODO:

  • Fees.
  • Lock juror stakes.

@bingen bingen requested a review from izqui May 27, 2019 12:58
@bingen bingen self-assigned this May 27, 2019
ßingen added 4 commits May 31, 2019 10:53
To check that final appeal can not be appealed again.
It should be independent of execution.
Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

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

Just some high-level comments, will continue the review when some of these are fixed

@@ -19,6 +19,8 @@ contract Court is ERC900, ApproveAndCallFallBack, ICRVotingOwner {
using SafeMath for uint256;

uint256 internal constant MAX_JURORS_PER_BATCH = 10; // to cap gas used on draft
uint256 internal constant MAX_DRAFT_ROUNDS = 4; // before the final appeal
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint256 internal constant MAX_DRAFT_ROUNDS = 4; // before the final appeal
uint256 internal constant MAX_REGULAR_APPEAL_ROUNDS = 4; // before the final appeal

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also make it configurable

Copy link
Contributor Author

@bingen bingen Jun 6, 2019

Choose a reason for hiding this comment

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

Makes sense, but I see 2 problems:

  • I'm afraid we'll hit stack too deep issue again in the constructor. Minor one, I'll figure it out.
  • It could be a little bit messy for on-going disputes, the logic would be more complex. I have to review all the occurrences of it to make sure I don't break anything.

So I'm going to open an issue for this (#55), so we don't get stuck here, ok?

@@ -530,7 +555,7 @@ contract Court is ERC900, ApproveAndCallFallBack, ICRVotingOwner {
Dispute storage dispute = disputes[_disputeId];
dispute.state = DisputeState.Executed;

uint8 winningRuling = _getWinningRuling(dispute);
uint8 winningRuling = dispute.rounds[_roundId].winningRuling;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless the dispute has been fully settled, round.winningRuling will be unset. This could send MissingRuling as the final ruling if it is executed before settlement.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we have talked before, I don't think that we should require waiting until a dispute has been settled to allow execution of the result.


struct Account {
mapping (address => uint256) balances; // token addr -> balance
// when deactivating, balance becomes available on next term:
uint64 deactivationTermId;
uint64 nextFinalAdjudicationRoundToSettle; // id in finalAdjudicationRounds array of the next pending final appeal round to settle
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this is not being set when the juror activates, which means a new juror's stake would have their tokens locked for all the final appeals that happened before they became a juror. The naive solution would be to just set it to the current finalAdjudicationRounds.length, but final appeals could still be created during the current term (in which the juror still isn't activated and cannot participate in the appeal).

There are also similar challenges around deactivating (and the complexity explodes if we take into account that a juror can activate again after deactivating)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I will push fixes in a while, but my initial approach, in order to avoid crazy complexity, is to forbid re-activations if there are outstanding final rounds, so jurors must settle them first.

{
for (uint256 i = finalAdjudicationRounds.length; i > 0; i--) {
if (finalAdjudicationRounds[i - 1].disputeId == _disputeId) {
return finalAdjudicationRounds[i - 1].lockedStakePerJuror * (_round.jurorNumber - _round.coherentJurors);
Copy link
Contributor

Choose a reason for hiding this comment

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

How would this take into account jurors that become over-leveraged?

uint8 winningRuling = _getWinningRuling(dispute);
uint256 coherentJurors = voting.getRulingVotes(voteId, winningRuling);
uint8 jurorRuling = voting.getCastVote(voteId, _juror);
uint256 coherentJurors = round.coherentJurors;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to prevent final round reward settlements from happening in this function as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, actually, I think weight here would be always 0, so it would revert, but I'm adding a sanity check.


if (jurorRuling == round.winningRuling) {
_assignTokens(jurorToken, _juror, round.slashedTokens * weight / coherentJurors);
_payFees(config.feeToken, _juror, config.jurorFee * round.jurorNumber * weight / coherentJurors, config.governanceFeeShare);
Copy link
Contributor

Choose a reason for hiding this comment

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

The rounding errors for distributing fees could be quite significant and a pretty big amount of fees would never be distributed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, well, assuming typical 18 decimals are used, each time the max amount lost would be 1e-18 of a token, right? But still, yes, I'm aware, not only here, that's why I opened an issue for this: #47

Copy link
Contributor

Choose a reason for hiding this comment

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

The amount of fees collected depends on the jurorNumber (which for the final round is calculated with totalStake / minStake), but each juror's weight is stake / minStake. I think the sum of the rest of the stake / minStake division for all jurors will be 'ghost stake' that is used for calculating the fee amount, but isn't actually distributed.

For example, there are only 2 jurors with 15 tokens each and the minStake is 10 tokens. Each juror will have a weight of 1, even though the total stake is 30 and therefore the jurorNumber is 3. If both jurors are coherent, when distributing the fees each juror will get 1/3 of the total reward, leaving 1/3 stuck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, although I think that error then belongs to here: https://github.com/aragon/aragon-court/blob/issue_19_final_appeal/contracts/Court.sol#L862

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bingen pushed a commit that referenced this pull request Jun 7, 2019
bingen pushed a commit that referenced this pull request Jun 7, 2019
@bingen
Copy link
Contributor Author

bingen commented Jun 18, 2019

Closed in favor of #56

@bingen bingen closed this Jun 18, 2019
@izqui izqui added this to the Freeze #1 milestone Oct 15, 2019
facuspagnuolo added a commit that referenced this pull request Oct 12, 2020
* chore: update aragon court to v1.1.2

* fees: distinguish jurors, appeal and subscriptions types

* fees: fix appeal fees handler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants