pubky / pkarr

Public Key Addressable Resource Records (sovereign TLDs)
https://app.pkarr.org
MIT License
187 stars 20 forks source link

Cleanup client. #18

Closed dvc94ch closed 1 year ago

dvc94ch commented 1 year ago

Not sure you want to keep any of this after your changes...

Nuhvi commented 1 year ago

@dvc94ch was just reading through it, will probably study your changes and try to learn from them instead, as I drifted a bit.

I wasn't quite sure if I should venture to using Tokio or would that bother people who would rather to use sync libraries... still want to give a Sync interface a try, even though Tokio is probably inevitable especially once I add the the DHT.

Speaking of the DHT, the reason I had a Pkarr struct, is that I wanted that to abstract over queries over both the DHT and relays.

While you are here, have you tried using simple-dns? is it worth it that I try add methods like add_ip add_cname etc? or are you comfortable passing ResourceRecords instead?

There is also one more thing I am not sure about, users input will probably look like _derp_region.iroh., but in reality, it should be normalized to _derp_region.iroh.{zbase32 of public key}, do you think this should be left to when we implement a true DNS server on top? or do you agree it should be saved in correct canonical form? If the later, should I just leave it for the users to worry about that and pass me ready to use ResourceRecord struct? because the way simple-dns is implmented, all structs are created from &'a str and thus it is extremely painful to mutate them inside a method.

So the question is: do you know another more forgiving DNS parsing/formatting library? do you know a nice way to mutate users' ResoruceRecord(s) internally? or do you think I shouldn't care about this?

dvc94ch commented 1 year ago

I wasn't quite sure if I should venture to using Tokio or would that bother people who would rather to use sync libraries...

it just uses builtin async. tokio is only needed for tests and main. Now that you removed main.rs it can be added as a dev dependency. it's necessary for anything other than a cli tool to return futures.

While you are here, have you tried using simple-dns? is it worth it that I try add methods like add_ip add_cname etc? or are you comfortable passing ResourceRecords instead?

not sure. the only thing I want to do is convert a iroh_net::PeerAddr to a Packet. for now I'm building resource records manually.

Speaking of the DHT, the reason I had a Pkarr struct, is that I wanted that to abstract over queries over both the DHT and relays.

that's fine. I'd implement both individually first and then add a struct that selects either based on configuration.

So the question is: do you know another more forgiving DNS parsing/formatting library? do you know a nice way to mutate users' ResoruceRecord(s) internally? or do you think I shouldn't care about this?

Well, I guess I wouldn't worry about it too much for now, and focus on the dht part.

Nuhvi commented 1 year ago

@dvc94ch Are you using simple-dns from git because the unpublished last commit?

Also, I made some cleanup built on the main (first) commit in this PR, would you check it?

I explicetly use relay_get and relay_put to give my self chance to add more porcelain API without changing older methods, for example methods like relays_concurrent_get or even resolve that concurrently race between relays and DHT nodes replies etc.

Nuhvi commented 1 year ago

I also went ahead and implemented the normalize_name method so now all names created using SignedPacket::from_packet will be saved with the zbase32 TLD

dvc94ch commented 1 year ago

Can you add simple-dns from git? I'm using it to construct/parse txt records into strings.

This is wrong. The packet comes from the network, so anyone can create an invalid packet. from_bytes would have to check it's a valid packet, which it doesn't.

Packet::parse(&self.bytes[72..]).expect("is valid DNS packet")

Not sure why relay_put returns a reqwest response?

dvc94ch commented 1 year ago

with these changes I can use your rust branch for my prototype. we currently use substrates p2p networking apis, which is a wrapper around libp2p. but we want to move away from that in favor of iroh-net + pkarr + mdns.

dvc94ch commented 1 year ago

here is a trivial p2p networking layer using pkarr + iroh-net I'm using for my prototype. p2p networking has never been so easy...

dvc94ch commented 1 year ago

how much work is it to implement bep44 in mainline and have a dht demo working in rust? I guess that's your rough plan?

Nuhvi commented 1 year ago

Can you add simple-dns from git? I'm using it to construct/parse txt records into strings.

Ok, will do.

This is wrong. The packet comes from the network, so anyone can create an invalid packet. from_bytes would have to check it's a valid packet, which it doesn't.

Good catch, I will add a check, might as well add the parsed Packet in SignedPacket with a self refrential struct, helps memoize .packet() as well I guess.

Not sure why relay_put returns a reqwest response? I didn't find a reason to hide it behind a custom error, for example if it is 400, I want the consumer to be able to read the reason it was considered a bad request, which the reqwest Response already offers. Happy to change it if you have a better suggestion.

with these changes I can use your rust branch for my prototype. we currently use substrates p2p networking apis, which is a wrapper around libp2p. but we want to move away from that in favor of iroh-net + pkarr + mdns.

Let's GOOOO.

how much work is it to implement bep44 in mainline and have a dht demo working in rust? I guess that's your rough plan?

It will probably take some time not going to lie, my initial plan was just to add BEP0044 support to my fork of already existing DHT library see Mainline, but, while that would work, it won't be as fast as the JS version as it needs some work to be more optimistic (return early responses) and skip unnecessary bootstrapping and cache routing table to disk etc.

My plan there is to implement a closest() method just like in JS, that returns an async stream, so I can do work on messages as I receive them, once that is done, I will apply the same logic in Pkarr so both configured relays and the Mainline responses are racing to first valid and new version of the Packet is seen.

Would definitely need some help to figure out how to return an AsyncIterable or if Rust has the concept of callback functions like JS.

Nuhvi commented 1 year ago

@dvc94ch can you please check this example and tell me what is missing there that you need TXT record for, or provide me with a TXT example.

dvc94ch commented 1 year ago

AsyncIterable would be a Stream right? Or an impl Iterator<Item = Box<dyn Future<Output = T>>? Yeah you can do callbacks in rust. impl FnMut(Event) for example. but it's usually more idiomatic to use Future or Iterator or Stream.

dvc94ch commented 1 year ago

@dvc94ch can you please check this example and tell me what is missing there that you need TXT record for, or provide me with a TXT example.

I sent you a link above. It's not exactly dns compliant, but A/AAAA/CNAME records don't really work. I want to store a socket address, so I'm using a TXT record containing "1.2.3.4:1234".

Nuhvi commented 1 year ago

@dvc94ch Ah I missed the port number, well yes TXT makes sense then.