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

feat: Implement evm_increaseTime and evm_setNextBlockTimestamp #93

Merged
merged 14 commits into from
Sep 11, 2023

Conversation

nbaztec
Copy link
Contributor

@nbaztec nbaztec commented Sep 4, 2023

What πŸ’»

Adds the following EVM rpc methods to the test node:

  • evm_increaseTime
  • evm_setNextBlockTimestamp
    • evm_setTime

Why βœ‹

  • Allows users to manipulate the node timestamp as per there specific preferences, which can be highly beneficial during testing.

Evidence πŸ“·

Passing unit tests:
image

Notes πŸ“

Fixes #66 & #68

SUPPORTED_APIS.md Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@nbaztec nbaztec changed the title feat: Implement evm_increaseTime and evm_setTime feat: Implement evm_increaseTime and evm_setNextBlockTimestamp Sep 7, 2023
@nbaztec
Copy link
Contributor Author

nbaztec commented Sep 7, 2023

@MexicanAce I changed the return types to native types u64 to allow for rpc calls to be numeric instead of the hex encoding that U64 requires.

@@ -86,7 +86,7 @@ The `status` options are:
| `EVM` | `evm_setBlockGasLimit` | `NOT IMPLEMENTED` | Sets the Block Gas Limit of the network |
| `EVM` | `evm_setIntervalMining` | `NOT IMPLEMENTED` | Enables (with a numeric argument greater than 0) or disables (with a numeric argument equal to 0), the automatic mining of blocks at a regular interval of milliseconds, each of which will include all pending transactions |
| `EVM` | `evm_setNextBlockTimestamp` | `NOT IMPLEMENTED`<br />[GitHub Issue #68](https://github.com/matter-labs/era-test-node/issues/68) | Works like `evm_increaseTime`, but takes the exact timestamp that you want in the next block, and increases the time accordingly |
| `EVM` | `evm_setTime` | `NOT IMPLEMENTED` | Sets the internal clock time to the given timestamp |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's actually a need for both here. Ganache only supports evm_setTime, while Hardhat only support evm_setNextBlockTimestamp ... but I think they both do the same thing. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can either implement evm_setTime in this PR, or leave the line here in this table.

Also, it looks like this change leaves 2 rows/entries for evm_setNextBlockTimestamp

Copy link
Contributor Author

@nbaztec nbaztec Sep 7, 2023

Choose a reason for hiding this comment

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

I think there's actually a need for both here. Ganache only supports evm_setTime, while Hardhat only support evm_setNextBlockTimestamp ... but I think they both do the same thing. Thoughts?

Yeah they do exactly the same thing with the following differences:

  • setNextBlockTimestamp allows only setting a timestamp forward in future, while setTime supports setting it backwards.
  • setNextBlockTimestamp returns the newly set timestamp in response, whereas setTime returns the difference between the current and the new timestamp.

AFAIK setTime can be seen as a superset of setNextBlockTimestamp and from most examples out there no one seems to be using the return value to do anything meaningful.

I'm fine with supporting both, though it must be explicitly mentioned (to avoid any confusion for users) that these two achieve the same result and are mostly arising out of API differences between hardhat and ganache - though it does tend to make our API interface a bit messy.

@dutterbutter
Copy link
Collaborator

@nbaztec thanks for the contribution! Can you address the conflicts on this PR please.

@dutterbutter dutterbutter added the needs review πŸ‘“ PR requires a review label Sep 7, 2023
Copy link
Collaborator

@MexicanAce MexicanAce left a comment

Choose a reason for hiding this comment

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

LGTM

@MexicanAce MexicanAce merged commit 40cf025 into matter-labs:main Sep 11, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review πŸ‘“ PR requires a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

evm_api: Implement evm_increaseTime API endpoint
3 participants