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

WIP: Add global option to specify the multibase encoding. #5289

Closed
wants to merge 7 commits into from

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Jul 27, 2018

This adds a global option to specify the multibase encoding. It is global as any command that outputs CID's should eventually be taught to respect this flag.

Several questions:

(1) What should it be called. Just --base is to generic and will conflict with existing flags. Right now I am using --mbase but perhaps --cid-base is more obvious to the end user. Another option is --encoding but that is also two generic.

(2) Should this flag imply that new objects are added using CIDv1? That is should it imply --cid-version=1 for command that support it? I am on the fence for this one.

(3) This is related to #5291. If we implement a flag to convert Cidv0 to CIDv1 for display, should this flag imply that. I would vote no.

Depends on ipfs/go-cid#60.

Closes #5233. Closes #5234. Towards https://github.com/ipfs/ipfs/issues/337.

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
Use it in 'ipfs add' and 'ipfs ls'.

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
Use it for "ipfs ls" and "ipfs resolve".  Resolve also reconized the
--cid-base option.

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 kevina added the status/blocked Unable to be worked further until needs are met label Aug 13, 2018
@kevina
Copy link
Contributor Author

kevina commented Aug 13, 2018

This is blocked until #5349 is resolved.

@kevina kevina removed the status/in-progress In progress label Aug 13, 2018
@Stebalien
Copy link
Member

  1. I'd go with --cid-base.
  2. No? That's a hard one. The problem is that inserts CIDv1s throughout the tree and changes a bunch of hashes. Given that we'll be modifying the blockstore/bitswap to not really care, I'd stick with CIDv0.
  3. I'd actually say yes. That is, if we say "give me base32", I'd expect IPFS to spit out base32 names. Again, this assumes that we get all the blockstore stuff in order.

@alanshaw
Copy link
Member

alanshaw commented Sep 4, 2018

I'm in agreement with @Stebalien on these:

(1) What should it be called.

--cid-base is fine by me

(2) Should this flag imply that new objects are added using CIDv1?

Agree no, should just be for output

(3) This is related to #5291. If we implement a flag to convert Cidv0 to CIDv1 for display, should this flag imply that. I would vote no.

If I ask for the CIDs to be output using --cid-base=base32 then I'd expect any CIDv0s to be auto converted to CIDv1 just before they are printed to allow it.

The blockstore should be able to give me back the content anyway if I request something added with CIDv0 with a CIDv1 (as I see has happened in #5285) so it should be fine!?

I can see how it would be unexpected to ipfs add file.txt --cid-base=base32 and get back base58 encoded hashes, and I can also see how it would be frustrating to then have to add another option (--cid-version=1) just to be able to get back base32 encoded hashes.

@kevina
Copy link
Contributor Author

kevina commented Sep 4, 2018

@alanshaw thanks for your feedback.

I am going with your and @Stebalien suggestion for the ipfs add and other high-level commands such as ipfs ls and maybe ips refs. Lower level data structure command such as ipfs dag and ipfs object will not get CidV0 auto upgraded as that can cause problems becuase CidV0 and CidV1 have different binary representations.

@alanshaw
Copy link
Member

Lower level data structure command such as ipfs dag and ipfs object will not get CidV0 auto upgraded as that can cause problems becuase CidV0 and CidV1 have different binary representations

Ah, ok, what problems are we talking about here?

@kevina
Copy link
Contributor Author

kevina commented Sep 14, 2018

Closing in favor of #5464.

@kevina kevina closed this Sep 14, 2018
@kevina
Copy link
Contributor Author

kevina commented Sep 20, 2018

@alanshaw

Ah, ok, what problems are we talking about here?

Any CidV1 will have the same binary representation regardless of what multibase is used. The same can not be said for CidV0 vs CIDv1. A object with CidV0 links will have a different hash than an object with the same links but in CidV1. cid object and cid dag are meant for manipulate DAG objects so the version of CIDs should be preserved in those commands otherwise a user could inadvertently change the hash of an object.

@hacdias hacdias deleted the kevina/multibase branch May 9, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked Unable to be worked further until needs are met topic/cidv1b32 Topic cidv1b32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants