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

Factory tests #132

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from
Open

Factory tests #132

wants to merge 4 commits into from

Conversation

kangarang
Copy link

@kangarang kangarang commented May 22, 2018

This PR address #125, adding tests to eip20Factory.js

  • Clean up the current test
  • Add test for created mapping
  • Add tests for verifyEIP20
  • Add comments

@kangarang kangarang changed the title cleanup, add comments & test Factory tests May 23, 2018
@kangarang
Copy link
Author

^ Ready for review! Thanks @maurelian for the guidance 👍

const result = await factory.verifyEIP20.call(newTokenAddr, { from: accounts[0] });
assert.strictEqual(result, true, 'the bytecode at newTokenAddr '
+ 'was not the same as the bytecode of an EIP20 token');

Copy link
Contributor

Choose a reason for hiding this comment

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

for consistency, would add // verify: new token's address is listed in the isEIP20 mapping


const { contractAddress } = await web3.eth.getTransactionReceipt(txHash);
const result = await factory.verifyEIP20.call(contractAddress);
assert.strictEqual(result, true, 'should have returned true because the bytecode was exact');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is failing for me. Here's some output from inserting a debugger statement on the line above, and inspecting the run time:

code == EIP20Bytecode
false
code.length
6262
EIP20Bytecode.length
7076

It looks like the hardcoded bytecode above was generated by a different truffle/solc version than the one listed in package.json here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed this test by changing line 2 to
const EIP20Bytecode = artifacts.require('EIP20').bytecode;

It feels a bit circular, but it translates to: "does the bytecode from compiling EIP20, match the bytecode deployed by the EIP20Factory".

Copy link
Contributor

Choose a reason for hiding this comment

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

This test is also broken when rebased onto staging because the factory is using SafeEIP20 there, so this is dependent on that.

@kangarang
Copy link
Author

made those 2 changes. will wait til #135 gets merged into master then make changes to support SafeEIP20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants