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

Support post EIP1559 tx types for RPC #2330

Closed
shresthagrawal opened this issue Oct 5, 2022 · 10 comments
Closed

Support post EIP1559 tx types for RPC #2330

shresthagrawal opened this issue Oct 5, 2022 · 10 comments

Comments

@shresthagrawal
Copy link

Current eth_estimateGas and eth_call do not support the post EIP1559 tx types

@acolytec3
Copy link
Contributor

I halfway fixed this out of necessity in #2316 but will need to be cleanly implemented in a separate PR. A minimal implementation will update this line to use TransactionFactory and also possibly update the RpcTx type to include maxFeePerGas and maxPriorityFeePerGas. Also, coming out of experience in with Shandong testnet, we should decide what to do with transactions that don't include any gasPrice or maxFeePerGas fields. In Shandong, we opted to just use the latest block's base fee as a reasonable default.

@g11tech
Copy link
Contributor

g11tech commented Oct 15, 2022

@acolytec3 should the eth_call directly do runTx than calling the vm's runCall?

@acolytec3
Copy link
Contributor

acolytec3 commented Oct 16, 2022

@jochem-brouwer any thoughts here? I can test it out but conceptually switching the logic of this handler to use TransactionFactory.fromTxData and then calling runTx would accomplish the same net effect, right? And that would be easier than rebuilding similar logic to handle the different transaction types that get passed in here.

@acolytec3
Copy link
Contributor

Maybe I can take a stab at this in the Shandong branch and see how it goes. It certainly limits the usability of our RPC if eth_call only supports legacy txns.

@jochem-brouwer
Copy link
Member

Yes that should work!

@acolytec3
Copy link
Contributor

Cool. I'll add it to my proofs of concept in shandong and we can pull out in separate PRs against master of everything goes well

@acolytec3
Copy link
Contributor

I believe this is closed by #2411. @shresthagrawal care to take a look at our latest master and verify if it meets your needs?

@holgerd77
Copy link
Member

I believe this is closed by #2411. @shresthagrawal care to take a look at our latest master and verify if it meets your needs?

@acolytec3 the PR you are referencing seems to not fix the eth_call part mentioned in the issue description. Is this also fixed? If so, any reference on this as well?

@acolytec3
Copy link
Contributor

After I reviewed the eth_call documentation more closely, it turns out it doesn't have any concept of transaction types at all. If you look at the description of the transaction object in the input parameters, it exactly matches what we already have implemented. eth_call mimics our evm.runCall more closely than vm.runTx so I think what we currently have in place is correct.

@acolytec3
Copy link
Contributor

Should be addressed at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants