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

[Flow EVM] Gas estimation #5603

Closed
Tracked by #5536
sideninja opened this issue Mar 28, 2024 · 2 comments · Fixed by #5749
Closed
Tracked by #5536

[Flow EVM] Gas estimation #5603

sideninja opened this issue Mar 28, 2024 · 2 comments · Fixed by #5749
Assignees
Labels

Comments

@sideninja
Copy link
Contributor

sideninja commented Mar 28, 2024

Gas estimation is a required functionality of any EVM.

Our initial idea for supporting estimation was to implement it as part of the EVM gateway endpoint, and then internally EVM gateway would use a script to run the interaction and report the gas used. Since gas estimation endpoint doesn't take in a signed transaction but instead parameters like "from", "to", "data" etc https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_estimategas it required us to internally build the transaction payload and sign it with a "hardcoded" account because it was run as part of the script it didn't matter.

But this approach has a problem, it requires this test account to have a balance that is at least as big as the value provided to this endpoint and this requirement can not be satisfied. An idea then was to overwrite the value in the transaction we submit to the EVM using a script to 0, but the problem with this approach is that some contracts base their logic on the value received by the transaction, thus the estimation wouldn't be correct.

One approach to tackle this would be to implement gas estimation in the EVM contract and not do the checks for balances as well as make sure to revert the state after, so similar how the go ethereum implementation does it.

@sideninja sideninja self-assigned this Mar 28, 2024
@m-Peter
Copy link
Collaborator

m-Peter commented Mar 28, 2024

Basically, what geth does is transform the transaction arguments to a Message call and apply it without every committing the stage change. (see https://github.com/ethereum/go-ethereum/blob/master/internal/ethapi/api.go#L1188-L1190)

One alternative to implement this was something like:

access(all)
fun estimateGas(
    from: [UInt8; 20],
    to: [UInt8; 20]?,
    data: [UInt8],
    gasLimit: UInt64,
    value: Balance
): Result {
    let acc <-create CadenceOwnedAccount()
    acc.initAddress(addressBytes: from)
    
    // If `to` is present, we are dealing with a contract
    // call or a plain value transfer.
    if let toAddress = to {
        let txResult = acc.call(
            to: EVMAddress(bytes: toAddress),
            data: data,
            gasLimit: gasLimit,
            value: value
        )

        destroy <-acc

        return txResult
    }

    // Since `to` is absent, we are dealing with a contract
    // deployment.
    let txResult = acc.deploy(
        code: data,
        gasLimit: gasLimit,
        value: value
    ) as! Result

    destroy <-acc

    return txResult
}

adding the above function to EVM contract. However, the blocker with this is that it has to be available only for Cadence scripts, because if it ever runs on a Cadence transaction, it will commit the state changes, and that is unwanted behavior.

Note that besides eth_estimateGas, the same applies to eth_call. One possible solution is to have a new type of DirectCall, which applies the relevant transaction arguments, without ever committing the state change and simply returning an EVM.Result.

@ramtinms
Copy link
Contributor

I see the point now to introduce the estimated endpoint, I think we can implement this using the same way we skip signature verification for direct calls. we just have to make sure no way the changes are committed or applied.

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

Successfully merging a pull request may close this issue.

3 participants