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

Expose eth_maxPriorityFeePerGas JSON RPC function #251

Merged

Conversation

Stefan-Ethernal
Copy link
Contributor

@Stefan-Ethernal Stefan-Ethernal commented Aug 7, 2023

This PR exposes the eth_maxPriorityFeePerGas endpoint, which is introduced with EIP-1559. It also added a unit test that tests it. However, when such a unit test was introduced, it was realized that with the current implementation, it was impossible to send dynamic fee transactions, because of the following error returned from Geth: both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified. That is why transaction marshaling and unmarshalling logic needed to be changed, so gasPrice is not included for dynamic fee transactions anymore.

Also, rename the eth_feeHistory parameters to be aligned with the specification and also provide the rewardPercentiles parameter (https://docs.alchemy.com/reference/eth-feehistory).

@vercel
Copy link

vercel bot commented Aug 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ethgo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 8, 2023 0:45am

@Stefan-Ethernal Stefan-Ethernal changed the title Expose eth_maxPriorityFeePerGas Expose eth_maxPriorityFeePerGas JSON RPC function Aug 7, 2023
@@ -401,3 +401,40 @@ func TestEthFeeHistory(t *testing.T) {
assert.NoError(t, err)
assert.NotNil(t, fee)
}

func TestEthMaxPriorityFeePerGas(t *testing.T) {
s := testutil.DeployTestServer(t, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use s := testutil.NewTestServer(t) instead? I am trying not to deploy a new server for each unit test as it was done before. Otherwise, it taxes too much the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 2be2739

@ferranbt ferranbt merged commit b501468 into umbracle:main Aug 8, 2023
2 checks passed
@Stefan-Ethernal Stefan-Ethernal deleted the feat/expose-eip-1559-endpoints branch August 8, 2023 15:08
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.

2 participants