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

Staking bug updates #97

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Staking bug updates #97

wants to merge 20 commits into from

Conversation

mbeylin
Copy link
Collaborator

@mbeylin mbeylin commented Aug 4, 2018

Implements fixes for:

#63 , #64 , #66, #69, #74, #75 , #76, #77, #78, #79 , #80 , #81 , #82 , #84 , #85 , #86, #88 , #89, #90, #91 , #92 , #94, #96

@skmgoldin
Copy link
Collaborator

Will review next week. 👍🏻

@skmgoldin
Copy link
Collaborator

@akuanti and I reviewed this! Overall it looks good. We did not review the tests.

Concrete suggestions:

  • Rename Whitelist struct to ClaimantTerms
  • Rename the whitelists array to whitelistedClaimants
  • Change isBeforeDeadline to use < rather than <=, otherwise the check is not strictly "before"
  • The @param _minimumFee definition for the whitelistClaimant function contains a typo.
  • Checking that the claimant is not the staker or the arbiter in the whitelistClaimant function seems a bit hopeless, as either party could simply sybil.
  • Stale comment in getNumWhitelists function. Should refer to the whitelists array, not the claims array.
  • Change getNumSettlements to getNumSettlementProposals and update the comments accordingly as well.

General notes:

  • Use SafeMath everywhere.
  • Don't forget to run the linter on your tests!!!!!
  • We should consider inheriting a shared enum for different ruling types that both DS and DV should use.
  • In light of:

Checking that the claimant is not the staker or the arbiter in the whitelistClaimant function seems a bit hopeless, as either party could simply sybil.

do we need to consider other solutions to the problem of stakers whitelisting themselves?

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.

2 participants