Skip to content

fix: add chainId to txn params #2450

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

Merged

Conversation

antazoey
Copy link
Contributor

@antazoey antazoey commented Apr 29, 2022

What was wrong?

I noticed even when caching chain_id, the request occurs multiple times. I figured out this happens because I am calling estimate_gas() and passing it a transaction and the RPC method's defined txn schema does not inclue chainId so it makes the request againt to add the chainId at some point (this is only deduced).. By adding chainId to the params, I can avoid an extra RPC call to get the chain ID`

How was it fixed?

Add 'chainId': 'uint', in TRANSACTION_PARAMS_ABIS.

Todo:

Cute Animal Picture

baby_bats

Put a link to a cute animal picture inside the parenthesis-->

@kclowes
Copy link
Collaborator

kclowes commented Apr 29, 2022

Nice catch! I don't think there is a way to indicate that it's optional, but I don't think we do any checks on it. Lmk if you want help debugging the tests. It looks like they're failing because a hex-to-int conversion isn't happening for the error message in the validation middleware.

@antazoey antazoey force-pushed the fix/jules-add-chain-id-to-txn-params branch from c7e537c to e55aa98 Compare May 2, 2022 12:47
@antazoey
Copy link
Contributor Author

antazoey commented May 2, 2022

It looks like they're failing because a hex-to-int conversion isn't happening for the error message in the validation middleware.

I believe I got it!

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@kclowes kclowes merged commit 5b6dea2 into ethereum:master May 2, 2022
kclowes added a commit to kclowes/web3.py that referenced this pull request May 2, 2022
* fix: add chainId to txn params

* fix: error message on chain id hex to int

* Add newsfragment

Co-authored-by: kclowes <[email protected]>
kclowes added a commit that referenced this pull request May 2, 2022
* fix: add chainId to txn params

* fix: error message on chain id hex to int

* Add newsfragment

Co-authored-by: kclowes <[email protected]>

Co-authored-by: Juliya Smith <[email protected]>
@antazoey antazoey deleted the fix/jules-add-chain-id-to-txn-params branch May 11, 2022 18:45
pacrob pushed a commit that referenced this pull request May 23, 2022
* fix: add chainId to txn params

* fix: error message on chain id hex to int

* Add newsfragment

Co-authored-by: kclowes <[email protected]>
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