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

feat: new multiformats #45

Merged
merged 9 commits into from
Aug 1, 2022
Merged

feat: new multiformats #45

merged 9 commits into from
Aug 1, 2022

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jul 26, 2022

image

After this PR

  • Add service worker or some other kind of check to update the multicodec table such that we do not have to always deploy this
  • Tests, tests, and more tests.

@hacdias hacdias requested a review from lidel July 26, 2022 13:08
@hacdias hacdias changed the title wip: new multiformats feat: new multiformats Jul 26, 2022
@lidel lidel mentioned this pull request Jul 26, 2022
makes it clear that the value is (digest, in specific multibase)
@lidel lidel marked this pull request as ready for review July 26, 2022 14:16
@lidel
Copy link
Member

lidel commented Jul 26, 2022

Based on @olizilla notes in #40 I've tweaked the label of multibase digest, so it is more clear, especially given the base will depend on CID's one:

2022-07-26_16-14

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@hacdias thank you! ❤️

CircleCI cleanup is tracked in #46, should not block this PR.

I think we should prioritize shipping this, and move "Perhaps web worker or something to update multiformats table periodically" to a separate issue/PR. Quick note: let's have GithubAction (CI) that updates codecs.json daily than client-side code that sends HTTP requests to GitHub.

@lidel lidel self-requested a review July 26, 2022 14:29
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@lidel lidel marked this pull request as draft July 26, 2022 14:39
@hacdias
Copy link
Member Author

hacdias commented Jul 28, 2022

@lidel should be fixed

@hacdias hacdias marked this pull request as ready for review July 28, 2022 12:56
@hacdias hacdias requested a review from lidel July 28, 2022 12:56
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidel lidel merged commit 5e0068d into master Aug 1, 2022
@lidel lidel deleted the feat/new-multiformats branch August 1, 2022 17:34
@lidel
Copy link
Member

lidel commented Aug 1, 2022

@hacdias fyi filled #47 and #48 will potential follow-up tasks (#48 being more important IMO, as it worked before but we had no tests and we broke it 😬 )

@hacdias hacdias mentioned this pull request Sep 1, 2022
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.

Can't decode emoji CIDs Can't decode CAR CIDs show multihash for a cid
2 participants