[added temporal snapshot/revert, added evm_decreaseTime, added tests for both]

[chris-shyft]

ftw <3

[naure]

Any reason not to let increaseTime accept negative values? That'd be one fewer public method.

[chris-shyft]

mainly because "increaseTime" as read by a developer would indicate time is going up, and a negative value on the variable is easy to mistake on a glance, while reading right to left of a function is pretty solid.

increaseTime(forwardValue)
increaseTime(-forwardValue)

vs

increaseTime(forwardValue)
decreaseTime(backwardsValue)

[chris-shyft]

plus reading pure function definitions in the readme (which I didn't update in this PR) would be immediately visible, while you'd have to create creating an extra note surrounding a negative value being acceptable in the alternative.

[benjamincburns]

Hi @chris-shyft!

Thanks for this! Just so I don't repeat myself, can you please review my comments over on #95 and let me know what you think?

Also FWIW, given that this PR seems to more directly address #390, I'm slightly more inclined to accept it than #95. Perhaps you and @PhABC might want to get in touch and coordinate changes?

[chris-shyft]

sure thing :)

so the reason for an additional evm_decreaseTime instead of using the snapshot directly is that I want to create a timelock contract test.

B = current blocktime
T = timelock end time

#1, I create a transaction that has a timelock to time B + T.
#2, I create a transaction that tries to withdraw at time B (should fail)
#3, I increase the time to B + T
#4, I create a transaction that tries to withdraw at time T (should pass)
#5, I decrease the time to B + (T - T)
(residual here is that the #4 is still kept around as the final result of that withdrawal)

then I continue with tests in the same truffle test file.

now, I could revert instead of decreasing the time. that would most likely also produce proper logical test results (probably even more "proper" than decreasing the time after running a transaction), except that because I know the final values of specific interactions, I wanted to keep these known values rolling within the tests instead of making notes that while X value was changed, it has been reverted in the next it().

simply test logic here.

my main goal with this was having snapshot/revert function properly, and I added the evm_decreaseTime because it was useful for my test logic flow. I think it'll be useful for people, but I can't specify an exact use-case, so my only proverbial "skin in the game" here is that I'll have to re-write my tests and/or have a fork that uses my methods for evm_decreaseTime.

[benjamincburns]

I wanted to keep these known values rolling within the tests instead of making notes that while X value was changed, it has been reverted in the next it()

Just to make sure you're aware, Truffle automatically runs evm_snapshot and evm_revert between tests. Given that #2 is now merged, I think you'll likely find you don't need evm_decreaseTime.

I agree however that without truffle doing the snapshot/revert adding an evm_decreaseTime is a bit more readable, and therefore nicer.

[chris-shyft]

yep I'm aware. it wasn't between test files (contract("xyx") {}) it was between tests within the same contract("xyz") {}.

[PhABC]

@benjamincburns If #2 was merged and revert is ran between every tests, why is the timestamp still not reverted between unit tests?

I'm specifically referring to unit tests within the same test, where following time sensitive tests will fail after increasing timestamp. AFAIK, there is no evm_revert executed between unit tests within the same test file, which makes evm_decreaseTime more appropriate for simplicity in cases like the example above.

Here's an example where the second test fails when place before after the timestamp increase, but succeed if executed before ;

it('should REVERT when expiration is passed', async function () {

  //Increasing time
  await web3.currentProvider.send({
    jsonrpc: "2.0", 
    method: "evm_increaseTime", 
    params: [timeToExpire], 
    id: 0
  });

  ...

}); 

// This test will fail because of previous test which increase the timestamp
it('should PASS when expiration is NOT passed', async function () {
  ...
}); 

[benjamincburns]

@PhABC I believe I was mistaken before - truffle calls evm_snapshot and evm_revert between test files, not individual tests. If we were to support evm_decreaseTime without reverting state, we'd also need to ensure that we were following the requirements around monotonicity of timestamps imposed by the protocol, meaning that most of the time an evm_decreaseTime followed by an evm_mine would still result in an error.

The correct way to handle this case is still to use evm_snapshot and evm_revert, as this causes the blockchain to actually behave as though it's gone back in time. You can call these methods yourself manually in your tests.

In the mean time we've added support for mining a block at an exact timestamp, and we've added the ability to set the time to a specific timestamp. Between these new features and snapshot/revert, hopefully your needs are covered.