Skip to content

Add chain_id setter to avoid eth_chainId requests #2207

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
merged 1 commit into from
Mar 2, 2022

Conversation

medariox
Copy link
Contributor

@medariox medariox commented Nov 16, 2021

What was wrong?

Every time the chain_id property is called, an eth_chainId request is triggered.

How was it fixed?

By manually specifying the chain_id, the request can be avoided.

web3 = Web3(HTTPProvider(...))
web3.eth.chain_id = 1 # mainnet

As a further improvement, it may be beneficial to cache the result of the first eth_chainId request.
The current change is intentionally conservative: there is no behavioral change if the setter is not used.

Todo:

Cute Animal Picture

cute otter

@@ -469,7 +470,14 @@ def blockNumber(self) -> BlockNumber:

@property
def chain_id(self) -> int:
return self._chain_id()
if self._default_chain_id is None:
return self._chain_id()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to set _default_chain_id here before returning the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it totally would! (That's what I meant with: "As a further improvement, it may be beneficial to cache the result of the first eth_chainId request.")

But that would also change the current behavior and I don't know if that's something that is wanted.

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.

Thanks @medariox! I assume this fixes the chain_id issue where it's requesting the chainId even though it shouldn't? Do you mind adding a test for that?

@medariox
Copy link
Contributor Author

Thanks @medariox! I assume this fixes the chain_id issue where it's requesting the chainId even though it shouldn't? Do you mind adding a test for that?

Sorry for the late reply.
This will avoid requesting the chainId every time the web3.eth.chain_id property is called (if previously set with the setter).
The question is: do we also want to avoid requesting the chainId after we have a chainId from the request as @unparalleled-js suggested (without having to set it manually, basically caching it after the web3.eth.chain_id property si called the first time)?

I'll try to add some meaningful tests next week.

@kclowes
Copy link
Collaborator

kclowes commented Nov 22, 2021

do we also want to avoid requesting the chainId after we have a chainId from the request as @unparalleled-js suggested (without having to set it manually, basically caching it after the web3.eth.chain_id property si called the first time)?

If I understand this code correctly, I think I lean towards no, because as it stands right now, when we call w3.eth.chain_id, we are asking the chain "what is your chain_id" vs. asking "what is the chain_id I've set", if that makes sense. And I think we want to maintain the functionality that asks "what is the chain_id for the chain I'm connected to" without having to manually set the default_chain_id to None every time we need an answer to that question. But if either you or @unparalleled-js have a strong opinion otherwise, I'd love to hear it.

And btw if you're seeing Timeout failures on integration tests, feel free to ignore. Those are just flaky.

@medariox
Copy link
Contributor Author

do we also want to avoid requesting the chainId after we have a chainId from the request as @unparalleled-js suggested (without having to set it manually, basically caching it after the web3.eth.chain_id property si called the first time)?

If I understand this code correctly, I think I lean towards no, because as it stands right now, when we call w3.eth.chain_id, we are asking the chain "what is your chain_id" vs. asking "what is the chain_id I've set", if that makes sense. And I think we want to maintain the functionality that asks "what is the chain_id for the chain I'm connected to" without having to manually set the default_chain_id to None every time we need an answer to that question. But if either you or @unparalleled-js have a strong opinion otherwise, I'd love to hear it.

And btw if you're seeing Timeout failures on integration tests, feel free to ignore. Those are just flaky.

I tend to agree with you. I was thinking about the possibility because as long as the endpoint (chain provider) is not changed, there should be no way to get a different result. That's my current understanding of it, but I may be wrong and I don't want to risk breaking anything. 😅

@BoboTiG
Copy link
Contributor

BoboTiG commented Feb 13, 2022

The patch seems quite interesting, what is the status? :)

@medariox
Copy link
Contributor Author

@BoboTiG
I'm not sure what is holding it back. The PR is ready from my side.

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.

Thanks @medariox! This looks great! Sorry it took me so long to circle back. I moved the newsfragment to be a feature instead of misc and pulled in the most recent master. I'll also cherry-pick this onto our v5 branch.

@kclowes kclowes mentioned this pull request Mar 2, 2022
1 task
@kclowes kclowes merged commit 81427e6 into ethereum:master Mar 2, 2022
@kclowes kclowes mentioned this pull request Mar 3, 2022
1 task
@medariox medariox deleted the patch-1 branch March 4, 2022 11:28
@kclowes
Copy link
Collaborator

kclowes commented Mar 14, 2022

We were talking about this internally - does anyone have a use case where you wouldn't use the chain_id returned by w3.eth.chain_id? We were thinking of either:

  1. making a w3.eth.chain_id call on provider instantiation and setting the default_chain_id that way, or
  2. validating that the default_chain_id that gets set is the same as the one that gets returned by w3.eth.chain_id

Does anyone have any opinions?

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.

4 participants