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: ipfs dht put/get commands with peerIDs encoded as CIDs #7633

Merged
merged 1 commit into from
Sep 2, 2020

Conversation

aschmahmann
Copy link
Contributor

No description provided.

…d fail early for namespaces other than /pk or /ipns
Comment on lines -680 to -685
case 1:
k, err := b58.Decode(s)
if err != nil {
return "", err
}
return string(k), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was this ever for?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe for find-provider? But that's not an issue anymore.

default:
if len(parts) != 3 ||
parts[0] != "" ||
!(parts[1] == "ipns" || parts[1] == "pk") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem like there's a reason to support anything other than these two namespaces. Should we even support querying for /pk anymore?

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 hard-code these. There's no reason not to just let the DHT handle it (in case we add a new validator).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd rather this yell at us early. The problem is that we're using peer.Decode(parts[2]) which seems unlikely to be valid with any other validator. This is a little more informative than a decoding error.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you. I just don't like having checks at multiple points. It's nice to be able to add a new record type without having to modify this check here.

Copy link
Contributor

@petar petar left a comment

Choose a reason for hiding this comment

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

lgtm

@aschmahmann aschmahmann merged commit b7d00a0 into master Sep 2, 2020
@aschmahmann aschmahmann mentioned this pull request Sep 10, 2020
72 tasks
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
@hacdias hacdias deleted the fix/dht-cmd-cid-ipns branch May 9, 2023 11:00
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.

3 participants