Skip to content

Commit

Permalink
Added isContract check to contract functions. Added missing tests. Fi…
Browse files Browse the repository at this point in the history 
…xed doc strings.
  • Loading branch information
@willjgriff
willjgriff committed Oct 1, 2019
1 parent 74550f9 commit 900a0c6
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 31 deletions.
49 changes: 28 additions & 21 deletions contracts/TokenRequest.sol
View file
Expand Up @@ -9,9 +9,9 @@ import "./lib/UintArrayLib.sol";
import "./lib/AddressArrayLib.sol";

/**
* Expected use requires the FINALISE_TOKEN_REQUEST_ROLE permission be given exclusively to a forwarder. A user can then
* request tokens by calling createTokenRequest() to deposit funds and then calling finaliseTokenRequest() which will be called
* via the forwarder if forwarding is successful, minting the user tokens.
* The expected use of this app requires the FINALISE_TOKEN_REQUEST_ROLE permission be given exclusively to a forwarder.
* A user can then request tokens by calling createTokenRequest() to deposit funds and then calling finaliseTokenRequest()
* which will be called via the forwarder if forwarding is successful, minting the user tokens.
*/
contract TokenRequest is AragonApp {

Expand All @@ -26,7 +26,7 @@ contract TokenRequest is AragonApp {

string private constant ERROR_TOO_MANY_ACCEPTED_TOKENS = "TOKEN_REQUEST_TOO_MANY_ACCEPTED_TOKENS";
string private constant ERROR_TOO_MANY_TOKEN_REQUESTS = "TOKEN_REQUEST_TOO_MANY_TOKEN_REQUESTS";
string private constant ERROR_TOKEN_ALREADY_ACCEPTED= "TOKEN_REQUEST_TOKEN_ALREADY_ACCEPTED";
string private constant ERROR_TOKEN_ALREADY_ACCEPTED = "TOKEN_REQUEST_TOKEN_ALREADY_ACCEPTED";
string private constant ERROR_TOKEN_NOT_ACCEPTED = "TOKEN_REQUEST_TOKEN_NOT_ACCEPTED";
string private constant ERROR_ADDRESS_NOT_CONTRACT = "TOKEN_REQUEST_ADDRESS_NOT_CONTRACT";
string private constant ERROR_NO_AMOUNT = "TOKEN_REQUEST_NO_AMOUNT";
Expand Down Expand Up @@ -63,8 +63,17 @@ contract TokenRequest is AragonApp {
event TokenRequestFinalised(uint256 requestId, address requester, address depositToken, uint256 depositAmount, uint256 requestAmount);

function initialize(address _tokenManager, address _vault, address[] _acceptedDepositTokens) external onlyInit {
require(isContract(_tokenManager), ERROR_ADDRESS_NOT_CONTRACT);
require(isContract(_vault), ERROR_ADDRESS_NOT_CONTRACT);
require(_acceptedDepositTokens.length <= MAX_ACCEPTED_DEPOSIT_TOKENS, ERROR_TOO_MANY_ACCEPTED_TOKENS);

for (uint256 i = 0; i < _acceptedDepositTokens.length; i++) {
address acceptedDepositToken = _acceptedDepositTokens[i];
if (acceptedDepositToken != ETH) {
require(isContract(acceptedDepositToken), ERROR_ADDRESS_NOT_CONTRACT);
}
}

tokenManager = TokenManager(_tokenManager);
vault = _vault;
acceptedDepositTokens = _acceptedDepositTokens;
Expand All @@ -77,6 +86,8 @@ contract TokenRequest is AragonApp {
* @param _tokenManager The new token manager address
*/
function setTokenManager(address _tokenManager) external auth(SET_TOKEN_MANAGER_ROLE) {
require(isContract(_tokenManager), ERROR_ADDRESS_NOT_CONTRACT);

tokenManager = TokenManager(_tokenManager);
emit SetTokenManager(_tokenManager);
}
Expand All @@ -86,6 +97,8 @@ contract TokenRequest is AragonApp {
* @param _vault The new vault address
*/
function setVault(address _vault) external auth(SET_VAULT_ROLE) {
require(isContract(_vault), ERROR_ADDRESS_NOT_CONTRACT);

vault = _vault;
emit SetVault(_vault);
}
Expand All @@ -96,13 +109,13 @@ contract TokenRequest is AragonApp {
*/
function addToken(address _token) external auth(MODIFY_TOKENS_ROLE) {
require(!acceptedDepositTokens.contains(_token), ERROR_TOKEN_ALREADY_ACCEPTED);
require(acceptedDepositTokens.length < MAX_ACCEPTED_DEPOSIT_TOKENS, ERROR_TOO_MANY_ACCEPTED_TOKENS);

if (_token != ETH) {
require(isContract(_token), ERROR_ADDRESS_NOT_CONTRACT);
}

acceptedDepositTokens.push(_token);
require(acceptedDepositTokens.length <= MAX_ACCEPTED_DEPOSIT_TOKENS, ERROR_TOO_MANY_ACCEPTED_TOKENS);

emit TokenAdded(_token);
}
Expand All @@ -119,7 +132,6 @@ contract TokenRequest is AragonApp {

/**
* @notice Create a token request depositing `@tokenAmount(_depositToken, _depositAmount, true, _depositToken.decimals(): uint256)` in exchange for `@tokenAmount(self.getToken(): address, _requestAmount, true, 18)`
* @dev Note the above radspec string seems to need to be on a single line. When split compile errors occur.
* @param _depositToken Address of the token being deposited
* @param _depositAmount Amount of the token being deposited
* @param _requestAmount Amount of the token being requested
Expand Down Expand Up @@ -156,10 +168,11 @@ contract TokenRequest is AragonApp {
*/
function refundTokenRequest(uint256 _tokenRequestId) external {
TokenRequest memory tokenRequestCopy = tokenRequests[_tokenRequestId];
delete tokenRequests[_tokenRequestId];

require(tokenRequestCopy.requesterAddress == msg.sender, ERROR_TOKEN_REQUEST_NOT_OWNER);

delete tokenRequests[_tokenRequestId];
addressesTokenRequestIds[msg.sender].deleteItem(_tokenRequestId);

address refundToAddress = tokenRequestCopy.requesterAddress;
address refundToken = tokenRequestCopy.depositToken;
uint256 refundAmount = tokenRequestCopy.depositAmount;
Expand All @@ -170,27 +183,23 @@ contract TokenRequest is AragonApp {
require(ERC20(refundToken).safeTransfer(refundToAddress, refundAmount), ERROR_TOKEN_TRANSFER_REVERTED);
}

addressesTokenRequestIds[msg.sender].deleteItem(_tokenRequestId);

emit TokenRequestRefunded(_tokenRequestId, refundToAddress, refundToken, refundAmount);
}

/**
* @notice Finalise the token request with id `_tokenRequestId`, minting the requester funds and moving payment
to the vault.
* @dev This function's FINALISE_TOKEN_REQUEST_ROLE permission is typically given exclusively to a forwarder.
* This contract also requires the MINT_ROLE on the TokenManager specified.
* It is recommended the forwarder is granted the FINALISE_TOKEN_REQUEST_ROLE permission to call this function
* before the MINT_ROLE permission on the TokenManager to prevent calling of this function before it has been
* restricted appropriately.
* This function requires the MINT_ROLE permission on the TokenManager specified.
* @param _tokenRequestId ID of the Token Request
*/
function finaliseTokenRequest(uint256 _tokenRequestId) external auth(FINALISE_TOKEN_REQUEST_ROLE) {
TokenRequest memory tokenRequestCopy = tokenRequests[_tokenRequestId];
delete tokenRequests[_tokenRequestId];

require(tokenRequestCopy.depositAmount > 0, ERROR_NO_DEPOSIT);

delete tokenRequests[_tokenRequestId];
addressesTokenRequestIds[requesterAddress].deleteItem(_tokenRequestId);

address requesterAddress = tokenRequestCopy.requesterAddress;
address depositToken = tokenRequestCopy.depositToken;
uint256 depositAmount = tokenRequestCopy.depositAmount;
Expand All @@ -204,8 +213,6 @@ contract TokenRequest is AragonApp {

tokenManager.mint(requesterAddress, requestAmount);

addressesTokenRequestIds[requesterAddress].deleteItem(_tokenRequestId);

emit TokenRequestFinalised(_tokenRequestId, requesterAddress, depositToken, depositAmount, requestAmount);
}

Expand All @@ -228,9 +235,9 @@ contract TokenRequest is AragonApp {
depositAmount = tokenRequest.depositAmount;
requestAmount = tokenRequest.requestAmount;
}

/**
* @dev convenience function for getting the token request token in a radspec string
* @dev Convenience function for getting the token request token in a radspec string
*/
function getToken() internal returns (address) {
return tokenManager.token();
Expand Down
112 changes: 102 additions & 10 deletions test/TokenRequestTest.js
View file
Expand Up @@ -16,7 +16,7 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => {
let daoDeployment = new DaoDeployment()
let requestableToken, tokenRequestBase, tokenRequest, tokenManager, tokenManagerBase, mockErc20, vaultBase, vault

let FINALISE_TOKEN_REQUEST_ROLE, MINT_ROLE, SET_TOKEN_MANAGER_ROLE, SET_VAULT_ROLE
let FINALISE_TOKEN_REQUEST_ROLE, MINT_ROLE, SET_TOKEN_MANAGER_ROLE, SET_VAULT_ROLE, MODIFY_TOKENS_ROLE
const ETH_ADDRESS = '0x0000000000000000000000000000000000000000'
let MOCK_TOKEN_BALANCE, ROOT_TOKEN_AMOUNT, ROOT_ETHER_AMOUNT

Expand All @@ -29,6 +29,7 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => {
FINALISE_TOKEN_REQUEST_ROLE = await tokenRequestBase.FINALISE_TOKEN_REQUEST_ROLE()
SET_TOKEN_MANAGER_ROLE = await tokenRequestBase.SET_TOKEN_MANAGER_ROLE()
SET_VAULT_ROLE = await tokenRequestBase.SET_VAULT_ROLE()
MODIFY_TOKENS_ROLE = await tokenRequestBase.MODIFY_TOKENS_ROLE()

tokenManagerBase = await TokenManager.new()
MINT_ROLE = await tokenManagerBase.MINT_ROLE()
Expand Down Expand Up @@ -81,14 +82,30 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => {
await tokenManager.initialize(requestableToken.address, false, 0)

mockErc20 = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE)
//await mockErc20.transfer(rootAccount, ROOT_TOKEN_AMOUNT)
})

describe('initialize(address _tokenManager, address _vault)', () => {
describe('initialize(address _tokenManager, address _vault, address[] _acceptedDepositTokens)', async () => {
it("reverts when passed non-contract address as token manager", async () => {
await assertRevert(tokenRequest.initialize(rootAccount, vault.address, []),
"TOKEN_REQUEST_ADDRESS_NOT_CONTRACT")
})

it("reverts when passed non-contract address as vault", async () => {
await assertRevert(tokenRequest.initialize(tokenManager.address, rootAccount, []),
"TOKEN_REQUEST_ADDRESS_NOT_CONTRACT")
})

it("reverts when passed non-contract address in accepted deposit tokens", async () => {
await assertRevert(tokenRequest.initialize(tokenManager.address, vault.address, [ETH_ADDRESS, rootAccount]),
"TOKEN_REQUEST_ADDRESS_NOT_CONTRACT")
})
})

describe('initialize(address _tokenManager, address _vault, address[] _acceptedDepositTokens)', () => {
let acceptedDepositTokens

beforeEach(async () => {
let acceptedDepositTokens = new Array(2)
acceptedDepositTokens[0] = mockErc20.address
acceptedDepositTokens[1] = ETH_ADDRESS
acceptedDepositTokens = [mockErc20.address, ETH_ADDRESS]
await tokenRequest.initialize(tokenManager.address, vault.address, acceptedDepositTokens)
})

Expand All @@ -101,25 +118,100 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => {
})

describe('setTokenManager(address _tokenManager)', () => {
it('sets a token manager', async () => {
const expectedTokenManagerAddress = accounts[2]
beforeEach(async () => {
await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, SET_TOKEN_MANAGER_ROLE, rootAccount)
})

it('sets a token manager', async () => {
const expectedTokenManagerAddress = tokenManager.address
await tokenRequest.setTokenManager(expectedTokenManagerAddress, { from: accounts[1] })

const actualTokenManager = await tokenRequest.tokenManager()
assert.strictEqual(actualTokenManager, expectedTokenManagerAddress)
})

it('reverts when setting non-contract address', async () => {
await assertRevert(tokenRequest.setTokenManager(rootAccount, { from: accounts[1] }),
'TOKEN_REQUEST_ADDRESS_NOT_CONTRACT')
})
})

describe('setVault(address _vault)', () => {
it('sets a vault', async () => {
const expectedVaultAddress = accounts[2]
beforeEach(async () => {
await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, SET_VAULT_ROLE, rootAccount)
})

it('sets a vault', async () => {
const expectedVaultAddress = vault.address
await tokenRequest.setVault(expectedVaultAddress, { from: accounts[1] })

const actualVault = await tokenRequest.vault()
assert.strictEqual(actualVault, expectedVaultAddress)
})

it('reverts when setting non-contract address', async () => {
await assertRevert(tokenRequest.setVault(rootAccount, { from: accounts[1] }),
'TOKEN_REQUEST_ADDRESS_NOT_CONTRACT')
})
})

describe('addToken(address _token)', async () => {
beforeEach(async () => {
await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, MODIFY_TOKENS_ROLE, rootAccount)
})

it('adds a token', async () => {
const newToken = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE)
const expectedTokens = [...acceptedDepositTokens, newToken.address]

await tokenRequest.addToken(newToken.address, { from: accounts[1] })

const actualTokens = await tokenRequest.getAcceptedDepositTokens()
assert.deepStrictEqual(actualTokens, expectedTokens)
})

it('cannot add more than max tokens', async () => {
const maxTokens = await tokenRequest.MAX_ACCEPTED_DEPOSIT_TOKENS();
for (let i = 0; i < maxTokens - 2; i++) {
const token = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE)
await tokenRequest.addToken(token.address, { from: accounts[1] })
}

const overflowToken = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE)
await assertRevert(tokenRequest.addToken(overflowToken.address, { from: accounts[1] }),
'TOKEN_REQUEST_TOO_MANY_ACCEPTED_TOKENS')
})

it('reverts when adding non-contract address', async () => {
await assertRevert(tokenRequest.addToken(rootAccount, { from: accounts[1] }),
'TOKEN_REQUEST_ADDRESS_NOT_CONTRACT')
})

it('reverts when adding already added token', async () => {
await assertRevert(tokenRequest.addToken(ETH_ADDRESS, { from: accounts[1] }),
'TOKEN_REQUEST_TOKEN_ALREADY_ACCEPTED')
})
})

describe('removeToken(address _token)', async () => {
beforeEach(async () => {
await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, MODIFY_TOKENS_ROLE, rootAccount)
})

it('removes a token', async () => {
const expectedTokens = [ETH_ADDRESS]

await tokenRequest.removeToken(mockErc20.address, { from: accounts[1] })

const actualTokens = await tokenRequest.getAcceptedDepositTokens()
assert.deepStrictEqual(actualTokens, expectedTokens)
})

it('reverts when removing unaccepted token', async () => {
await assertRevert(tokenRequest.removeToken(rootAccount, { from: accounts[1] }),
'TOKEN_REQUEST_TOKEN_NOT_ACCEPTED')
})

})

describe('createTokenRequest(address _depositToken, uint256 _depositAmount, uint256 _requestAmount)', () => {
Expand Down

0 comments on commit 900a0c6

Please sign in to comment.