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

Implement the eth_estimateGas JSON-RPC endpoint #81

Merged
merged 4 commits into from
Feb 26, 2024

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Feb 26, 2024

Residual from: #9

Description

Utilizes the new interface of EVM.run inside a Cadence script, to run an EVM transaction in order to retrieve the gas consumption and any possible failures. No state change occurs.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@m-Peter m-Peter added this to the Flow-EVM-M2 milestone Feb 26, 2024
@m-Peter m-Peter self-assigned this Feb 26, 2024
@m-Peter m-Peter requested a review from devbugging February 26, 2024 14:23

access(all)
fun main(encodedTx: [UInt8]): [UInt64; 2] {
let coa <- EVM.createCadenceOwnedAccount()
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use evm account resource:

    let account = getAuthAccount<auth(Storage) &Account>(Address(0xCOA))

    let coa = account.storage.borrow<auth(EVM.Call) &EVM.CadenceOwnedAccount>(from: /storage/evm)
        ?? panic("Could not borrow reference to the COA!")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch 👍 Fixed in eeea98e

I didn't use any entitlements, because EVM.run does not have any. It is simply defined as:

access(all) fun run(

data = *args.Data
} else if args.Input != nil {
data = *args.Input
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what if neither, have another else with error

Copy link
Collaborator Author

@m-Peter m-Peter Feb 26, 2024

Choose a reason for hiding this comment

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

Actually, the TX can also be a value transfer, so it is legit to not provide the input or data fields.

)

// address: 658bdf435d810c91414ec09147daa6db62406379
const eoaTestAccount1KeyHex = "9c647b8b7c4e7c3490668fb6c11473619db80c93704c70893d3813af4090c39c"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better to use the configured evm account, better than have random keys hardcoded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice idea 👍
Updated in fac85cb

func (r *rpcTest) estimateGas(
from common.Address,
data []byte,
gas uint64,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is gas here? gas limit or gas price? better to rename

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point 👍
Updated in fac85cb

@@ -128,6 +140,9 @@ func (e *EVM) SendRawTransaction(ctx context.Context, data []byte) (common.Hash,
uint64(len(data)),
),
)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🏅

e.logger.Panic().Msg(fmt.Sprintf("failed to convert value to array: %v", value))
}

result := value.(cadence.Array)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a todo here, to have a decoder for EVM.Result. For now this is ok but in future I would want to have unified decoder from EVM.Result back to Go type so we can reuse it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I think we might need it for the other scripts/transactions that use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added TODO in fac85cb

@@ -319,3 +366,82 @@ func bytesFromCadenceArray(value cadence.Value) ([]byte, error) {

return res, nil
}

func getErrorForCode(errorCode uint64) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth adding a todo that this would make sense to live inside flow-go/evm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I will probably move it there, in the errors file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added TODO in fac85cb

Copy link
Contributor

@devbugging devbugging left a comment

Choose a reason for hiding this comment

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

Left some more comments, but otherwise looks good.

@m-Peter m-Peter merged commit f9a0352 into onflow:gregor/dev Feb 26, 2024
@m-Peter m-Peter deleted the eth-estimate-gas-endpoint branch February 26, 2024 19:13
@devbugging devbugging mentioned this pull request Feb 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants