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

Jp/hotfix optional gas price in rpc transaction type #1699

Conversation

jp-imx
Copy link
Contributor

@jp-imx jp-imx commented Jul 4, 2023

Description

I've made GasPrice optional in the transaction struct in jsonrpc/types.go. The previous implementation made it impossible to call eth_getTransactionByHash for EIP-1559 transactions or eth_getBlockByNumber with the transaction details flag. on, if the block contained EIP-1559 transactions.

Changes include

Marking this as a HOTFIX as Immutable needs this issue solved before we can release our public testnet.

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

N/A

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

I had a local polygon-edge chain with 4 nodes. Three of them running v1.0.0. One of them running a version with this fix. A call to eth_getBlockByNumber with the transaction details flag on would fail on v1.0.0 with nil dereference error, but it would work on the node with the fix.

Documentation update

N/A

Additional comments

N/A

@goran-ethernal goran-ethernal requested a review from a team July 7, 2023 07:06
@jp-imx
Copy link
Contributor Author

jp-imx commented Jul 10, 2023

@goran-ethernal thanks for approving. I understand there's some flakyness in the CI, so was wondering if you think the failed PolyBFT E2E task action is unrelated to my changes. Note, I don't have access to retrying the action or merging this PR.

@goran-ethernal
Copy link
Collaborator

@jp-imx Nope, your changes didn't affect that test. We have fixed that test in: #1702, so we are good to merge this one. Just need one more approval.

@jp-imx
Copy link
Contributor Author

jp-imx commented Jul 10, 2023

@goran-ethernal do you need me to merge that PR or maybe develop into my branch? Or can we merge as is? I can't retry the build though 🤔

jsonrpc/types.go Outdated Show resolved Hide resolved
@goran-ethernal
Copy link
Collaborator

@jp-imx No need, this test has nothing to do with your changes, so we can merge this PR. I left a small comment. After that, we can merge it.

@jp-imx jp-imx force-pushed the jp/HOTFIX-optional-gas-price-in-rpc-transaction-type branch from c840ccd to 567acfb Compare July 11, 2023 06:53
@goran-ethernal goran-ethernal merged commit 8565d45 into 0xPolygon:develop Jul 11, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants