Skip to content
This repository has been archived by the owner on Jun 29, 2018. It is now read-only.

Deal tests #11

Merged
merged 21 commits into from
Mar 2, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
21 changes: 15 additions & 6 deletions src/lib/eth.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,21 @@ const Eth = {
contract.setProvider(provider);
return contract;
},
async deployContract(name, options = {}) {
deployContract(name, options = {}) {
const contractBody = Eth.loadContract(name);
contractInstance = await contractBody.deploy({arguments: options.params}).send(options.txParams);
// Workaround, see https://github.com/FrontierFoundry/web3-server-tools/issues/3
contractInstance.setProvider(provider);
return contractInstance;
txParams = {
from: options.txParams.from,
gas: options.txParams.gas,
gasPrice: options.txParams.gasPrice
}
return contractBody.deploy({arguments: options.params}).send(txParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should get eslint on this project. small things; add const txParams (undefined var otherwise)

i did think perhaps you should check options.txParams exists, otherwise code will throw error. however, thinking about it, it is probably better that it throws the error (and the error will be pretty clear)

.then(contractInstance => {
// Workaround, see https://github.com/FrontierFoundry/web3-server-tools/issues/3
contractInstance.setProvider(provider);
return contractInstance;
}).catch(e => {
console.log('error while deploying contract: ', e);
})
},
findTransactions(address) {
logger.debug('findTransactions(address)', address);
Expand Down Expand Up @@ -188,7 +197,7 @@ const Eth = {
.then(([gasPrice, transactionCount]) => {
logger.debug('gas price', gasPrice);
const gasPriceHex = web3.utils.numberToHex(gasPrice);
const gasLimitHex = web3.utils.numberToHex(68308);
const gasLimitHex = web3.utils.numberToHex(683080);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be defined in an env var or some config.


const tra = {
gasPrice: gasPriceHex,
Expand Down
2 changes: 1 addition & 1 deletion test/mocha.opts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
--recursive
--require ./test/setup
--timeout 10000
--timeout 20000
25 changes: 0 additions & 25 deletions test/spec/lib/deal-factory.spec.js

This file was deleted.

140 changes: 127 additions & 13 deletions test/spec/lib/deal-token.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,137 @@ const Wallet = require('../../../src/models/wallet'),
BigNumber = require('bignumber.js'),
{accounts, keys} = require('../../accounts');

describe('DealToken', () => {

const testAccount = {
address: accounts[0], // '0x31767228EE17C34a821b0aF50E45C705506E32e4',
privateKey: keys[0], // '0xb3bbe022606f7b7345df92ab110d1582566a7be7487e34f5085dd49cb5236369',
};
let deal;
const testAccount = {
address: accounts[0], // '0x31767228EE17C34a821b0aF50E45C705506E32e4',
privateKey: keys[0], // '0xb3bbe022606f7b7345df92ab110d1582566a7be7487e34f5085dd49cb5236369',
};

const options = {
params: [],
txParams: {from: testAccount.address, gas: '6712388', gasPrice: '0x174876e800'},
};

describe('Deal Token', () => {
before(() => Wallet.deleteById(testAccount.address));
let dealToken;

/**
* implement the same tests as in truffle/test/spec/deal-token.spec.js
* but without using truffle-contract/truffle.
*
*/
});
function checkBalance(account, expected) {
return dealToken.methods.balanceOf(account).call()
.then((balance) => {
console.log('balance of token', balance);
assert.equal(expected, balance);
});
}


function mintTokens(account, amountToMint) {
return dealToken.methods.mint(account, amountToMint).send(options.txParams)
.then(() => dealToken.methods.approve(account, amountToMint).send(options.txParams));
}


function authorize(account) {
console.log('authorizing');
return dealToken.methods.authorizeAddress(account, true).send(options.txParams);
}

function unauthorize(account) {
console.log('authorizing');
return dealToken.methods.authorizeAddress(account, false).send(options.txParams);
}

function checkAuthorized(account, expected) {
console.log('checking auth');
return dealToken.methods.isAuthorized(account).call()
.then((authorized) => assert.equal(expected, authorized));
}

function transfer(from, to, amount, shouldFail) {
return dealToken.methods.transferFrom(from, to, amount).send(options.txParams)
.then((result) => {
if (shouldFail) {
console.log('transfer success - should happen now', result);
console.log('transfer logs ', JSON.stringify(result.logs));
// should not reach this code
assert.fail('transfer succeeded', 'transfer should fail');
}
})
.catch((e) => {
// if transfer should fail this is ok
if (!shouldFail) {
console.log('transfer failed', e);
assert.fail('transfer failed', 'transfer should succeed');
}
});
}

const ownerAccount = accounts[0];
const userAccount = accounts[1];

beforeEach(() => {
console.log('redeploying token');
return Eth.deployContract('DealToken', options)
.then(result => {
dealToken = result;
})
});

it('should get instance of lp token', () => {
assert.isNotNull(dealToken);
});

it('should get zero balance of lp token', () => checkBalance(userAccount, 0));

it('should authorize ', () => {
return checkAuthorized(userAccount, false)
.then(() => authorize(userAccount))
.then(() => checkAuthorized(userAccount, true));
});

it('should mint tokens ', () => {
return authorize(userAccount)
.then(() => checkBalance(userAccount, 0))
.then(() => mintTokens(userAccount, 25))
.then(() => checkBalance(userAccount, 25));
});

it('should transfer tokens to authorized address ', () => {
const shouldFail = false;
return checkBalance(ownerAccount, 0)
.then(() => authorize(ownerAccount))
.then(() => authorize(userAccount))
.then(() => mintTokens(ownerAccount, 100))
.then(() => checkBalance(ownerAccount, 100))
.then(() => transfer(ownerAccount, userAccount, 33, shouldFail))
.then(() => checkBalance(userAccount, 33))
.then(() => checkBalance(ownerAccount, 67));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since each then has no argument dependencies, we could parallelize some of these calls if this test is long-running (noticed the timeout). Promise.all([checkBalance(userAccount, 33), checkBalance(ownerAccount, 67)]) or similar

Copy link
Contributor

Choose a reason for hiding this comment

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

as you likely see, the ordering here is important for the first two.

yes, you could do the last two (as you suggest). i don't think this will be a big impact on speed though. to be clear, @naumenkogs - she means replacing 126+127 with .then(() => Promise.all([checkBalance(userAccount, 33), checkBalance(ownerAccount, 67)]))

i'm not sure i would do this here (code readability>minimal performance), but in general i agree that calling promises async where you can is good practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, the timeout is because he is using a setTimeout instead of evm_increaseTime - that would be the slowness you are seeing. i think it should be changed to evm_increaseTime if not already done

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that for single tests it may not be a huge improvement, but we did also have to up the mocha timeout. As we get more and more tests with more and more expectations, this will only grow.

RE: readability, Promise.all is ugly but can be made more readable with await: [await foo(), await bar()]

Copy link
Contributor

Choose a reason for hiding this comment

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

on readability - sure, but i'd stick it in a helper checkBalances to keep the test easy to read. re time, not sure you saw the comment about evm_increaseTime - that is the problem here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, just seeing the comment above now 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'm not sure evm_increaseTime will help a lot here. I think the timeout is caused by the fact that I have to check "deal time-window" works well. And to be sure, I make windows bigger. It's easy to make mistakes by running unrealistic evm_increaseTime I think. And it's not that long.

Copy link
Contributor

Choose a reason for hiding this comment

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

to be clear; i am suggesting you use evm_increaseTime to skip the ganache EVM into the future. right now, you are using setTimeout in a few places instead. i believe evm_increaseTime would mean you don't need to use setTimeout (which causes a wall-clock (aka real) time delay )

});


it('should not transfer tokens to an unauthorized address', () => {
const shouldFail = true;
return checkBalance(ownerAccount, 0)
.then(() => authorize(ownerAccount))
.then(() => mintTokens(ownerAccount, 100))
.then(() => transfer(ownerAccount, userAccount, 40, shouldFail))
.then(() => checkBalance(userAccount, 0))
.then(() => checkBalance(ownerAccount, 100));
});

// Do we want unauthorized people to be able to send?
// Do we really nead unauthorizing action then (removal from the whitelist)?
it.skip('should not transfer tokens from an unauthorized address', () => {
const shouldFail = true;
return checkBalance(ownerAccount, 0)
.then(() => authorize(userAccount))
.then(() => authorize(ownerAccount))
.then(() => mintTokens(ownerAccount, 100))
.then(() => unauthorize(ownerAccount))
.then(() => transfer(ownerAccount, userAccount, 40, shouldFail))
.then(() => checkBalance(userAccount, 0))
.then(() => checkBalance(ownerAccount, 100));
});

});


Loading