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

Implement circuit relay v2 #345

Merged
merged 23 commits into from
Jan 15, 2024
Merged

Conversation

@@ -61,7 +61,11 @@ data class Multiaddr(val components: List<MultiaddrComponent>) {
* @throws IllegalArgumentException if existing component value doesn't match [value]
*/
private fun withComponentImpl(protocol: Protocol, value: ByteArray?): Multiaddr {
val existingComponent = getFirstComponent(protocol)
val existingComponent = if (has(Protocol.P2PCIRCUIT)) {
split { it == Protocol.P2PCIRCUIT }.get(1).getFirstComponent(protocol)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those existing withComponent methods hit their limitations with introducing relay protocol. I believe we would need to introduce a builder for Multiaddr which would be able to construct/modify components in a more explicit way.
This could be addressed later in a separate PR

@Nashatyrev
Copy link
Collaborator

Basically looks good to me!
I didn't dive deeply into the circuit implementation code though
I would be happy if you could add some (at least sanity) unit tests for protocol implementations and
if possible add the RelayTransport test as a subclass of TransportTests

Could you also please add a 🍋 for the Circuit Relay component row on the main README.md 😄

@ianopolous
Copy link
Contributor Author

It seems like the protobuf changes have broken a peerid test. It's not clear to me why yet.

@ianopolous
Copy link
Contributor Author

I had a look at making RelayTransportTest a subclass of TransportTests, but it seems quite awkward, because it needs the Host in the transport via setHost and the host builder modifier. It would be nice to find a better solution to this that didn't need to use HostBuilder().builderModifier()

Copy link
Collaborator

@Nashatyrev Nashatyrev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StefanBratanov may be you could give another review?

@StefanBratanov
Copy link
Collaborator

LGTM

@StefanBratanov may be you could give another review?

I am having a look.

Copy link
Collaborator

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to me as well

@Nashatyrev Nashatyrev merged commit ac6127b into libp2p:develop Jan 15, 2024
2 checks passed
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 this pull request may close these issues.

circuit-relay-v2
3 participants