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

Refactor KademliaEvent #2283

Closed
supercmmetry opened this issue Oct 11, 2021 · 2 comments
Closed

Refactor KademliaEvent #2283

supercmmetry opened this issue Oct 11, 2021 · 2 comments

Comments

@supercmmetry
Copy link
Contributor

I was thinking if we could refactor InboundPutRecordRequest and InboundAddProviderRequest into a FilterRequest or something similar inside KademliaEvent.

So instead of this

KademliaEvent::InboundPutRecordRequest {
                source,
                connection,
                record,
            } => {
                println!(
                    "inbound put record request {:?} {:?}",
                    std::str::from_utf8(record.key.as_ref()).unwrap(),
                    std::str::from_utf8(&record.value).unwrap(),
                );

                self.kademlia.store_mut().put(record);
            }

we could have something like this:

KademliaEvent::FilterRequest{request} => {
        match request {
            KademliaFilter::InboundPutRecordRequest{record, ..} => {
                println!(
                    "inbound put record request {:?} {:?}",
                    std::str::from_utf8(record.key.as_ref()).unwrap(),
                    std::str::from_utf8(&record.value).unwrap(),
                );

                self.kademlia.store_mut().put(record);
            }
            _ => {}
        }
    }
@mxinden
Copy link
Member

mxinden commented Oct 15, 2021

I am fine combining the InboundPutRecordRequest and InboundAddProviderRequest under a single KademliaEvent variant.

I find FilterRequest a bit misleading as the remote is not actually requesting the local node to filter, but to store.

Today, when filtering is enabled, an inbound request is reported both through KademliaEvent::InboundPutRecordRequest and KademliaEvent::InboundRequestServed. We could rename the latter to e.g. KademliaEvent::InboundRequest and include the actual record in InboundRequest::PutRecord when filtering is enabled.

What do you think of the latter suggestion @supercmmetry?

@supercmmetry
Copy link
Contributor Author

Yeah, that's seems more apt @mxinden
I'll put up a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants