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

Add global option to specify the multibase encoding. #5464

Closed
wants to merge 13 commits into from

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Sep 14, 2018

This p.r. is being redone (again) please see #5777 and #5789

This is a redo of #5289. It depends on ipfs/go-cidutil#8 and ipfs/go-mfs#5

It adds a --cid-base option for specifying the multibase. It also provides a --upgrade-cidv0--output-cidv1 option to upgrade CidV0 to CidV1 for display. Specifying --cid-base enables --upgrade-cidv0--output-cidv1 by default.

Unlike the last attempt, this does most of the conversion client side. The exception to the rule is that as Paths are meant to be strings the server creates new paths with the correct base when a CID is part of the path.

When a use provides a hash on the command line, this p.r. attempts to preserve that string when returning information on the same hash unless --cid-base is used. For example in ipfs pin.

The multibase of a CID, is now preserved in ipfs resolve. Other commands will need to be reviewed on a case by case basis.

Not every command honers --cid-base yet. The implementation details may still change and a large part of the helper code will likely get moved to go-cidutil.

Closes #5233. Closes #5234. Closes #5349.

@kevina kevina requested a review from Kubuxu as a code owner September 14, 2018 03:14
@ghost ghost assigned kevina Sep 14, 2018
@ghost ghost added the status/in-progress In progress label Sep 14, 2018
@kevina kevina force-pushed the kevina/multibase2 branch 2 times, most recently from 675738c to de63fbc Compare September 14, 2018 03:50
@kevina kevina added the topic/cidv1b32 Topic cidv1b32 label Sep 14, 2018
@kevina
Copy link
Contributor Author

kevina commented Sep 17, 2018

@Stebalien this is still a WIP but I have something I want to double check on: Is it really okay to set a global variable when translating the Cid client side. It does make things easier but I want be 100% sure the client code (in core/commands) won't be used as a library where this can cause a problem. The alternative is to thread the state though to the point where the CID is translated.

Also note I have a whole bunch of changes not pushed yet and I changed a lot of the implementation details. The general idea is still the same though.

@kevina kevina force-pushed the kevina/multibase2 branch 5 times, most recently from f7d408d to 6f145f2 Compare September 19, 2018 06:11
@kevina
Copy link
Contributor Author

kevina commented Sep 20, 2018

This depends on ipfs/go-cidutil#8 and ipfs/go-mfs#5

This should be ready for initial review. I added support for as many commands as I could:

ipfs add -- client side
ipfs block -- skipping for now, will soon need to use raw multihashs
ipfs daemon -- ??? [1]
ipfs dag -- low level, skipping for now
ipfs dns -- ??? [2]
ipfs file ls -- ???
ipfs files ls -- client side
ipfs files stat -- client side
ipfs filestore dups -- server side due to the use of RefWrapper
ipfs filestore ls -- client side
ipfs filestore verify -- client side
ipfs name resolve -- ??? [2]
ipfs object -- low level, skipping for now
ipfs pin add -- client side [m!]
ipfs pin ls -- client side
ipfs pin rm -- client side [m!]
ipfs pin update -- TODO
ipfs pin verify -- client side [m!]
ipfs ref -- server side due to the use of RefWrapper [m]
ipfs refs local -- server side due to the use of RefWrapper
ipfs resolve -- server side [p]
ipfs tar add -- client side
ipfs urlstore add -- FIXME: Uses BlockStat

[m] Any cids identical to the ones passed in on the command line will
be returned using the same version and encoding.  Othercids will use
the default settings.  Setting --cid-base will override this behavior
and convert all cids.

[m!] These commands should likely have the same behavior as in [m],
but the arguments of the command are not always available in the
Marshalers, thus making it difficult to due client cide without
additional hacks.

[p] The cid passed in on the command line is used to influence the
encoding of any other cids unless --cid-base is used.

[1] should --cid-flags set defaults if launched with the daemon?
Probably not, better to use config settings.

[2] I am not 100% sure but I don't think CID's are ever explicitly
constructed, instead I think it is just returning the string found in
the lookup

@kevina kevina changed the title WIP: Add global option to specify the multibase encoding. Add global option to specify the multibase encoding. Sep 20, 2018
@kevina kevina mentioned this pull request Sep 20, 2018
2 tasks
@alanshaw
Copy link
Member

@kevina could you explain what the use case is for --upgrade-cidv0?

@alanshaw
Copy link
Member

@kevina does this PR also allow ?cid-base to be passed to the equivalent HTTP API routes?

@kevina kevina force-pushed the kevina/multibase2 branch 2 times, most recently from caea23f to 8c4460c Compare September 20, 2018 17:19
@kevina
Copy link
Contributor Author

kevina commented Sep 20, 2018

@kevina could you explain what the use case is for --upgrade-cidv0?

I am not sure --upgrade-cidv0 is the best name (I might change it to --display-cidv1 or something similar).

The idea is that when a user uses --cid-base=base32 they expect the hashes to be in base32. This is currently only possible with cidv1 so the CID is upgraded from version 0 to 1. Once the blockstore (and hopefully bitswap see #5378) are converted this won't be a problem in most cases, but the binary representation is different, so it should be an option so the user can turn the conversion off if required.

@kevina
Copy link
Contributor Author

kevina commented Sep 20, 2018

@kevina does this PR also allow ?cid-base to be passed to the equivalent HTTP API routes?

It allows it, but the JSON output of the request will remain unchanged except in the cases I marked as server side in the list mentioned in a comment above.

@kevina kevina mentioned this pull request Sep 21, 2018
8 tasks
@Stebalien Stebalien requested a review from djdv September 25, 2018 05:29
@Stebalien
Copy link
Member

Could you add some comments (especially godoc comments)?

@kevina
Copy link
Contributor Author

kevina commented Sep 25, 2018

Could you add some comments (especially godoc comments)?

Yeah that would be a good idea. :) I work on that tomorrow. I am also unhappy with the choice of --upgrade-cidv0 as we might need that option to actually upgrade Cid's for example when pinning. I am thinking --output-cidv1 or something similar as that is more direct and to the point.

@kevina kevina force-pushed the kevina/multibase2 branch 2 times, most recently from c6690dd to 9a795e4 Compare September 27, 2018 04:57
Also add tests for both `files ls -l` and `files stat`.

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@kevina
Copy link
Contributor Author

kevina commented Sep 27, 2018

@Stebalien rebased and documentation added.

Copy link
Contributor

@djdv djdv left a comment

Choose a reason for hiding this comment

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

Mostly just pedantic text changes.
I'm curious if 1 case can be simplified, but I may be understanding things wrong.
I'll likely put together a patch that can be cherry-picked if desired for documentation changes.
There are some that I have not highlighted, mostly just comma additions.

As for the code itself, it seems sensible to me. No strong opinions on anything at the moment.

core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/dns.go Outdated Show resolved Hide resolved
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@kevina
Copy link
Contributor Author

kevina commented Nov 5, 2018

This needs to be redone, but is also blocked now on the switch to the new commands library of the affected commands.

@kevina kevina added the status/blocked Unable to be worked further until needs are met label Nov 5, 2018
@Stebalien
Copy link
Member

@kevina why is the switch to the new cmds lib blocking this? #5611 is a complex PR and I really don't want to have to rebase it unless we have to.

@kevina
Copy link
Contributor Author

kevina commented Nov 5, 2018

@Stebalien it's not blocking it's conflicting, so it just makes more work for me.

@kevina kevina removed the status/blocked Unable to be worked further until needs are met label Nov 7, 2018
@kevina
Copy link
Contributor Author

kevina commented Nov 14, 2018

Redoing this P.R., see #5777.

With the massive switch to the new command line API it is easier to just redo this p.r. then try to rebase it. The interface for reading the --cid-base global option can now also be simplified thanks to the fact that all commands are now using the new API.

@kevina kevina closed this Nov 14, 2018
@ghost ghost removed the status/in-progress In progress label Nov 14, 2018
@alanshaw
Copy link
Member

@kevina should I be expecting to see base32 CIDs in the output here:

https://ipfs.io/api/v0/resolve?arg=/ipfs/QmTVyRqL8rdJzpCjZSjjw9bJ1M3ioWuyJhCLpYfgRMmzD4&cid-base=base32

@kevina
Copy link
Contributor Author

kevina commented Nov 22, 2018

@alanshaw

@kevina should I be expecting to see base32 CIDs in the output here:

https://ipfs.io/api/v0/resolve?arg=/ipfs/QmTVyRqL8rdJzpCjZSjjw9bJ1M3ioWuyJhCLpYfgRMmzD4&cid-base=base32

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/cidv1b32 Topic cidv1b32
Projects
None yet
4 participants