Staking app #101
Staking app #101
Conversation
| function unlockedBalanceOf(address acct) public view returns (uint256) { | ||
| uint256 unlockedTokens = accounts[acct].amount; | ||
|
|
||
| Lock[] storage locks = accounts[acct].locks; |
|
|
|
|
|
Biggest hurdle will be the forwarding interface, will help you on this since I have some thoughts from my old unfinished Aragon Labs implementation |
|
Awesome @onbjerg. I was thinking forwarding should either be |
|
@izqui Biggest issue is not really the forwarding itself, it's mostly returning results from the contract we forward to. For example, depending on the outcome of a vote, we might want to unlock stake for some users (allowing stuff like TCRs). We could modify the callscript but I feel a little dirty just bringing it up... |
|
|
||
| function processStake(address acct, uint256 amount, bytes data) internal { | ||
| accounts[acct].amount += amount; // overflow check in token contract | ||
| assert(accounts[acct].amount >= amount); |
There was a problem hiding this comment.
What do you mean with the comment one line above? Aren't you checking overflow here?
Shouldn't we use SafeMath lib like the rest of the apps?
Do we allow amount = 0?
| } | ||
| } | ||
|
|
||
| function processStake(address acct, uint256 amount, bytes data) internal { |
There was a problem hiding this comment.
Why are using another function here? It's kind of short and only called once.
| } | ||
|
|
||
| function unstake(uint256 amount, bytes data) authP(UNSTAKE_ROLE, arr(amount)) checkUnlocked(amount) public { | ||
| require(accounts[msg.sender].amount >= amount); |
|
|
||
| function unlockAll(address acct) external { | ||
| while (accounts[acct].locks.length > 0) { | ||
| if (canUnlock(acct, 0)) { |
There was a problem hiding this comment.
Maybe I'm missing something here, but:
- Why unlock head of array first, instead of tail? With tail there would be no need to move elements of array.
- If the first element of the array can't be unlocked, how would the loop be exited?
| function stakeFor(address user, uint256 amount, bytes data) public; | ||
| function unstake(uint256 amount, bytes data) public; | ||
|
|
||
| function totalStakedFor(address addr) public view returns (uint256); |
There was a problem hiding this comment.
Missing totalStaked? I saw discussion here and your response, but it seems it's part of the interface definition.
|
|
||
| Lock[] storage locks = accounts[acct].locks; | ||
| for (uint256 i = 0; i < locks.length; i++) { | ||
| if (!canUnlock(acct, i)) { |
There was a problem hiding this comment.
Not sure about this. I think I understand the rationale, but I see a potential problem: If unstake function is called, it will call this function to check available staked but not locked tokens. If there are potentially unlocked, but actually locked tokens, they may be unstaked, leaving a bigger total locked balance than the staked balance.
| function lock( | ||
| uint256 amount, | ||
| uint8 lockUnit, | ||
| uint64 lockEnds, |
There was a problem hiding this comment.
Wouldn't be useful a helper/wrapper to indefinitely lock tokens without forcing the user to pass MAX UINT64?
| Unlocked(acct, msg.sender, lockId); | ||
| } | ||
|
|
||
| function moveTokens(address from, address to, uint256 amount) authP(GOD_ROLE, arr(from, to, amount)) external { |
There was a problem hiding this comment.
Shouldn't we check if moved tokens are unlocked? Otherwise an account could end up having more locked tokens than staked.
Staking: Address PR #101 comments
|
Missing items from PR #351
|
|
Closed in favor of https://github.com/aragon/staking/ |
Implements a generic Staking primitive that can be used to build things such as the Aragon Network v2 or TCRs.
Inspired by @onbjerg's aragonlabs' staking app and Harbour's Stakebank
Conforms to ERC900 staking interface.
Needs tests, documentation and review.