Ipld selector traversals implementation

[austinabell]

Summary of changes
Changes introduced in this pull request:

• Implements walk functions for selectors, which is the last missing part needed for GraphSync

  • The part that still can be done is Focus impls, which is just being able to traverse ipld by path, which isn't needed for any use case we have (afaik)

• I removed exposing the selector operation since there should be no external use of those (if there is it can just be easily updated when needed)

Test suite are json files that will probably be moved out to some shared repo, but for now I will leave as is

Reference issue to close (if applicable)

Closes #241

Other information and links

[StaticallyTypedAnxiety]

can we use a filter_map + map here? interests.filter_map.(...).map(..)

[austinabell]

I'm a bit confused by this, can you explain? Do you mean filtering based on the available paths of the ipld?

[StaticallyTypedAnxiety]

filtering based on the ipld.look_up_segment with a filtermap essentially. would save us from having the continue clause there since filter_map is based on options. I guess the issue though is that you might have to unwrap v in the map clause but it should be already filtered out

[austinabell]

yeah I had considered, I think the reason I hadn't was because integer path segments can be used ambiguously as fields of a map, but let me revisit to remember why I didn't go through with it

Edit: yeah it very quickly gets more unreadable, I didn't try very in depth though. Do you have a suggestion on how this can be done? I think it's possible, there just isn't an idiomatic call that fits this need well

[timvermeulen]

The trouble here is that calling filter_map with ipld.lookup_segment will give you an iterator of Iplds without the paths (which you still need) so it does indeed become pretty verbose:

for (ps, ipld) in interests
    .into_iter()
    .filter_map(|ps| ipld.lookup_segment(&ps).map(|ipld| (ps, ipld)))
{
    // ...
}

Not really an improvement. Unless there's a way to improve this which I haven't thought of

[austinabell]

There is a bit of a bug in the go impl and by extension this right now. I'm writing a test for the edge case and will restructure to fix so hold off on reviewing

Edit: good to go now

[timvermeulen]

I suggest we just make this L instead of Option<L>, and implement LinkResolver for Option<T> whenever T implements it. Option's implementation of the trait can then handle a missing resolver (i.e. return Ok(None)), rather than baking it into this logic here

[austinabell]

for what benefit? That just seems less readable

[austinabell]

Like, what I mean by this is when you don't specify a link resolver the type inference is either more verbose or you pass in something other than None into the function and that be less readable. I think usability is more important than possibly some checks removed in the code (and less prone to unexpected behaviour if expanded on)

[timvermeulen]

I guess what I'm saying is that it's double that the link resolver is optional, and the ipld it returns is also optional. if you require an L and use () or some other type instead of None then the code becomes simpler, I've tried it — I'm also happy to make this change later if my description isn't really clear

[austinabell]

Yeah but then it's not obvious that the resolver is optional and a little janky to read when you are passing () or otherwise in as a parameter. That is specifically what Option is for, plus also you are auto deriving the trait impl for every linkresolver impl.

Maybe I'm misunderstanding a detail of what you are saying. I am open to considering, it just seems like an opinionated and less idiomatic API

[timvermeulen]

That is specifically what Option is for

I would definitely agree in the case of a concrete type; the annoying thing about Option in combination with generics though is that the None is still generic over some T that you don't really care about when you don't want to pass a link resolver at all.

Anyway, this whole API is inherently not very idiomatic Rust because of how directly it was ported from Go, so it's not a big deal whether this part is idiomatic or not. This is definitely not a blocker, but rather something to think about when we decide to refactor it.

[timvermeulen]

Suggested change

	async fn load_link(&self, link: &Cid) -> Result<Option<Ipld>, String> {
	Err("load_link not implemented on the LinkResolver".into())
	}
	async fn load_link(&self, link: &Cid) -> Result<Option<Ipld>, String>;

Seems to me like you'd want to require conforming types to implement this? They can always return an error themselves if that's appropriate behavior

[austinabell]

Nah there will be other methods added to this if/when they are needed, makes sense to just default to not implemented so someone implementing their own resolver doesn't have to specify the functions if they don't want to use

[timvermeulen]

Should it return an error if you don't implement it though, rather than returning Ok(None)?

[austinabell]

Yeah so I'm not a huge fan of the setup, but the intention is either you don't specify a resolver and those will just be skipped, or you do specify a resolver and if it requires that function for resolving, then it would error.

The idea being if you specify a resolver, it shouldn't quietly skip if it's not configured correctly, thoughts?

[timvermeulen]

If quietly skipping links is not deemed acceptable then I would generally speaking rather have the compiler tell me about it than seeing some error at runtime. But I'm having a hard time seeing how this API will evolve exactly so I can't say for sure which approach is the most logical

[austinabell]

Ah ya mb I forgot about this and had merged in, definitely open to switching it to not default, just for now doesn't matter because this isn't an API used outside of our codebase yet

For some reason I thought I already made this change