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, accounts/abi/bind/backends: implement estimate_gas_list #21268

Closed
wants to merge 2 commits into from

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Jun 28, 2020

This PR is a reworked version based on the #21106

If we want to merge this PR, please don't squash in order to keep the original commit from @kvhnuke

What's more, if we consider accepting this change, maybe we should also support executing a batch of
calls.

@adamschmideg
Copy link
Contributor

adamschmideg commented Aug 4, 2020

@kvhnuke please open another PR to the gh-pages branch that documents this method. The docs should be on this page:
https://geth.ethereum.org/docs/rpc/ns-eth

@kvhnuke
Copy link

kvhnuke commented Sep 3, 2020

Just saw this request, will do asap thank you

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Minor comments, in general this looks good to me


// EstimateGasList returns an estimate of the amount of gas needed to execute list of
// given transactions against the current pending block.
func (b *SimulatedBackend) EstimateGasList(ctx context.Context, calls []ethereum.CallMsg) ([]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.

I personally prefer EstimateGasSequenced or SequencedEstimateGas -- then we could later have other Sequenced... RPC apis, and try to gather the info about all eth.sequencedXXX in one place

}

// copy returns the copied instance of call environment.
func (callctx *callContext) copy() *callContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer a less generic name, perhaps sequencedCallContext

kvhnuke and others added 2 commits September 29, 2020 14:21
add multiple gas estimation

merge

fix test errors ethereum#21062

fix previous state error

lint

set empty previousState if not set

cleanup and create state copy during DoCall

create eth_estimateGasList method

add comments for eth_estimateGasList

enfore gasCap on eth_estimateGasList
@PhABC
Copy link

PhABC commented Jun 28, 2021

Any chance we could revive this PR? Seems like a tiny change and would be very useful to a smart contract wallet I'm working on which supports sequenced transactions.

Happy to do a seperate PR and do the rebase + address @holiman's comments

@fjl
Copy link
Contributor

fjl commented Jun 29, 2021

@PhABC it's great that you want to work on it! Please note though: adding RPC functionality needs some
additional work beyond just implementing it in geth. We also need to change the documentation and
propose the new method in a PR to the eth JSON-RPC specs.

For now though, you can start by rebasing the PR.

@kvhnuke If you still want this feature, would be nice if it could be proposed as an official addition to the spec.
The proposal doesn't need to be an EIP. I think a PR to the eth1.0 specs repo would be sufficient.

@PhABC
Copy link

PhABC commented Jun 29, 2021

@fjl Going back on my word, we found an alternative method of estimating gas usage of sequenced transactions without needing this new endpoint, so we likely won't be needing this method. Other people may still find it useful however.

@rjl493456442
Copy link
Member Author

Close it since @s1na 's work.

@pkieltyka
Copy link

@rjl493456442 cool :) can you link to the PR with the alternate approach/work?

@rjl493456442
Copy link
Member Author

@pkieltyka #25743 it's batch_call, and I think batch_estimateGas should be pretty similar and I am pretty sure that sina will take on it :)

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.

7 participants