Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Adding prototype.evm_decreaseTime #95

Closed
wants to merge 2 commits into from

Conversation

PhABC
Copy link

@PhABC PhABC commented Apr 2, 2018

It's currently hard to perform tests that are timestamp dependent since the timestamp changes between tests are shared (E.g. see #2, Ganache-cli-issue-390 and here) . Using evm_increaseTime leads to non-reversible timestamp changes that persist through all the tests.

Adding evm_decreaseTime allow developers to revert the timestamp change after each test, using a logic such as

beforeEach(async function () {
  await web3.currentProvider.send({
      jsonrpc: "2.0", 
      method: "evm_increaseTime", 
      params: [timeToIncrease], 
      id: 0
  });
});

afterEach(async function () {
  await web3.currentProvider.send({
      jsonrpc: "2.0", 
      method: "evm_decreaseTime", 
      params: [timeToIncrease], 
      id: 0
  });
});

@benjamincburns
Copy link
Contributor

Hi @PhABC,

Thanks for submitting this! Unfortunately, unless there's a specific reason why a fix to #390 won't work for your testing/development work, I'd rather accept a fix to that issue than an explicit evm_decreaseTime.

That is, a user should be able to call evm_snapshot, then call evm_increaseTime, then do whatever is necessary with the fast-forwarded timestamp, and finally call evm_revert with the expectation that the timestamp advancement will roll back along with whatever transactions were performed after evm_snapshot was called.

Can I request that you please either explain why the above workflow won't work for your use case, or rework this change to fix that issue?

@benjamincburns
Copy link
Contributor

@roderik since you reacted to this PR positively, please feel free to pipe up and say why you'd like to see this change go through. I'm very happy to accept if people need it - just want to make sure that it's not already covered by existing plans.

@roderik
Copy link

roderik commented Apr 12, 2018

I need it to test different stages in a crowdsale. Now I have to make 1 huge test with jumps between the tests for specific periods. Different test files do not work because it "reset" everything, except the time.

@benjamincburns
Copy link
Contributor

@roderik could you elaborate, please? By any chance is your code open? A link to a repo or a gist would help my feeble mind tremendously! 😄

@PhABC
Copy link
Author

PhABC commented Apr 12, 2018

@benjamincburns Thank you for the response!

I believe trufflesuite/ganache-cli#390 is fixed in this Pull Request (see my comments on that pull request).

I'm in favor of either method.

@roderik
Copy link

roderik commented Apr 12, 2018

Sure,

look at this repo: https://github.com/DataBrokerDAO/dtx-crowdsale-contracts
I worked around it in this case though.

I like my source files small, so it is more easy to get an overview of what is happening. Which means I would make lots of test files with specific things I wanted to test.

But multiple tests run at a random order. So if the first test that runs is one that jumps in time, all the other tests would run at that time in the future.

So If i wanted to test "cannot contribute before the sale" and "can contribute during the sale", it would be total guess work if they would work, because this depends if the "during saler" test runs first because it jumps ahead in time.

While the entire idea of the snapshots is that you start "clean" (but still faster than redeploy every time) at the beginning of each file.

@benjamincburns
Copy link
Contributor

@roderik given that #2 is now merged, do you still need evm_decreaseTime?

@benjamincburns
Copy link
Contributor

While the entire idea of the snapshots is that you start "clean" (but still faster than redeploy every time) at the beginning of each file.

@roderik as an aside, I've suggested internally that a cleaner approach to this might be if our tests were to use the forking feature of ganache-core. That would also enable tests to be run in parallel against multiple chains which were forked from an initial chain post-setup.

I'll raise it again during our next internal planning meeting.

@roderik
Copy link

roderik commented Apr 12, 2018

No, seems fine, do not have anything to test it on right now :)

@benjamincburns
Copy link
Contributor

@roderik I think you were the only one still advocating for this issue now that #2 is merged. Can you please reach out to me at [email protected] if this is still important to you?

Otherwise in the mean time I'm closing this to clean up our backlog some.

@wjmelements
Copy link

I would find this useful for gas-profiling contracts where a timestamp is passed as a parameter, because the zeroeness of the bytes of the timestamp affect the gas consumption. I can then profile as if it was way in the past without reconfiguring ganache for just this test.

Instead of evm_decreasetime, I think a better interface would be passing a negative value to evm_increasetime.

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

Successfully merging this pull request may close these issues.

5 participants