-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
resolve: use unixfs ResolveOnce #5484
Conversation
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Can we switch to to the CoreAPI? It should select the correct resolver based on the namespace. |
Good point, I'll see how that works |
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
448dfc4
to
92a302e
Compare
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
92a302e
to
f915730
Compare
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good but I'd like to consider a few API options before we go with this.
// DhtRecordCount is an option for Name.Resolve which specifies how many records | ||
// we want to validate before selecting the best one (newest). Note that setting | ||
// this value too low will have security implications | ||
func (nameOpts) DhtRecordCount(rc int) NameResolveOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have DHT specific options like this or a more general purpose way to pass routing options (that is, go-libp2p-routing/options.Option
). That is:
func RoutingOption(r ropts.Option) NameResolveOption ...
Or is this leaking too much into this API?
My worry here is adding a ton of special-purpose options for each router we may implement.
Local: false, | ||
Cache: true, | ||
|
||
DhtRecordCount: 16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should hard-code this. Can we use the value from the DHT (unless we go with the "resolve options" solution in which case this point is moot).
Note: I'm fine keeping this as it is, I'd just like to consider a more generalized solution to adding more and more API options. |
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k This is wonderful. These issues have been plaguing us for ages. |
(This is assuming we want to make
ipfs resolve
operate on unixfs paths instead of ipld)Fixes #5270 (comment)