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

Add stake delegate functionality #121

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
669154b
Add `stake delegate` functionality to staking contracts
ngrinkevich May 18, 2018
4e7927d
Split tests to avoid out of gas issue
ngrinkevich May 18, 2018
0af3219
Add stake delegate tests
ngrinkevich May 21, 2018
fd508eb
Improve doc comments + minor typos fixes
ngrinkevich May 23, 2018
69011c7
Fix doc string
ngrinkevich May 30, 2018
53c9654
Emit staked/unstaked events via operator if staker works via operator
ngrinkevich May 31, 2018
ae58bb7
Add test to ensure changes to the parent stake are reflected in an op…
ngrinkevich May 31, 2018
92069a0
Refactor code with a modifier for null address check
ngrinkevich Jun 1, 2018
ad36803
Refactor code with a modifier for non staker check
ngrinkevich Jun 1, 2018
713dba6
Refactor delegate stake with the "handshake" approach
ngrinkevich Jun 1, 2018
e1a09a9
Fix broken modifiers
ngrinkevich Jun 1, 2018
0838359
Update tests
ngrinkevich Jun 5, 2018
8089c06
Split tests to avoid out of gas issue
ngrinkevich Jun 5, 2018
c15f280
Add a test for balance check if operator becomes a staker
ngrinkevich Jun 8, 2018
2e03659
Add revert to prevent a delegate to stake
ngrinkevich Jun 15, 2018
06fe284
Rename staker/operator to delegator/delegate for more clarity
ngrinkevich Jun 15, 2018
c265be3
Make sure delegator stake balance is zero when delegating
ngrinkevich Jun 15, 2018
e73e240
Merge branch 'master' into delegate-stake-to-operator-alternative
ngrinkevich Aug 2, 2018
1868900
Split token grant tests
ngrinkevich Aug 3, 2018
237e2a5
Remove duplicated test
ngrinkevich Aug 3, 2018
1dc0163
Provide error messages for require and revert
ngrinkevich Aug 3, 2018
7f4e5f7
Cleanup solidity tests
ngrinkevich Aug 3, 2018
1d2d17f
Improve doc strings and fix typos
ngrinkevich Aug 30, 2018
c5ec90c
Rename `stakeBalanceOf` for better clarity
ngrinkevich Aug 31, 2018
5f2c3ce
Switch `equal` to a safer `equal or less` check
ngrinkevich Aug 31, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions contracts/solidity/contracts/StakeDelegatable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
pragma solidity ^0.4.24;

import "openzeppelin-solidity/contracts/math/SafeMath.sol";


/**
* @title Stake Delegatable
* @dev A contract that allows a staker to delegate their staked balance
* to any address that is not already a staker. Delegator refers to a
* staker who has delegated their stake to another address, while delegate
* refers to an address that has had a stake delegated to it.
*/
contract StakeDelegatable {

/**
* @dev Only not null addresses can be passed to the functions with this modifier.
*/
modifier notNull(address _address) {
require(_address != address(0), "Provided address can not be zero.");
_;
}

/**
* @dev Only non staker addresses can be passed to the functions with this modifier.
*/
modifier notStaker(address _address) {
require(stakeBalances[_address] <= 0, "Provided address is not a staker.");
_;
}

mapping(address => uint256) public stakeBalances;
mapping(address => address) public delegatorToDelegate;
mapping(address => address) public delegateToDelegator;

/**
* @dev Gets the staked balance of the specified address.
* @param _address The address to query the balance of.
* @return The amount staked by the passed address.
*/
function stakedBalanceOf(address _address)
public
view
notNull(_address)
returns (uint256)
{
// If provided address is a delegate return its delegator balance.
address delegator = delegatorToDelegate[_address];
if (delegator != address(0) && delegateToDelegator[delegator] == _address) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we checking the reverse lookup here?

return stakeBalances[delegator];
}

// If provided address is a delegator return zero balance since the
// balance is delegated to a delegate.
address delegate = delegateToDelegator[_address];
if (delegate != address(0) && delegatorToDelegate[delegate] == _address) {
Copy link
Member

Choose a reason for hiding this comment

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

It appears this branch will never be hit- it's the same logic as the branch above, with a single variable name changed (delegator -> delegate).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I see. This terminology continues to be difficult to parse.

return 0;
}

return stakeBalances[_address];
}

