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

Write humanize_ipfs_uri #162

Merged
merged 2 commits into from Jun 13, 2019

Conversation

Projects
None yet
3 participants
@njgheorghita
Copy link
Contributor

commented Jun 10, 2019

What was wrong?

This is a util that could be helpful to have available when working with ipfs uris.

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the njgheorghita:humanize-ipfs-uri branch 7 times, most recently from 07e0352 to 9da28d0 Jun 10, 2019

@njgheorghita njgheorghita requested a review from pipermerriam Jun 10, 2019

@pipermerriam
Copy link
Member

left a comment

Some forwards compatibility concerns (they are minor). IPFS technically supports arbitrary hash types (go look up self-describing-hashes). The Qm IIRC denotes the SHA256 hash. You might go verify there aren't any others that are actively supported.

@@ -1286,6 +1286,19 @@ hexidecimal digits.
'0001..1e1f'
``humanize_ipfs_uri(string)`` -> string
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jun 11, 2019

Member

I think this works but maybe go ahead and extend the ^^^^^ to match line length of the header text.

return "ipfs://{0}..{1}".format(head, tail)


def _is_ipfs_uri(uri: str) -> bool:

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jun 11, 2019

Member

In theory this should take Any and there should be a type check of isinstance(value, str) at the beginning since the is_thing APIs should in theory support passing any arbitrary type or value into them. That also implies that the variable name should be value to make it more generic.

Since this isn't a public API (yet) this isn't a big deal, but I could see that changing at some point in the near-ish-term future.

@njgheorghita

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

You might go verify there aren't any others that are actively supported.

At the moment, only CIDv0 (hashes beginning with Qm) is supported, but that's in the process of changing to CIDv1 - ipfs/ipfs#337. So, this current solution should be fine until that update is made.

@njgheorghita njgheorghita force-pushed the njgheorghita:humanize-ipfs-uri branch from 9da28d0 to ef70a91 Jun 12, 2019

@pipermerriam

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

So, this current solution should be fine until that update is made.

How would you forsee the API changing when that update happens. eth-utils basically cannot have a breaking change because it's such a fundamental component across our tools. Doing a major version bump would almost definitely cause us a lot of pain.

@njgheorghita

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

How would you forsee the API changing when that update happens.

The move to CIDv1 is going to maintain backwards compatibility with CIDv0 ("Qm")

example CIDv1b32 multihash which is the default scheme to be used going forward
BJV2WY5DJMJQXGZJANFZSAYLXMVZW63LFEEQFY3ZP

Though, the target state is support for ALL CIDv1 schemes. Which is a fair amount of schemes to support, but imo even this wouldn't necessarily change the API humanize_ipfs_uri(uri), just the internal logic of is_ipfs_uri verifying that the provided value conforms to one of the supported schemes.

@pipermerriam

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Would that imply that we might have lower granularity is_CIDv0_ipfs_hash functions. If so what do you think about implementing the granular one and holding off on the main one so that we don't accidentally get the API funky before we're ready for the generic one? This sort of reminds me of back when I originally implemented is_address and had to go through a few iterations to get it correct with all the different formats.

@njgheorghita

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@pipermerriam Sounds good to me!

@njgheorghita

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@pipermerriam ping for another 👀. It's not exactly what we settled on, but I thought it might be good to leave is_ipfs_uri, since there are features that all valid ipfs uris will share, regardless of their hashing scheme.

Also, I changed _is_ipfs_uri to is_ipfs_uri, which might belong better in a new ipfs.py module? Along with _is_CIDv0_ipfs_hash. (which I'm no longer certain should remain a private function? thoughts?)

@pipermerriam

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

If the only thing we are exposing as new API is the humanize function then I'm good with this moving forward as-is and deciding about whether we expose the other IPFS utilities at a later date. Make sure they aren't importable from the top level module.

@njgheorghita

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@pipermerriam just to make sure I'm on the same page, by "expose as new API" you mean making importable form the top level module? and not simply that a function is no longer private?

@pipermerriam

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Expose API roughly equal to documenting it and/or making it available for import from a prominent module.

@njgheorghita njgheorghita merged commit 2b13060 into ethereum:master Jun 13, 2019

5 checks passed

ci/circleci: doctest Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py35 Your tests passed on CircleCI!
Details
ci/circleci: py36 Your tests passed on CircleCI!
Details
ci/circleci: pypy3 Your tests passed on CircleCI!
Details

@njgheorghita njgheorghita deleted the njgheorghita:humanize-ipfs-uri branch Jun 13, 2019

@lidel

This comment has been minimized.

Copy link

commented Jun 15, 2019

I am bit late to the party, but while CIDv1 is not the default, those are being used in production.
While not critical, it is important to add support for CIDv1 now, as it will be the new default in IPFS before this year ends.

People can already opt-in when adding files via:

$ ipfs add --cid-version 1 cat.jpg

It is also possible to convert existing CIDv0 to CIDv1 (eg. to use in subdomains):

$ ipfs cid base32 QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR
bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi

CIDv1 in the wild: https://ipfs.io/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi

CIDv1-in-subdomain (in base32, as Origin needs to becase-insensitive): https://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.dweb.link/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.