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: improved error message on broken CIDv0 #116

Closed
wants to merge 1 commit into from

Conversation

lidel
Copy link
Member

@lidel lidel commented Dec 1, 2020

This aims to close DX/UX issue described in ipfs/kubo#7792

Before

invalid path "/ipfs/qmbwqxbekc3p8tqskc98xmwnzrzdtrlmimpl8wbutgsmnr/": invalid CID: selected encoding not supported

After

invalid path "/ipfs/qmbwqxbekc3p8tqskc98xmwnzrzdtrlmimpl8wbutgsmnr/": invalid CID: selected encoding not supported: This looks like a CIDv0 that has been lowercased. Convert the CIDv0 to CIDv1 in case-insensitive base32 using 'ipfs cid base32 <Qm..>' and try again.

@lidel lidel force-pushed the fix/broken-cidv0-ux branch 2 times, most recently from cb0a609 to 20232f8 Compare December 1, 2020 09:38
@@ -254,6 +254,9 @@ func Decode(v string) (Cid, error) {

_, data, err := mbase.Decode(v)
if err != nil {
if len(v) == 46 && v[:2] == "qm" { // https://github.com/ipfs/go-ipfs/issues/7792
return Undef, fmt.Errorf("%v: This looks like a CIDv0 that has been lowercased. Convert the CIDv0 to CIDv1 in case-insensitive base32 using 'ipfs cid base32 <Qm..>' and try again.", err)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like an awfully low level to be handing out command specific advice, since this library has wide use, well beyond go-ipfs. This could arise out of use within Filecoin, or a CAR manipulation library, use-cases where ipfs isn't even a thing.

Is there a place above the stack from here, closer to the user, where this check could be inserted instead?

In lieu of that, maybe softening it to seem a little less prescriptive?

"%v: This may be a CIDv0 that has been lowercased and cannot be decoded. Base32 CIDv1 is recommended where case-insensitivity is required (e.g. with 'ipfs cid base32 <Qm..>')."

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Rod; the error message here should be reasonably generic, and IPFS could always do the extra check if go-cid returns an error.

Copy link

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

I can't weigh in on how generic/specific we need to be, but either the original proposed text or the message in #116 (comment) read well to me.

@Stebalien
Copy link
Member

This is probably best fixed in go-path.

rvagg added a commit to ipfs/go-path that referenced this pull request Dec 2, 2020
rvagg added a commit to ipfs/go-path that referenced this pull request Dec 2, 2020
@rvagg
Copy link
Member

rvagg commented Dec 2, 2020

go-path is a much better idea, since it's already ipfs-specific. I've copied this fix, almost exactly as is (including original msg), to ipfs/go-path#33, would that be acceptable to all?

rvagg added a commit to ipfs/go-path that referenced this pull request Dec 3, 2020
@lidel
Copy link
Member Author

lidel commented Dec 7, 2020

I believe ipfs/go-path#33 is fine, this can be closed.
Thank you all for landing that one!

@lidel lidel closed this Dec 7, 2020
@lidel lidel deleted the fix/broken-cidv0-ux branch December 7, 2020 17:53
Jorropo pushed a commit to ipfs/go-libipfs-rapide that referenced this pull request Mar 23, 2023
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.

5 participants