Skip to content

Adding gas estimate function#24

Merged
pdyraga merged 1 commit intomasterfrom
gas-estimate-function
Apr 10, 2020
Merged

Adding gas estimate function#24
pdyraga merged 1 commit intomasterfrom
gas-estimate-function

Conversation

@dimpar
Copy link
Contributor

@dimpar dimpar commented Apr 7, 2020

Refs threshold-network/keep-core#1479

In this PR we add the gas estimation functionality. This will be used by Keep applications to retrieve the estimation for a specific transaction.

- Added *GasEstimate template.
- Added EstimateGas() in ethutil.go
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

This looks good to me. Any objections to merging as-is? Looks like threshold-network/keep-core#1550 gave this a good exercising, so we should be good here.

msg := ethereum.CallMsg{
From: from,
To: &to,
Data: input,
Copy link
Member

Choose a reason for hiding this comment

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

We are not passing Value to CallMsg so the estimation may not be correct for payable functions. I think we need to generate a variation of estimate function just for payable functions.

It's not a top priority because the problem we are having in threshold-network/keep-core#1479 is solved with the code here. I'll open a separate issue to address payable functions.

Copy link
Member

@pdyraga pdyraga Apr 10, 2020

Choose a reason for hiding this comment

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

Captured this issue in #26

@pdyraga pdyraga merged commit 86a57ad into master Apr 10, 2020
@pdyraga pdyraga deleted the gas-estimate-function branch April 10, 2020 12:12
@pdyraga pdyraga added this to the v0.2.0 milestone Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants