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

Hard coded dated seems to be outdated and PreICOProxyBuyer State enums doesn't match with Crowdsale enums #138

Open
voith opened this issue Jul 31, 2018 · 5 comments

Comments

@voith
Copy link
Collaborator

voith commented Jul 31, 2018

https://github.com/TokenMarketNet/ico/blob/d80e6d1afe8569512e0b53e6e927354ef476b7bf/ico/tests/contracts/test_deploy_acceptance.py#L70-L72
In the code above it can be seen that the date has been hardcoded. Howver, the date has already passed away.
https://github.com/TokenMarketNet/ico/blob/d80e6d1afe8569512e0b53e6e927354ef476b7bf/ico/tests/contracts/test_deploy_acceptance.py#L97-L107
The above mentioned date is used to instantiate PreICOProxyBuyer.
The PreICOProxyBuyer contract has the following states:
https://github.com/TokenMarketNet/ico/blob/d80e6d1afe8569512e0b53e6e927354ef476b7bf/contracts/PreICOProxyBuyer.sol#L74
The getState function has the following logic:
https://github.com/TokenMarketNet/ico/blob/d80e6d1afe8569512e0b53e6e927354ef476b7bf/contracts/PreICOProxyBuyer.sol#L305-L318
and the test has the following assertion:
https://github.com/TokenMarketNet/ico/blob/d80e6d1afe8569512e0b53e6e927354ef476b7bf/ico/tests/contracts/test_deploy_acceptance.py#L107

Since now >= freezeEndsAt, getState will return State.Refunding which has a enum value of 3,
and so the above assertion will fail.

The test was added on 28/08/2017 and the freezeEndsAt date being 20/09/2017, the test might have worked well at the time of adding.
What I don't understand is that this test passed successfully till date, but after upgrading populus the test seems to fail.

how can it be fixed?

Simply set the date to a distant future value.

@voith voith changed the title Hard coded dated seems to be outdated and PreICOProxyBuyer State enums doesn't match with cr Hard coded dated seems to be outdated and PreICOProxyBuyer State enums doesn't match with Crowdsale enums Jul 31, 2018
@voith voith changed the title Hard coded dated seems to be outdated and PreICOProxyBuyer State enums doesn't match with Crowdsale enums Hard coded dated seems to be outdated and PreICOProxyBuyer State enums doesn't match with Crowdsale enums Jul 31, 2018
@voith
Copy link
Collaborator Author

voith commented Jul 31, 2018

@villesundell Here's some investigation. The old populus used eth-testrpc, but the new populus uses eth-tester for testing.

eth-tester sets timestamp of the genesis block as the current_time:
https://github.com/ethereum/eth-tester/blob/41c6fd155db0f9cb1e79fdfef5b83c10f42d6248/eth_tester/backends/pyevm/main.py#L141
eth-testrpc has a hardcoded timestamp https://github.com/ethereum/pyethereum/blob/ddaac54b35ecbfb9d49fc48967722a77c8e0c4f4/ethereum/tester.py#L182.

time_travel has nothing to do with this. The hardcoded value fooled us to believe that our test-suite was working fine

@petri
Copy link
Contributor

petri commented Jul 31, 2018

Re ^ just setting the date to a distant future value in the test, as a fix: how about instead adding a delta to current timestamp?

@voith
Copy link
Collaborator Author

voith commented Jul 31, 2018

@petri That's a better fix.

@petri
Copy link
Contributor

petri commented Jul 31, 2018

BTW looks like we have another implementation as well (with a nice way to set the freeze end):

https://github.com/TokenMarketNet/ico/blob/94517d20b5a2c6a7b52717a063f4870e76dec7d1/ico/tests/contracts/test_preico_proxy_buy.py#L54-L57

And both of these seem to have wrong return type in the annotation.

@voith
Copy link
Collaborator Author

voith commented Jul 31, 2018

We don't have mypy enabled yet to tell us about annotation bugs

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

No branches or pull requests

2 participants