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

fix: use adjusted time on estimate gas when latest block is being used #4287

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

luzzif
Copy link

@luzzif luzzif commented Mar 4, 2023

When estimating gas, use the adjusted timestamp in case the latest block tag is being used. This helps avoid some errors in time-dependent contracts when using Ganache in fork mode.

Should fix #3528.

This is my first PR here, would love some guidance.

@luzzif luzzif changed the title Use adjusted time on estimate gas when latest block is being used fix: use adjusted time on estimate gas when latest block is being used Mar 4, 2023
Copy link
Contributor

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

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

Thanks for opening this!

I've added a couple of thoughts - can you please also add tests to cover this behaviour?

| LegacyTransaction
| EIP2930AccessListTransaction
| EIP1559FeeMarketTransaction,
blockNumber: QUANTITY | Ethereum.Tag = Tag.latest
Copy link
Contributor

Choose a reason for hiding this comment

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

QUANTITY type shouldn't be used inside of the application logic - it's an alias type that's used to describe parameters in the API layer. Instead, accept type Quantity here, and do the conversion in the caller.

Comment on lines +597 to +599
let timestamp = previousHeader.timestamp;
if (blockNumber === "latest")
timestamp = Quantity.from(this.#adjustedTime(previousHeader.timestamp));
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to me that if the request is for "latest", this should be incrementing the timestamp, otherwise we use the timestamp of the previous block, rather than the "next" block.

@@ -577,6 +583,40 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {

coinbase: Address;

gasEstimateBlock = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be nice if this were a verb, ie createEstimateGasBlock or something.

I haven't the time to look into this further today, but I wonder if there's some value in combining the readyNextBlock and this function, as they are very similar (it might be that it just becomes too complicated to be worthwhile though).

@luzzif
Copy link
Author

luzzif commented May 25, 2023

A bit of delay on this but... I just implemented the same logic on eth_call on a local fork, but using a slightly different way. I made the readyNextBlock in the blockchain class public and when latest is passed as the block I use that instead of the standard built RuntimeBlock. I wonder if we could simplify the implementation of the eth_estimateGas logic by doing the same here. The only thing we probably need to change is that I noticed that in the eth_estimateGas case the gas limit defaults to the miner's call gas limit instead of the default block gas limit used in the standard readyNextBlock function.

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

Successfully merging this pull request may close these issues.

eth_estimateGas does not use blockchain's adjustedTime
2 participants