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

fix: base32 for ipfs:// urls and cidv1 everywhere #5

Merged
merged 1 commit into from
May 18, 2021

Conversation

olizilla
Copy link
Contributor

@olizilla olizilla commented May 17, 2021

  • make POST return a base32 encoded cidv1. (it was returning bsae36 which is valid, but surprising)
  • default to using cidv1 everywhere.
  • update to use the new ipfs-core module.

base36 is the right thing to use to respect the max authority length for ipns:// scheme urls.
The assumption is folks should use base32 for CIDs in ipfs:// scheme urls, and it's the default output for cidv1.

The new ipfs-core modules has the same api as the ipfs module but doesn't include the cli and http-server code with it.

At time of writing the tests pass apart from the last one, which hangs. I think it's due to an unrelated bug ipfs/js-ipfs#3692

License: MIT
Signed-off-by: Oli Evans oli@tableflip.io

- make POST return a base32 encoded cidv1.
- default to using cidv1 everywhere.
- update to use the new ipfs-core module.

base36 is the right thing to use to respect the max authority length for ipns:// scheme urls.
The assumption is folks should use base32 for CIDs in ipfs:// scheme urls, and it's the default output for cidv1.

The new ipfs-core modules has the same api as the `ipfs` module but doesn't include the cli and http-server code with it.

At time of writing the tests pass apart from the last one, which hangs. I think it's due to an unrelated bug ipfs/js-ipfs#3692

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@olizilla
Copy link
Contributor Author

I'll chase down ipfs/js-ipfs#3692 when i get time and see if I can get that unrelated test hang fixed.

Copy link
Owner

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @olizilla !

Just had some questions about the migration to ipfs-core and some clarification for the base32 stuff. 😁

I'll look into the test more just before merging.

index.js Show resolved Hide resolved
test.js Show resolved Hide resolved
Copy link
Owner

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

LGTM!

@RangerMauve
Copy link
Owner

Thanks for the clarifications, @lidel!

@RangerMauve RangerMauve merged commit caaad5b into RangerMauve:default May 18, 2021
@RangerMauve
Copy link
Owner

Published in 2.0.0 since this changes how URLs work. Thanks again for the PR and thanks @lidel for the clarification.

@olizilla
Copy link
Contributor Author

🎉 what a nice thread to wake up to! Thanks @lidel and lovely work on this module @RangerMauve !

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.

3 participants