/**
* @dev Returns address of a delegate if it exists for the
* provided address or the provided address otherwise.
* @param _address The address to check.
* @return Delegate address or provided address.
*/
function getDelegatorOrDelegate(address _address)
Copy link
Member

Choose a reason for hiding this comment

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

Function name is misleading- it doesn't get the delegator or the delegate, it gets the delegator if it exists, or returns the original address.

public
view
notNull(_address)
returns (address)
{
address delegate = delegateToDelegator[_address];
Copy link
Member

Choose a reason for hiding this comment

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

If this is looking up from delegateToDelegator, I'd expect the variable should be delegator rather than delegate?

if (delegate != address(0) && delegatorToDelegate[delegate] == _address) {
return delegate;
}
return _address;
}

/**
* @dev Approves address to delegate your stake balance. You can only
* have one delegate address. Delegate must also request to operate
* your stake by calling requestDelegateFor() method.
Copy link
Member

Choose a reason for hiding this comment

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

If the delegate never calls requestDelegateFor(), are the funds lost?

* @param _address Address to where you want to delegate your balance.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better

@param _address Address to which you want to delegate your staked balance management.

*/
function approveDelegateAt(address _address)
public
notNull(_address)
notStaker(_address)
{
delegateToDelegator[msg.sender] = _address;
}

/**
* @dev Requests to delegate stake for a specified address.
* An address must approve you first to delegate by calling
* requestDelegateFor() method.
Copy link
Member

Choose a reason for hiding this comment

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

The second line of the doc here looks like copy-pasta from approveDelegateAt()

* @param _address Address for which you request to delegate.
*/
function requestDelegateFor(address _address)
public
ngrinkevich marked this conversation as resolved.
Show resolved Hide resolved
notNull(_address)
{
delegatorToDelegate[msg.sender] = _address;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this would be safer if it confirmed the delegator has already requested this address be a delegate?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also seriously afraid we're switching delegate and delegator in these functions. Can we get some solid term definitions somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also seriously afraid we're switching delegate and delegator in these functions.

We're not actually switching, it's like a handshake, if those two mapping records match then delegation works, if any of them changes, delegation stops straight away.

It seems like this would be safer if it confirmed the delegator has already requested this address be a delegate?

with the current logic delegating won't work until both delegator and delegate add each other,
but I guess yeah would be safer to force delegator approve delegate address first (handshake only can be initiated by delegator)

Can we get some solid term definitions somewhere?

I'm also gonna look at ethereum/EIPs#1207 @Shadowfiend found, maybe we could use terms and concepts from there

}

/**
* @dev Removes delegate for your stake balance.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you want would to do this? Also... should you ever be allowed to, considering our threat model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking first case would be: if a delegate account is compromised then you definitely would like to remove it right? Even though delegate doesn't have any access to transfer balances, withdraw stakes etc.. it still can behave bad in the network and you as the delegator would be punished for that?

*/
function removeDelegate() public {
address delegate = delegateToDelegator[msg.sender];
delete delegatorToDelegate[delegate];
delete delegateToDelegator[msg.sender];
}

/**
* @dev Revert if a delegate tries to stake.
* @param _address The address to check.
*/
function revertIfDelegateStakes(address _address) internal {
address delegator = delegatorToDelegate[_address];
if (delegator != address(0)) {
revert("Provided address can not stake since it has stake delegated to it.");
}
}
}
4 changes: 2 additions & 2 deletions contracts/solidity/contracts/StakingProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
import "./utils/AddressArrayUtils.sol";

interface authorizedStakingContract {
function stakeBalanceOf(address addr) external view returns (uint256);
function stakedBalanceOf(address addr) external view returns (uint256);
}


Expand Down Expand Up @@ -48,7 +48,7 @@ contract StakingProxy is Ownable {
require(_staker != address(0), "Staker address can't be zero.");
uint256 balance = 0;
for (uint i = 0; i < authorizedContracts.length; i++) {
balance = balance + authorizedStakingContract(authorizedContracts[i]).stakeBalanceOf(_staker);
balance = balance + authorizedStakingContract(authorizedContracts[i]).stakedBalanceOf(_staker);
}
return balance;
}
Expand Down
17 changes: 4 additions & 13 deletions contracts/solidity/contracts/TokenGrant.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import "openzeppelin-solidity/contracts/token/ERC20/StandardToken.sol";
import "openzeppelin-solidity/contracts/token/ERC20/SafeERC20.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";
import "./StakingProxy.sol";
import "./StakeDelegatable.sol";


/**
Expand All @@ -14,7 +15,7 @@ import "./StakingProxy.sol";
* released gradually based on the vesting schedule cliff and vesting duration.
* Optionally grant can be revoked by the token grant creator.
*/
contract TokenGrant {
contract TokenGrant is StakeDelegatable {
using SafeMath for uint256;
using SafeERC20 for StandardToken;

Expand Down Expand Up @@ -54,9 +55,6 @@ contract TokenGrant {
// available to be released to the beneficiary
mapping(address => uint256) public balances;

// Token grants stake balances.
mapping(address => uint256) public stakeBalances;

// Token grants stake withdrawals.
mapping(uint256 => uint256) public stakeWithdrawalStart;

Expand All @@ -82,15 +80,6 @@ contract TokenGrant {
return balances[_owner];
}

/**
* @dev Gets the grants stake balance of the specified address.
* @param _owner The address to query the grants balance of.
* @return An uint256 representing the grants stake balance owned by the passed address.
*/
function stakeBalanceOf(address _owner) public view returns (uint256 balance) {
return stakeBalances[_owner];
}

/**
* @dev Gets grant by ID. Returns only basic grant data.
* If you need vesting schedule for the grant you must call `getGrantVestingSchedule()`
Expand Down Expand Up @@ -240,6 +229,8 @@ contract TokenGrant {
uint256 available = grants[_id].amount.sub(grants[_id].released);
require(available > 0, "Must have available granted amount to stake.");

revertIfDelegateStakes(msg.sender);

// Lock grant from releasing its balance.
grants[_id].locked = true;

Expand Down
33 changes: 16 additions & 17 deletions contracts/solidity/contracts/TokenStaking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import "openzeppelin-solidity/contracts/token/ERC20/SafeERC20.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";
import "./StakingProxy.sol";
import "./utils/UintArrayUtils.sol";
import "./StakeDelegatable.sol";


/**
Expand All @@ -13,7 +14,7 @@ import "./utils/UintArrayUtils.sol";
* A holder of the specified token can stake its tokens to this contract
* and unstake after withdrawal delay is over.
*/
contract TokenStaking {
contract TokenStaking is StakeDelegatable {
using SafeMath for uint256;
using SafeERC20 for StandardToken;
using UintArrayUtils for uint256[];
Expand All @@ -35,7 +36,6 @@ contract TokenStaking {
uint256 public withdrawalDelay;
uint256 public numWithdrawals;

mapping(address => uint256) public balances;
mapping(address => uint256[]) public withdrawalIndices;
mapping(uint256 => Withdrawal) public withdrawals;

Expand Down Expand Up @@ -67,14 +67,19 @@ contract TokenStaking {
require(StandardToken(_token) == token, "Token contract must be the same one linked to this contract.");
require(_value <= token.balanceOf(_from), "Sender must have enough tokens.");

revertIfDelegateStakes(_from);

// Transfer tokens to this contract.
token.transferFrom(_from, this, _value);

// Maintain a record of the stake amount by the sender.
balances[_from] = balances[_from].add(_value);
emit Staked(_from, _value);
stakeBalances[_from] = stakeBalances[_from].add(_value);

// Emit staked event. Check if staker works via operator first.
address delegatorOrDelegate = getDelegatorOrDelegate(_from);
emit Staked(delegatorOrDelegate, _value);
if (address(stakingProxy) != address(0)) {
stakingProxy.emitStakedEvent(_from, _value);
stakingProxy.emitStakedEvent(delegatorOrDelegate, _value);
}
}

Expand All @@ -86,16 +91,19 @@ contract TokenStaking {
*/
function initiateUnstake(uint256 _value) public returns (uint256 id) {

require(_value <= balances[msg.sender], "Staker must have enough tokens to unstake.");
require(_value <= stakeBalances[msg.sender], "Staker must have enough tokens to unstake.");

balances[msg.sender] = balances[msg.sender].sub(_value);
stakeBalances[msg.sender] = stakeBalances[msg.sender].sub(_value);

id = numWithdrawals++;
withdrawals[id] = Withdrawal(msg.sender, _value, now);
withdrawalIndices[msg.sender].push(id);
emit InitiatedUnstake(id);

// Emit unstaked event. Check if staker delegated its balance first.
address delegatorOrDelegate = getDelegatorOrDelegate(msg.sender);
if (address(stakingProxy) != address(0)) {
stakingProxy.emitUnstakedEvent(msg.sender, _value);
stakingProxy.emitUnstakedEvent(delegatorOrDelegate, _value);
}
return id;
}
Expand Down Expand Up @@ -123,15 +131,6 @@ contract TokenStaking {
emit FinishedUnstake(_id);
}

/**
* @dev Gets the stake balance of the specified address.
* @param _staker The address to query the balance of.
* @return An uint256 representing the amount owned by the passed address.
*/
function stakeBalanceOf(address _staker) public view returns (uint256 balance) {
return balances[_staker];
}

/**
* @dev Gets withdrawal request by ID.
* @param _id ID of withdrawal request.
Expand Down
6 changes: 3 additions & 3 deletions contracts/solidity/test/TestStake.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ contract TestStake {
token.approveAndCall(address(stakingContract), 100, "");

Assert.equal(token.balanceOf(address(this)), balance - 100, "Stake amount should be taken out from token holder's main balance.");
Assert.equal(stakingContract.stakeBalanceOf(address(this)), 100, "Stake amount should be added to token holder's stake balance.");
Assert.equal(stakingContract.stakedBalanceOf(address(this)), 100, "Stake amount should be added to token holder's stake balance.");
}

// Token holder should be able to initiate unstake of it's tokens
Expand All @@ -46,7 +46,7 @@ contract TestStake {
Assert.equal(amount, 100, "Withdrawal request should maintain a record of the amount.");
Assert.equal(start, now, "Withdrawal request should maintain a record of when it was initiated.");

Assert.equal(stakingContract.stakeBalanceOf(address(this)), 0, "Unstake amount should be taken out from token holder's stake balance.");
Assert.equal(stakingContract.stakedBalanceOf(address(this)), 0, "Unstake amount should be taken out from token holder's stake balance.");
Assert.equal(token.balanceOf(address(this)), balance, "Unstake amount should not be added to token holder main balance.");
}

Expand All @@ -63,7 +63,7 @@ contract TestStake {
// r will be false if it threw and true if it didn't.
bool r = throwProxy.execute.gas(200000)();
Assert.isFalse(r, "Should throw when trying to unstake when withdrawal delay is not over.");
Assert.equal(stakingContract.stakeBalanceOf(address(this)), 0, "Stake balance should stay unchanged.");
Assert.equal(stakingContract.stakedBalanceOf(address(this)), 0, "Stake balance should stay unchanged.");
}

// Token holder should not be able to stake without providing correct stakingContract address.
Expand Down
Loading