-
Notifications
You must be signed in to change notification settings - Fork 76
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
Changes from 22 commits
669154b
4e7927d
0af3219
fd508eb
69011c7
53c9654
ae58bb7
92069a0
ad36803
713dba6
e1a09a9
0838359
8089c06
c15f280
2e03659
06fe284
c265be3
e73e240
1868900
237e2a5
1dc0163
7f4e5f7
1d2d17f
c5ec90c
5f2c3ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 delegate your stake 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."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd use |
||
_; | ||
} | ||
|
||
mapping(address => uint256) public stakeBalances; | ||
mapping(address => address) public delegatorToDelegate; | ||
mapping(address => address) public delegateToDelegator; | ||
|
||
/** | ||
* @dev Gets the stake balance of the specified address. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc nit - "staked balance" rather than "stake balance" |
||
* @param _address The address to query the balance of. | ||
* @return The amount staked by the passed address. | ||
*/ | ||
function stakeBalanceOf(address _address) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is looking up from |
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the delegate never calls |
||
* @param _address Address to where you want to delegate your balance. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe better
|
||
*/ | ||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second line of the doc here looks like copy-pasta from |
||
* @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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also seriously afraid we're switching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
with the current logic delegating won't work until both delegator and delegate add each other,
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 try to stake. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. *tries |
||
* @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."); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
import increaseTime, { duration, increaseTimeTo } from './helpers/increaseTime'; | ||
import exceptThrow from './helpers/expectThrow'; | ||
const KeepToken = artifacts.require('./KeepToken.sol'); | ||
const StakingProxy = artifacts.require('./StakingProxy.sol'); | ||
const TokenStaking = artifacts.require('./TokenStaking.sol'); | ||
|
||
|
||
contract('TestStakeDelegate', function(accounts) { | ||
|
||
let token, stakingProxy, stakingContract, | ||
account_one = accounts[0], | ||
account_two = accounts[1], | ||
account_three = accounts[2], | ||
account_four = accounts[3]; | ||
|
||
beforeEach(async () => { | ||
token = await KeepToken.new(); | ||
stakingProxy = await StakingProxy.new(); | ||
stakingContract = await TokenStaking.new(token.address, stakingProxy.address, duration.days(30)); | ||
await stakingProxy.authorizeContract(stakingContract.address, {from: account_one}) | ||
|
||
// Stake tokens as account one | ||
await token.approveAndCall(stakingContract.address, 200, "", {from: account_one}); | ||
|
||
// Send tokens to the accounts | ||
await token.transfer(account_two, 200, {from: account_one}); | ||
await token.transfer(account_three, 500, {from: account_one}); | ||
|
||
// Stake tokens as account two | ||
await token.approveAndCall(stakingContract.address, 200, "", {from: account_two}); | ||
}); | ||
|
||
it("should not be able to delegate stake to a delegate address that is a staker", async function() { | ||
await stakingContract.requestDelegateFor(account_one, {from: account_two}); | ||
await exceptThrow(stakingContract.approveDelegateAt(account_two)); | ||
}); | ||
|
||
it("should be able to delegate stake to a delegate address to represent your stake balance", async function() { | ||
await stakingContract.requestDelegateFor(account_one, {from: account_three}); | ||
await stakingContract.approveDelegateAt(account_three, {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 200, "Delegate account should represent delegator's stake balance."); | ||
assert.equal(await stakingProxy.balanceOf(account_one), 0, "Delegator account stake balance should become zero."); | ||
}); | ||
|
||
it("should not be able to delegate stake to a delegate address that is not approved", async function() { | ||
await stakingContract.requestDelegateFor(account_two, {from: account_three}); | ||
await stakingContract.approveDelegateAt(account_three, {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 0, "Delegate account should be zero since there were no handshake with delegator."); | ||
}); | ||
|
||
it("should be able to update delegate address if new delegate request exist", async function() { | ||
await stakingContract.requestDelegateFor(account_one, {from: account_three}); | ||
await stakingContract.approveDelegateAt(account_three, {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 200, "Delegate account should represent delegator's stake balance."); | ||
assert.equal(await stakingProxy.balanceOf(account_one), 0, "Delegator account stake balance should become zero."); | ||
|
||
await stakingContract.approveDelegateAt(account_four, {from: account_one}); | ||
await stakingContract.requestDelegateFor(account_one, {from: account_four}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 0, "Previous delegate account should stop representing delegator's stake balance."); | ||
assert.equal(await stakingProxy.balanceOf(account_four), 200, "Updated delegate account should represent delegator's stake balance."); | ||
}); | ||
|
||
it("should be able to remove delegated address that represents your stake balance", async function() { | ||
await stakingContract.requestDelegateFor(account_one, {from: account_three}); | ||
await stakingContract.approveDelegateAt(account_three, {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 200, "Delegate account should represent delegator's stake balance."); | ||
await stakingContract.removeDelegate(); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 0, "Delegate account should stop representing delegator's stake balance."); | ||
assert.equal(await stakingProxy.balanceOf(account_one), 200, "Delegator account should get its balance back."); | ||
}); | ||
|
||
it("should be able to change stake and get delegate to reflect updated balance", async function() { | ||
await stakingContract.requestDelegateFor(account_one, {from: account_three}); | ||
await stakingContract.approveDelegateAt(account_three, {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 200, "Delegate account should represent delegator's stake balance."); | ||
|
||
// Stake more tokens | ||
await token.approveAndCall(stakingContract.address, 100, "", {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 300, "Delegate account should reflect delegator's updated stake balance."); | ||
assert.equal(await stakingProxy.balanceOf(account_one), 0, "Delegator account stake balance should be zero."); | ||
|
||
// Unstake everything | ||
await stakingContract.initiateUnstake(300); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 0, "Delegate account should reflect delegator's updated stake balance."); | ||
}); | ||
|
||
it("should revert if delegate tries to stake", async function() { | ||
await stakingContract.requestDelegateFor(account_one, {from: account_three}); | ||
await stakingContract.approveDelegateAt(account_three, {from: account_one}); | ||
assert.equal(await stakingProxy.balanceOf(account_three), 200, "Delegate account should represent delegator's stake balance."); | ||
|
||
// Stake tokens as account three | ||
await exceptThrow(token.approveAndCall(stakingContract.address, 500, "", {from: account_three})); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this should read "that allows a staker to delegate their staked balance to any..."?