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

[PR] v0.0.1 #3

Merged
merged 14 commits into from
Dec 1, 2022
Merged

[PR] v0.0.1 #3

merged 14 commits into from
Dec 1, 2022

Conversation

LuchoTurtle
Copy link
Member

addresses #1

This PR creates a basic version of that creates a cid with raw multicodec and v1 versioning, akin to what's being made with dwyl/cid.

I've noticed that, unlike Elixir and other languages, there isn't a canonical repo that allows users to encode their hash strings with whatever codec they want. . Should be a fun task for another day.

This PR has basic testing and mocks dwyl/dart_multihash library, which is waiting to be published. So multihash needs to be published first so I can make use of it before publishing this one.

It's still in progress, as the README still needs to be written (I don't think there's much need to delve into the topics that are written in dwyl/cid, so as to not repeat information. I'll reference it on a "Further reading" basis.

I also want to add property based tests from IPFS, but I still need to figure out how to.

@LuchoTurtle LuchoTurtle added documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person priority-1 Highest priority issue. This is costing us money every minute that passes. labels Nov 26, 2022
@LuchoTurtle LuchoTurtle self-assigned this Nov 26, 2022
@LuchoTurtle
Copy link
Member Author

base32 encoding wasn't working at first but mainly because of lack of information.

I am using the base32 package, which allows me to encode it with different standards. Firstly I tried to get it working the same way dwyl/cid had it, with the "b" suffix (basically referencing the standard case-insensitive, no-padding lowercase base32 encoding.

However, I was using the IPFS CID inspector to check if the output hash was correctly formatted and it wasn't. Turns out that the z-base-32 encoding used in the package is confusing, since it uses illegal characters that are not supported for the IPFS convention (only numbers 2-7 are allowed).

Therefore, to make it IPFS-compatible, I went with the other default base32 multicodec available, which was "B" - uppercase base32 with no padding. The package had this option but with padding, so I had to remove that to get it working.

Just wanted to share why I'm not using the default, base32 lowercase no-padding approach that was used in dwyl/cid.

@nelsonic
Copy link
Member

Makes sense. Thanks for the update/clarity. 👌

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Nov 28, 2022

Added IPFS tests that create a random file and write random strings and check the hash created with ipfs add file.txt -n --cid-version1.

I had a bit of trouble using processes in Dart, I couldn't properly fetch the stdout so I had to use a wrapper that would allow me to fetch the yielded hash.

Going to hand it over for review because the generated cids so far have all complied, inclusively using the CID inspector. The IPFS tests also pass.

@LuchoTurtle LuchoTurtle requested a review from nelsonic November 28, 2022 13:08
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Nov 28, 2022
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Nov 28, 2022
@LuchoTurtle LuchoTurtle marked this pull request as draft November 28, 2022 13:10
@LuchoTurtle LuchoTurtle assigned LuchoTurtle and unassigned nelsonic Nov 28, 2022
@LuchoTurtle LuchoTurtle added in-progress An issue or pull request that is being worked on by the assigned person BLOCKED :fire: Core team's HIGHEST priority, blocking critical work and removed awaiting-review An issue or pull request that needs to be reviewed labels Nov 28, 2022
@LuchoTurtle
Copy link
Member Author

Marking this as draft and blocked because multihash's package PR needs to be reviewed so I can publish it and use it on this one, instead of mocking. dwyl/dart_multihash#2

@nelsonic
Copy link
Member

dwyl/dart_multihash#2 merged. :shipit:
Please proceed. 🙏

@nelsonic
Copy link
Member

@LuchoTurtle assign when you feel it's reviewable. (Code is already looking good!)

@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person BLOCKED :fire: Core team's HIGHEST priority, blocking critical work labels Nov 30, 2022
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Nov 30, 2022
@LuchoTurtle
Copy link
Member Author

Just made the needed changes to get this into a reviewable state.
The dart_multihash project is used as a dependency now.
I converted the package structure and files for a typical Dart package, following the conventions for publishing. A--dry-run does not yield any warnings.
Also added CI that should work once it is merged.

I will update the README with badges once this PR is merged.

@LuchoTurtle LuchoTurtle marked this pull request as ready for review November 30, 2022 19:47
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@LuchoTurtle implementation looks good. 🚀

@nelsonic nelsonic merged commit e7c9650 into main Dec 1, 2022
@nelsonic nelsonic deleted the v0.0.1 branch December 1, 2022 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality priority-1 Highest priority issue. This is costing us money every minute that passes.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants