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

automatic gas estimation #855

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

ncitron
Copy link
Contributor

@ncitron ncitron commented Nov 7, 2021

Description

Adds automatic gas estimation to seth mktx if ETH_GAS is not set. Since seth mktx is the underlying for seth send and dapp create, dapptools users will likely never have to input gas limits manually.

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@MrChico
Copy link
Member

MrChico commented Nov 7, 2021

Very nice, been wanting this for some time. I have fixed ci on master, could you rebase so we can run it again?

@ncitron
Copy link
Contributor Author

ncitron commented Nov 8, 2021

This should be good to go now. I've made a couple of changes so that instead of directly calling eth_estimateGas, I make all gas estimations through seth estimate (which I made a slight modification to).

I also made sure to do this gas estimation in both seth send and seth mktx. This is because if you use an RPC signer, just adding the estimation in seth mktx is not sufficient.

Copy link
Contributor

@d-xo d-xo left a comment

Choose a reason for hiding this comment

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

This is a very nice UX upgrade, thanks a lot! We definitely need tests before merging, (you can add tests to this file).

I'm a little worried that there may be some error (either in seth, or the node doing the estimating) that may result in us automatically setting a very high gas value. I think we should at least display the value that we get back from seth estimate and potentially also ask for user confirmation if we're running in an interactive terminal?

src/seth/libexec/seth/seth-estimate Show resolved Hide resolved
@@ -13,20 +13,34 @@ if [[ $2 ]]; then
data=$(seth calldata "${@:2}")
fi

data=${data:-0x}
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 put this in an else for the above if statement?

data=${data:-0x}

if [[ -z "$SETH_CREATE" ]]; then
TO=$(seth --to-address "$1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only for --create?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If SETH_CREATE is unset, then we are doing a normal transaction that has a recipient. If not, we are creating a contract, and TO must be unset since this is a requirement for a contract creation tx.

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.

None yet

4 participants