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

internal/ethapi: don't query wallets at every execution of gas estimation #20261

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 11, 2019

A user ( @sarvesh-ost ) reported that doing estimateGas with clef as a backend caused it to prompt for approval 26 times.
This PR changes the gas estimator so it resolves the address to use only once, not once for each execution.

@sarvesh-ost , could you please check if this solves the issue?

@karalabe
Copy link
Member

Could you do the same pls in account/abi/bind/backends/simulated.go?

@karalabe
Copy link
Member

Also pls fix the commit msg typos

@karalabe karalabe added this to the 1.9.8 milestone Nov 12, 2019
@fjl fjl changed the title ethernnal/ethapi: don't query wallets at every execution of gas estimation internal/ethapi: don't query wallets at every execution of gas estimation Nov 12, 2019
@fjl
Copy link
Contributor

fjl commented Nov 12, 2019

Why are we auto-setting the sender address at all?

@karalabe
Copy link
Member

karalabe commented Nov 12, 2019

Legacy API behavior, use account[0] if unset.

@karalabe
Copy link
Member

karalabe commented Nov 12, 2019

I'd be happy to start rejecting this behavior, but that's a breaking enough change IMHO to warrant a major Geth version bump (as in 1.10)

@holiman
Copy link
Contributor Author

holiman commented Nov 17, 2019

Could you do the same pls in account/abi/bind/backends/simulated.go?

Afaict, that one doesn't use the accounts. It takes an ethereum.CallMsg where From cannot be nil

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM

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