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

Update method_formatters.py #1757

Merged
merged 2 commits into from
Oct 28, 2020

Conversation

tmckenzie51
Copy link
Contributor

@tmckenzie51 tmckenzie51 commented Oct 1, 2020

removes duplicate block_number_formatter method in favor of to_hex_if_integer method.

What was wrong?

The methods block_number_formatter and to_hex_if_integer were duplicate functionality.

Related to Issue #1745

How was it fixed?

block_number_formatter method was replaced with the to_hex_if_integer method.

Todo:

Cute Animal Picture

Red-Tibetan-Mastiff

@njgheorghita
Copy link
Contributor

njgheorghita commented Oct 2, 2020

So, it looks to me like Infura recently updated the version of their IPFS endpoint to 7.0 (running curl -X POST "https://ipfs.infura.io:5001/api/v0/version" from your terminal shows the currently supported version). And py-ipfs-http-client only supports 0.4.22 ≤ … < 0.7.0. There are a couple open issues that appear to reference the exact same problem, I've left a comment on one of them. It seems like web3 is blocked until we resolve this, which is definitely not ideal.

Lmk what you think the best way to proceed is, as I see it we have a couple of options.

  • Wait until a new version of py-ipfs-http-client is released (it's hard to say how soon we can expect this to happen)
  • Temporarily skip all tests that use InfuraIPFSBackend until py-ipfs-http-client is updated
  • Reconfigure the tests to use the LocalIPFSBackend (might require a significant chunk of refactoring)
  • Find a different public gateway we can use. (I've spent a little time trying to connect to a different public gateway, but have had trouble finding something that works with the multiaddr format required by py-ipfs-http-client) - imo this is probably the best temporary solution to get us until py-ipfs-http-client is updated

@tmckenzie51 tmckenzie51 force-pushed the tiffany/remove-duplicate-method branch from 4457072 to b2e8657 Compare October 2, 2020 19:22
@tmckenzie51 tmckenzie51 closed this Oct 2, 2020
@tmckenzie51 tmckenzie51 reopened this Oct 2, 2020
@tmckenzie51
Copy link
Contributor Author

tmckenzie51 commented Oct 2, 2020

@njgheorghita Okay, I see. Thank you for explaining that. Those sound like decent options that we can consider. @pipermerriam @kclowes Of these options, which do you think would be the best for us to proceed with?

@wolovim
Copy link
Member

wolovim commented Oct 7, 2020

@njgheorghita pointed out a fix PR waiting for review, but it's not a very active repo. I'm not opposed to just xfail these tests for now, but is that doable for doctests?

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.

This looks good to me as long as the tests pass after you merge in master!

@tmckenzie51 tmckenzie51 force-pushed the tiffany/remove-duplicate-method branch from b2e8657 to 52cec1c Compare October 15, 2020 20:43
@kclowes
Copy link
Collaborator

kclowes commented Oct 16, 2020

@tmckenzie51 You can try and push an empty commit to see if that readthedocs failure will work itself out: git commit --allow-empty -m "Trigger Build" Let me know if you have questions or want to pair!

tmckenzie51 and others added 2 commits October 16, 2020 14:57
removes duplicate block_number_formatter method in favor of to_hex_if_integer method.
adds release notes
@tmckenzie51 tmckenzie51 force-pushed the tiffany/remove-duplicate-method branch from 52cec1c to 3332989 Compare October 16, 2020 19:57
@kclowes kclowes merged commit 4e0e932 into ethereum:master Oct 28, 2020
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