Skip to content
This repository was archived by the owner on Jun 19, 2023. It is now read-only.

feat!: use path.Path instead of string in routing API #104

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Feb 8, 2023

Based on #103. It changes the string key to a path.Path - after all, we always need routing keys for a certain path. It makes the interface more clear.


[..] the interface we added for the routing API might not be the most intuitive. I think replacing the string key by a path.Path would be more intuitive.

type RoutingAPI interface {
	// Get retrieves the best value for a given key
-	Get(context.Context, string) ([]byte, error)
+	Get(context.Context, path.Path) ([]byte, error)

	// Put sets a value for a given key
-	Put(ctx context.Context, key string, value []byte) error
+	Put(ctx context.Context, path.Path, []byte) error
}

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM:

kubo$ git grep "routing get"
core/commands/name/name.go:  $ ipfs routing get "/ipns/$PEERID" > ipns_record
test/cli/basic_commands_test.go:                "ipfs routing get":              true,
test/sharness/t0100-name.sh:        ipfs routing get "/ipns/$PEERID" > ipns_record
test/sharness/t0170-routing-dht.sh:  # ipfs routing get <key>
test/sharness/t0170-routing-dht.sh:    ipfsi 1 routing get "/ipns/$PEERID_2" >get_result
test/sharness/t0170-routing-dht.sh:    test_must_fail ipfsi 0 routing get "foo" &&
test/sharness/t0170-routing-dht.sh:    test_must_fail ipfsi 0 routing get "/pk/foo"

@Jorropo Jorropo merged commit 6518db1 into test/routing Feb 8, 2023
@Jorropo Jorropo deleted the improve-routing-interface branch February 8, 2023 11:38
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I worry this will do more harm than good.

This is a low level API that we recently hijacked for ipns-record work, but it is not limited to /ipns/ and /pk paths, nor any path at all. In fact, routing key can be arbitrary bytes.

We have high level APIs for working with paths (name resolve, name publish).

ipfs dht get and put are used by people for publishing records in their own namespaces, and since we've allowed arbitrary prefix since the beginning, we will be breaking someone's setup. Since we renamed ipfs dht to ipfs routing, tese are useful diagnostic commands for probing at routing system behaviors beyond DHT now.

If we don't have to break users, I'd leave it as-is.

@hacdias
Copy link
Member Author

hacdias commented Feb 8, 2023

@Jorropo can we revert this on #103 and only keep the tests then? Maybe in the future we should think on how to turn this into a higher level API, such that it is not awkward to have on the Core API.

@Jorropo
Copy link
Contributor

Jorropo commented Feb 8, 2023

@lidel I don't really see why that an issue, path.Path is fairly generic, it's just wrapping string and adds some helper functions: https://pkg.go.dev/github.com/ipfs/go-path#Path

It's quite nice at hinting what we expect here.

@hacdias
Copy link
Member Author

hacdias commented Feb 8, 2023

@Jorropo yes, keep it. Had a sync discussion with @lidel.

ipfs routing get/put already fails on anything other than IPNS/PK paths, so we don't break anything and it's more clear.

@lidel
Copy link
Member

lidel commented Feb 8, 2023

Ack, if ipfs dht put|get never accepted arbitrary bytes, then fine to change this.

But won't ParsePath error on /pk now?
(https://github.com/ipfs/go-path/blob/34ee0e38d040b22a4baf20740af45e43c8a5d50e/path.go#L126)

We are mixing content path abstraction (where /pk is not valid) with routing system prefix abstraction (where /pk is valid).

@Jorropo
Copy link
Contributor

Jorropo commented Feb 8, 2023

@lidel you are right, I was saying that you can just path.Path(someStr).

Actually I want to revert this, the dht code uses string for the keys: https://pkg.go.dev/github.com/libp2p/go-libp2p-kad-dht#IpfsDHT.PutValue

Ideally we would need a new routingPath type or something like this, anyway that more work than is worth.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants