smart-contracts icon indicating copy to clipboard operation
smart-contracts copied to clipboard

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

Open voith opened this issue 7 years ago • 5 comments

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 avatar Jul 31 '18 07:07 voith

@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

voith avatar Jul 31 '18 11:07 voith

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?

petri avatar Jul 31 '18 11:07 petri

@petri That's a better fix.

voith avatar Jul 31 '18 11:07 voith

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.

petri avatar Jul 31 '18 12:07 petri

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

voith avatar Jul 31 '18 12:07 voith