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

commands/block: use CIDv1 with custom mhtype #4563

Merged
merged 3 commits into from
Mar 23, 2018
Merged

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Jan 8, 2018

Closes #4741 ~Kubuxu

@ghost ghost assigned magik6k Jan 8, 2018
@ghost ghost added the status/in-progress In progress label Jan 8, 2018
Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

This looks fine.

Note that this differs from how a multi-hash is specified in the add and files command which use just --hash.

@Stebalien
Copy link
Member

Shouldn't this actually just return an error if the user passes -f v0 --mhtype=other-hash --mhlen=other-len?

@magik6k
Copy link
Member Author

magik6k commented Jan 22, 2018

Done

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Should be correct once you remove the default from the --format flag (you should probably note the behavior in the command's help itself).

var pref cid.Prefix
pref.Version = 1

format, _ := req.Options["format"].(string)
format, userFormat := req.Options["format"].(string)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, userFormat will always be true there is a default value. The only way I know to fix this, is to simply remove the default.

@magik6k magik6k force-pushed the fix/block-mhtype branch 2 times, most recently from 9c43c2c to fd5479a Compare February 1, 2018 23:57
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@@ -209,4 +209,16 @@ test_expect_success "no panic in output" '
test_expect_code 1 grep "panic" stat_out
'

test_expect_success "can set multihash type and length on block put without format" '
HASH=$(echo "foooo" | ipfs block put --mhtype=sha3 --mhlen=16)
Copy link
Member

Choose a reason for hiding this comment

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

16 will be too short of a hash length after cid changes come through, please change it to 20.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
`,
},

Arguments: []cmdkit.Argument{
cmdkit.FileArg("data", true, false, "The data to be stored as an IPFS block.").EnableStdin(),
},
Options: []cmdkit.Option{
cmdkit.StringOption("format", "f", "cid format for blocks to be created with.").WithDefault("v0"),
cmdkit.StringOption("format", "f", "cid format for blocks to be created with.").WithDefault(""),
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't specify a default at all. As it is, passing -f "" will be considered valid even though it makes no sense.

If you don't specify a default, you can tell if the parameter was passed with format, formatSet := req.Options["format"].(string).

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@Kubuxu Kubuxu requested a review from Stebalien March 10, 2018 22:56
@schomatis
Copy link
Contributor

This PR looks correct to me (and it fixes the addressed issue) but let me raise an off-topic question: shouldn't part of this prefix preprocessing logic be in go-cid?

It seems to me there are a couple of hard-coded pieces of information of what constitutes a valid CID prefix that the ipfs block command doesn't need to know because it shouldn't be its responsibility to check for it, e.g., the (now) default CIDv1 specified here (pref.Version = 1) will have to be updated if (or when) a new CID version is released.

The ipfs block command doesn't do anything with the version and hash function except forward them to Prefix, which is the one that computes the hash/CID that ipfs block is really interested in (to create block).

@Kubuxu
Copy link
Member

Kubuxu commented Mar 11, 2018

I think most of the processing here is due to 1. us wanting to provide good error messages for user 2. be lenient with the user input. One check in question (CIDv0 can only be full length SHA256) is currently not implemented but we are in process of changing that (https://github.com/ipfs/go-cid/pull/40/files#diff-beb384810133d725af6b502e9b6e3de8R139)

I am not sure if go-cid would be right place to cover all of those areas but having a helper function somewhere in go-ipfs would not hurt.

@schomatis
Copy link
Contributor

us wanting to provide good error messages for user

That's a good point I hadn't considered, but looking at the errors:

  • unrecognized multihash function
  • unrecognized format
  • cannot generate CIDv0 with non-sha256 hash function

I think all of them can be outsourced without losing expressiveness for the user (the missing option "mhlen" seems to me the only error that truly belongs here).

I am not sure if go-cid would be right place to cover all of those areas but having a helper function somewhere in go-ipfs would not hurt.

That's fine, my main point wasn't where this logic should be placed but that it should be encapsulated somewhere outside this command function.

Anyway, maybe this should be discussed in another issue after this PR is merged, trying to change all this here in this PR now would be to messy, but I wanted to leave this suggestion documented somewhere.

@Stebalien
Copy link
Member

I think the issue here is that we want to catch all of these errors early and provide reasonable defaults (that may change based on the command in question). However, we probably could have something in go-cid that takes in a set of CID "constraints" and spits out either an error or the set of CID parameters.

An alternative solution would be to add flag "sets" to go-ipfs-cmds and create a flag set for CID options somewhere.

@Kubuxu Kubuxu added RFM and removed status/in-progress In progress labels Mar 11, 2018
@Kubuxu Kubuxu added this to the go-ipfs 0.4.15 milestone Mar 13, 2018
@whyrusleeping whyrusleeping merged commit a9dfe41 into master Mar 23, 2018
@whyrusleeping
Copy link
Member

First PR merged of the 0.4.15 cycle goes to @magik6k :) 👏

@whyrusleeping whyrusleeping deleted the fix/block-mhtype branch March 23, 2018 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants