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

feat: add put / get methods #54

Closed
wants to merge 7 commits into from
Closed

Conversation

yusefnapora
Copy link

This adds put and get methods that wrap the client's dht methods.

I noticed that it errors out if I pass in Uint8Arrays to dht.put and dht.get, so this will convert to string and complain if it's not given a valid UTF-8 key. Alternatively, we could just only accept strings, but this way we support the same argument types as dht.put and dht.get, so we can front them both with one interface.

As for which interface, we could either expand the ContentRouting interface to include put and get, or define a new one and just have the DelegatedContentRouter implement both. Defining a new one seems a bit cleaner... maybe ContentStorage?

@vasco-santos you seem like you'd have some thoughts on the interface question.

closes #49

@vasco-santos
Copy link
Member

Hey @yusefnapora thanks for the PR ❤️

I have been thinking about namings given it feels strange to have them part of "Content Routing", when Content Routing is a "ContentRouting is a value provider layer of indirection. It is used to find information about who has what content.".

Looking into go libp2p interface definitions, I think we should probably follow them here https://github.com/libp2p/go-libp2p-core/blob/master/routing/routing.go#L49-L68 . No strong opinion on naming, but for consistency probably ValueStoreInterface. What do you think?

Also for broader context here, when we did a DHT refactor a couple of years ago, we also created a distinction between content routing and put(get ops with content-fetching naming per: https://github.com/libp2p/js-libp2p-kad-dht/tree/master/src/content-fetching . Should we move everything into value-store?

@yusefnapora
Copy link
Author

I like ValueStoreInterface 👍

Do you think that we should have a separate DelegatedValueStore class? Perhaps we could still put it in this repo...

Then we can update js-libp2p's content-routing module to have both routers and valueStores, and if you enable the DHT it will just get added to both lists.

@vasco-santos
Copy link
Member

Do you think that we should have a separate DelegatedValueStore class? Perhaps we could still put it in this repo...

GIven we do not have typescript magic to easily see if X implements or not the interface, I think that is probably the best solution to what you suggest. However, it will require a libp2p breaking change to change configuration.

I am ok with shipping this as a breaking change, but if you want to start by keeping things simple we can just inspect if the router has a put/get method for now.

@yusefnapora
Copy link
Author

yusefnapora commented Oct 7, 2021

I realized that the reason I was getting errors passing Uint8Arrays to the ipfs-http-client is that we're depending on an old version of the client here. js-libp2p now depends on ^52.0.2, which requires Uint8Array instead of strings. So I bumped the dependency here and changed the code to only accept Uint8Array.

I also realized that my IPNS test fixture record expires, so I made a new one with a > 100 year expiration date.

@vasco-santos do you think the DelegatedValueStore should live in its own repository? I've started pulling this into js-libp2p locally to test it, and it's a little awkward at the moment since you need to do

import DelegatedValueStore from 'libp2p-delegated-content-routing/src/value-store'

I suppose we could export it from index.js instead, so you could do

import DelegatedContentRouter, { DelegatedValueStore } from 'libp2p-delegated-content-routing'

but that also feels a bit awkward. I'm not sure that's worth having one more repo to manage though.

@vasco-santos
Copy link
Member

Yes, that require is not the best UX. We could create a breaking change here and return named exports as:

import { DelegatedContentRouter, DelegatedValueStore } from 'libp2p-delegated-content-routing'

or have everything in index.js instead. I am more in favour of just merging the classes considering this, as otherwise it would probably make sense a new repo, but we are trying to reduce repos and not create new ones. What do you think?

@lidel
Copy link
Member

lidel commented Nov 8, 2021

@yusefnapora will you have bandwidth to continue this work during this week's tribute?
Can't wait for delegated IPNS resolution working in browsers ✨

@BigLep BigLep marked this pull request as draft January 28, 2022 15:55
@BigLep
Copy link

BigLep commented Jan 28, 2022

Moving this to draft until we're able to give a concerted effort on IPNS improvements as part of ipfs/js-ipfs#2921

@achingbrain
Copy link
Member

Fixed in #68

@achingbrain achingbrain deleted the feat/put-get-support branch May 31, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add supported for Put/Get operations (IPNS in browsers etc)
5 participants