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

[WIP] Staking checkpointing #415

Closed
wants to merge 7 commits into
base: staking
from

Conversation

Projects
None yet
3 participants
@izqui
Member

izqui commented Aug 6, 2018

Implements a Staking sub-app that checkpoints a stake holder's balance on every stake/unstake action, conforming to the optional history methods of ERC900.

This is a required primitive for implementing stake-based voting with any ERC20 (w/o Minime) in a governance model that doesn't require locking tokens. An interesting combination would be a Voting app that requires a stake-lock in order to perform certain actions (create a proposal or vote favorably).

I'm currently thinking about whether we should move locking functionality off the main Staking contract in a similar fashion, as it would make it easier for us to implement different types of locking without adding exponential complexity, like it is currently happening with overlocking in #409 (cc @bingen)

To do:

  • Test StakingHistory with existing staking.js tests ( all passing)
  • Test history-specific functionality
  • (?) MiniMe interface wrapping for automatic compatibility with current MiniMe-compatible voting apps.

@izqui izqui self-assigned this Aug 6, 2018

@izqui izqui requested a review from bingen Aug 6, 2018

@coveralls

This comment has been minimized.

coveralls commented Aug 6, 2018

Coverage Status

Coverage remained the same at 94.91% when pulling 2d72cdf on staking-checkpointing into b8604b1 on staking.

@bingen

bingen approved these changes Sep 12, 2018

I left some minor questions, but overall looks good to me.

uint256 constant MAX_UINT192 = uint256(uint192(-1));
uint256 constant MAX_UINT64 = uint256(uint64(-1));
function add128(History storage self, uint192 value, uint64 time) internal {

This comment has been minimized.

@bingen

bingen Sep 12, 2018

Contributor

What does 128 in function name here mean? Shouldn't it be 192? Same below.

@@ -14,7 +14,6 @@ contract Staking is ERCStaking, AragonApp {
uint64 constant public MAX_UINT64 = uint64(-1);
struct Account {
uint256 amount;

This comment has been minimized.

@bingen

bingen Sep 12, 2018

Contributor

Maybe we can get rid of the struct now, as it only has one field.

This comment has been minimized.

@bingen

bingen Sep 12, 2018

Contributor

What's the reason for moving amount out of the struct, btw?

@@ -0,0 +1,90 @@
pragma solidity 0.4.18;
library Checkpointing {

This comment has been minimized.

@bingen

bingen Sep 12, 2018

Contributor

What's the point of making this a Library? Is it going to be used outside of StakingHistory?

@bingen bingen referenced this pull request Oct 2, 2018

Open

Decouple voting functionality from MiniMe token #388

0 of 3 tasks complete
while (high > low) {
uint256 mid = (high + low + 1) / 2; // average, ceil round
if (time >= self.history[mid].time) {

This comment has been minimized.

@bingen

bingen Oct 3, 2018

Contributor

I think I would split this condition so if (time == self.history[mid].time) return self.history[mid].value;

@bingen

This comment has been minimized.

Contributor

bingen commented Dec 2, 2018

@bingen bingen closed this Dec 2, 2018

@sohkai sohkai deleted the staking-checkpointing branch Dec 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment