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

fix: zero base fee #1280

Merged
merged 6 commits into from
Oct 3, 2023
Merged

fix: zero base fee #1280

merged 6 commits into from
Oct 3, 2023

Conversation

holic
Copy link
Contributor

@holic holic commented Oct 3, 2023

I just bumped viem from 1.6.0 to 1.14.0 and I am seeing "intrinsic gas too high" when using writeContract on an anvil chain running with --base-fee 0 and viem chain set to fees: { defaultPriorityFee: 0n }.

Looking at request logs, I can see a new call to eth_maxPriorityFeePerGas before eth_estimateGas that returns 0x3b9aca00, which is used as the value for maxFeePerGas and maxPriorityFeePerGas in the eth_estimateGas call.

Before the version bump, there was no eth_maxPriorityFeePerGas call and the 0n from the chain config's defaultPriorityFee was used as the value of maxFeePerGas and maxPriorityFeePerGas.

I am not sure if I am misunderstanding the new fee calculation but this seems like a regression from the previous behavior that respected the chain config's defaultPriorityFee? I think this behavior changed in #1058 but haven't read it thoroughly.

I've added a test for this here, but can't seem to get tests running locally to verify that it fails + add a fix.


PR-Codex overview

Focus of the PR:

This PR focuses on improving the handling of defaultPriorityFee in the code.

Detailed summary:

  • Added a check for typeof chain?.fees?.defaultPriorityFee to handle undefined values.
  • Added tests for chain override with zero base fee and async zero base fee.
  • Updated package.json to include typesVersions for better type definitions.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2023

⚠️ No Changeset found

Latest commit: a6177e7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 3, 2023

@holic is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

@holic holic force-pushed the holic/zero-base-fee branch from f9e864b to fffe4f0 Compare October 3, 2023 14:18
@holic
Copy link
Contributor Author

holic commented Oct 3, 2023

I think I found it. We had an typeof defaultPriorityFee !== 'undefined' check in https://github.com/wagmi-dev/viem/pull/1006/files#diff-e055abee919d609c1a367c63ac0c8877d34a82f21e687f8901db46bb7eb4a18bR90 but that changed to a falsey check in https://github.com/wagmi-dev/viem/pull/1044/files#diff-cb3ad47420815660f8a1c71a9587607f6ad26342fd4220688031852b1d327c7dR71.

I discovered this because I could change defaultPriorityFee: 0n to defaultPriorityFee: () => 0n and it works again.

I'm still not sure why the prepareTransactionRequest test is failing.

@holic holic changed the title add test for zero base fee fix: zero base fee Oct 3, 2023
@jxom jxom merged commit 309b583 into wevm:main Oct 3, 2023
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