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

accounts/abi.bind: don't fetch head in transact unless required #25988

Merged
merged 1 commit into from
Nov 4, 2022
Merged

accounts/abi.bind: don't fetch head in transact unless required #25988

merged 1 commit into from
Nov 4, 2022

Conversation

saman-pasha
Copy link
Contributor

if GasFeeCap and GasTipCap are specified no need to retrieve head

if GasFeeCap and GasTipCap are specified no need to retrieve head
@Szybka22
Copy link

.devcontainer/Dockerfile

@MariusVanDerWijden
Copy link
Member

The change looks kinda reasonable to me, could you explain your reasoning a bit more and add a test maybe?

@rjl493456442
Copy link
Member

rjl493456442 commented Oct 19, 2022

@MariusVanDerWijden I guess it's for avoiding fetching head. Since we are already in London fork and 1559 is already activated, it's unnecessary to apply the check(header.basefee == nil) at this moment.

But personally I still feel the check is necessary, mostly for other geth-based chains. They may or may not activate the 1559.

@saman-pasha
Copy link
Contributor Author

The change looks kinda reasonable to me, could you explain your reasoning a bit more and add a test maybe?

I have used abigen to generate code for create and sends some transactions to a contract, but in some cases I dont need to send a tx I only need to produce a tx so I have used TransactOpts.NoSend and I send nil to NewContract instance as the client.
When I set GasPrice I dont get any error and tx is produced but when I set GasFeeCap and GasTipCap to use tx.Type = 2 EIP 1559, it returns nil error because it feches head to set fees and needs a client insrance, so if it checks those fees are set no need to fetch head and tx is produced

@rjl493456442
Copy link
Member

Maybe you can just setup the client instance?

@saman-pasha
Copy link
Contributor Author

When I want to use EIP-712 to produce input data and I dont have access to Internet so fetching head is just an overhead and a problem

@rjl493456442
Copy link
Member

I get your point, but I don't think we will hold the condition that: sign transaction doesn't need any network connection.

There are a few cases that we need to retrieve information from the node to fulfill the transaction a bit

  • GasLimit is missing: estimate gas usage
  • GasPrice is missing: query price oracle for recommendation
  • Nonce is missing: retrieve nonce

Generally, the network access is required in our library I think.

@rjl493456442
Copy link
Member

Clef is another tool which designed for off-line signing, which also supports EIP-712. Maybe it's something suitable for you?https://geth.ethereum.org/docs/clef/introduction

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.

LGTM

@holiman holiman changed the title Update base.go transact method accounts/abi.bind: don't fetch head in transact unless required Nov 4, 2022
@holiman holiman merged commit 33e23ee into ethereum:master Nov 4, 2022
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
…reum#25988)

If GasFeeCap and GasTipCap are specified, we don't need to retrieve the head block for constructing a transaction
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.

5 participants