Skip to content

Commit

Permalink
Address PR #1 comments
Browse files Browse the repository at this point in the history
  • Loading branch information
ßingen committed Apr 12, 2019
1 parent 053e22b commit 6f7728f
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 62 deletions.
4 changes: 2 additions & 2 deletions contracts/IStakingLocking.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ interface IStakingLocking {
function setLockAmount(address _account, uint256 _lockId, uint256 _newAmount) external;
function setLockManager(address _account, uint256 _lockId, ILockManager _newManager) external;
function setLockData(address _account, uint256 _lockId, bytes _newData) external;
function transfer(uint256 _amount, address _to, uint256 _toLockId) external;
function transferFromLock(address _account, uint256 _lockId, uint256 _amount, address _to, uint256 _toLockId) external;
function transfer(address _to, uint256 _toLockId, uint256 _amount) external;
function transferFromLock(address _account, uint256 _lockId, address _to, uint256 _toLockId, uint256 _amount) external;

function locksCount(address _account) external view returns (uint256);
function getLock(address _account, uint256 _lockId)
Expand Down
78 changes: 36 additions & 42 deletions contracts/Staking.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,17 @@ contract Staking is ERCStaking, ERCStakingHistory, IStakingLocking, TimeHelpers,
// unstaking 0 tokens is not allowed
require(_amount > 0, ERROR_AMOUNT_ZERO);

// transfer tokens
// it will fail for non ERC20 compliant tokens which don't return anything
// see: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
require(stakingToken.transfer(msg.sender, _amount), ERROR_TOKEN_TRANSFER);

// checkpoint updated staking balance
_modifyStakeBalance(msg.sender, _amount, false);

// checkpoint total supply
_modifyTotalStaked(_amount, false);

// transfer tokens
// it will fail for non ERC20 compliant tokens which don't return anything
// see: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
require(stakingToken.transfer(msg.sender, _amount), ERROR_TOKEN_TRANSFER);

emit Unstaked(msg.sender, _amount, totalStakedFor(msg.sender), _data);
}

Expand Down Expand Up @@ -161,62 +161,40 @@ contract Staking is ERCStaking, ERCStakingHistory, IStakingLocking, TimeHelpers,

/**
* @notice Transfer `_amount` tokens to `_to``_toLockId > 0 ? '\'s lock #' + _toLockId : ''`
* @param _amount Number of tokens to be transferred
* @param _to Recipient of the tokens
* @param _toLockId Lock id of the recipient to add the tokens to, if any
* @param _amount Number of tokens to be transferred
*/
function transfer(uint256 _amount, address _to, uint256 _toLockId) external {
// transferring 0 staked tokens is invalid
require(_amount > 0, ERROR_AMOUNT_ZERO);
function transfer(address _to, uint256 _toLockId, uint256 _amount) external {
// have enough unlocked funds
require(_amount <= unlockedBalanceOf(msg.sender), ERROR_NOT_ENOUGH_BALANCE);

// first valid lock starts at 1, so _toLockId = 0 means no lock
if (_toLockId > 0) {
_updateActiveLockAmount(_to, _toLockId, _amount, true);
}

// update stakes
_modifyStakeBalance(msg.sender, _amount, false);
_modifyStakeBalance(_to, _amount, true);

emit StakeTransferred(msg.sender, 0, _amount, _to, _toLockId);
_transfer(msg.sender, 0, _to, _toLockId, _amount);
}

/**
* @notice Transfer `_amount` tokens from `_account`'s lock #`_lockId` to `_to``_toLockId > 0 ? '\'s lock #' + _toLockId : ''`
* @param _account Owner of locked tokens
* @param _lockId Id of the lock for the given account
* @param _amount Number of tokens to be transferred
* @notice Transfer `_amount` tokens from `_from`'s lock #`_fromLockId` to `_to``_toLockId > 0 ? '\'s lock #' + _toLockId : ''`
* @param _from Owner of locked tokens
* @param _fromLockId Id of the lock for the given account
* @param _to Recipient of the tokens
* @param _toLockId Lock id of the recipient to add the tokens to, if any
* @param _amount Number of tokens to be transferred
*/
function transferFromLock(
address _account,
uint256 _lockId,
uint256 _amount,
address _from,
uint256 _fromLockId,
address _to,
uint256 _toLockId
uint256 _toLockId,
uint256 _amount
)
external
isLockManager(_account, _lockId)
isLockManager(_from, _fromLockId)
{
// No need to check that lockId > 0, as isLockManager would fail
// transferring 0 locked tokens is invalid
require(_amount > 0, ERROR_AMOUNT_ZERO);
// no need to check that have enough locked funds, as _updateActiveLockAmount will fail

_updateActiveLockAmount(_account, _lockId, _amount, false);
// first valid lock starts at 1, so _toLockId = 0 means no lock
if (_toLockId > 0) {
_updateActiveLockAmount(_to, _toLockId, _amount, true);
}

// update stakes
_modifyStakeBalance(_account, _amount, false);
_modifyStakeBalance(_to, _amount, true);
// No need to check that have enough locked funds, as _updateActiveLockAmount will fail

emit StakeTransferred(_account, _lockId, _amount, _to, _toLockId);
_transfer(_from, _fromLockId, _to, _toLockId, _amount);
_updateActiveLockAmount(_from, _fromLockId, _amount, false);
}

/**
Expand Down Expand Up @@ -504,4 +482,20 @@ contract Staking is ERCStaking, ERCStakingHistory, IStakingLocking, TimeHelpers,
}
}
}

function _transfer(address _from, uint256 _fromLockId, address _to, uint256 _toLockId, uint256 _amount) internal {
// transferring 0 staked tokens is invalid
require(_amount > 0, ERROR_AMOUNT_ZERO);

// first valid lock starts at 1, so _toLockId = 0 means no lock
if (_toLockId > 0) {
_updateActiveLockAmount(_to, _toLockId, _amount, true);
}

// update stakes
_modifyStakeBalance(_from, _amount, false);
_modifyStakeBalance(_to, _amount, true);

emit StakeTransferred(_from, _fromLockId, _amount, _to, _toLockId);
}
}
10 changes: 5 additions & 5 deletions contracts/test/mocks/LockManagerMock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ contract LockManagerMock is ILockManager {

function transferFromLock(
Staking _staking,
address _account,
uint256 _lockId,
uint256 _amount,
address _from,
uint256 _fromLockId,
address _to,
uint256 _toLockId
uint256 _toLockId,
uint256 _amount
)
external
{
_staking.transferFromLock(_account, _lockId, _amount, _to, _toLockId);
_staking.transferFromLock(_from, _fromLockId, _to, _toLockId, _amount);
}

function setLockAmount(Staking _staking, address _account, uint256 _lockId, uint256 _newAmount) external {
Expand Down
26 changes: 13 additions & 13 deletions test_embark/transfers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ contract('Staking app, Transfering', () => {
it('transfers', async () => {
//const initialTotalStake = await staking.totalStaked().call()
await approveAndStake()
await staking.transfer(DEFAULT_STAKE_AMOUNT / 2, user1, 0).send()
await staking.transfer(user1, 0, DEFAULT_STAKE_AMOUNT / 2).send()

assert.equal((await staking.unlockedBalanceOf(owner).call()).toString(), DEFAULT_STAKE_AMOUNT / 2, "Owner balance should match")
assert.equal((await staking.unlockedBalanceOf(user1).call()).toString(), DEFAULT_STAKE_AMOUNT / 2, "User 1 balance should match")
Expand All @@ -73,7 +73,7 @@ contract('Staking app, Transfering', () => {
await approveAndStake()
await token.mint(user1, DEFAULT_STAKE_AMOUNT).send()
const lockId = await approveStakeAndLock(user2, DEFAULT_LOCK_AMOUNT, DEFAULT_STAKE_AMOUNT, user1)
await staking.transfer(DEFAULT_LOCK_AMOUNT, user1, lockId).send()
await staking.transfer(user1, lockId, DEFAULT_LOCK_AMOUNT).send()

assert.equal((await staking.unlockedBalanceOf(owner).call()).toString(), DEFAULT_STAKE_AMOUNT - DEFAULT_LOCK_AMOUNT, "Owner balance should match")
assert.equal((await staking.unlockedBalanceOf(user1).call()).toString(), DEFAULT_STAKE_AMOUNT - DEFAULT_LOCK_AMOUNT, "User 1 balance should match")
Expand All @@ -87,14 +87,14 @@ contract('Staking app, Transfering', () => {
it('fails transfering zero tokens', async () => {
await approveAndStake()
return assertRevert(async () => {
await staking.transfer(0, user1, 0).send()
await staking.transfer(user1, 0, 0).send()
})
})

it('fails transfering more than unlocked balance', async () => {
await approveAndStake(DEFAULT_STAKE_AMOUNT)
return assertRevert(async () => {
await staking.transfer(DEFAULT_STAKE_AMOUNT + 1, user1, 0).send()
await staking.transfer(user1, 0, DEFAULT_STAKE_AMOUNT + 1).send()
})
})
})
Expand All @@ -103,7 +103,7 @@ contract('Staking app, Transfering', () => {
it('transfers', async () => {
const lockId = await approveStakeAndLock(lockManagerAddress)
const transferAmount = DEFAULT_LOCK_AMOUNT / 2
await lockManager.transferFromLock(stakingAddress, owner, lockId, transferAmount, user1, 0).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, 0, transferAmount).send()

assert.equal((await staking.unlockedBalanceOf(owner).call()).toString(), DEFAULT_STAKE_AMOUNT - DEFAULT_LOCK_AMOUNT, "Owner balance should match")
assert.equal((await staking.unlockedBalanceOf(user1).call()).toString(), transferAmount, "User 1 balance should match")
Expand All @@ -117,7 +117,7 @@ contract('Staking app, Transfering', () => {

it('transfers the whole lock amount', async () => {
const lockId = await approveStakeAndLock(lockManagerAddress)
await lockManager.transferFromLock(stakingAddress, owner, lockId, DEFAULT_LOCK_AMOUNT, user1, 0).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, 0, DEFAULT_LOCK_AMOUNT).send()

assert.equal((await staking.unlockedBalanceOf(owner).call()).toString(), DEFAULT_STAKE_AMOUNT - DEFAULT_LOCK_AMOUNT, "Owner balance should match")
assert.equal((await staking.unlockedBalanceOf(user1).call()).toString(), DEFAULT_LOCK_AMOUNT, "User 1 balance should match")
Expand All @@ -133,7 +133,7 @@ contract('Staking app, Transfering', () => {
const lockId = await approveStakeAndLock(lockManagerAddress)
await token.mint(user1, DEFAULT_STAKE_AMOUNT).send()
const toLockId = await approveStakeAndLock(user2, DEFAULT_LOCK_AMOUNT, DEFAULT_STAKE_AMOUNT, user1)
await lockManager.transferFromLock(stakingAddress, owner, lockId, DEFAULT_LOCK_AMOUNT, user1, toLockId).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, toLockId, DEFAULT_LOCK_AMOUNT).send()

assert.equal((await staking.unlockedBalanceOf(owner).call()).toString(), DEFAULT_STAKE_AMOUNT - DEFAULT_LOCK_AMOUNT, "Owner balance should match")
assert.equal((await staking.unlockedBalanceOf(user1).call()).toString(), DEFAULT_STAKE_AMOUNT - DEFAULT_LOCK_AMOUNT, "User 1 balance should match")
Expand All @@ -148,28 +148,28 @@ contract('Staking app, Transfering', () => {
await approveStakeAndLock(lockManagerAddress)
return assertRevert(async () => {
// it will fail at isLockManager modifier
await lockManager.transferFromLock(stakingAddress, owner, 0, DEFAULT_LOCK_AMOUNT, user1, 0).send()
await lockManager.transferFromLock(stakingAddress, owner, 0, user1, 0, DEFAULT_LOCK_AMOUNT).send()
})
})

it('fails transfering zero tokens', async () => {
const lockId = await approveStakeAndLock(lockManagerAddress)
return assertRevert(async () => {
await lockManager.transferFromLock(stakingAddress, owner, lockId, 0, user1, 0).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, 0, 0).send()
})
})

it('fails transfering more than locked balance', async () => {
const lockId = await approveStakeAndLock(lockManagerAddress)
return assertRevert(async () => {
await lockManager.transferFromLock(stakingAddress, owner, lockId, DEFAULT_LOCK_AMOUNT + 1, user1, 0).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, 0, DEFAULT_LOCK_AMOUNT + 1).send()
})
})

it('fails if sender is not manager', async () => {
const lockId = await approveStakeAndLock(lockManagerAddress)
return assertRevert(async () => {
await staking.transferFromLock(owner, lockId, DEFAULT_LOCK_AMOUNT, user1, 0).send({ from: user1 })
await staking.transferFromLock(owner, lockId, user1, 0, DEFAULT_LOCK_AMOUNT).send({ from: user1 })
})
})

Expand All @@ -178,7 +178,7 @@ contract('Staking app, Transfering', () => {
// unlock
await staking.unlock(owner, lockId).send({ from: user1 })
return assertRevert(async () => {
await lockManager.transferFromLock(stakingAddress, owner, lockId, DEFAULT_LOCK_AMOUNT, user1, 0).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, 0, DEFAULT_LOCK_AMOUNT).send()
})
})

Expand All @@ -189,7 +189,7 @@ contract('Staking app, Transfering', () => {
// unlock
await staking.unlock(user1, toLockId).send({ from: user2 })
return assertRevert(async () => {
await lockManager.transferFromLock(stakingAddress, owner, lockId, DEFAULT_LOCK_AMOUNT, user1, toLockId).send()
await lockManager.transferFromLock(stakingAddress, owner, lockId, user1, toLockId, DEFAULT_LOCK_AMOUNT).send()
})
})
})
Expand Down

0 comments on commit 6f7728f

Please sign in to comment.