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.

Sign up for GitHub

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

Jump to bottom

Multiaddress refactor for issue #20 #58

Merged
merged 15 commits into from
Dec 6, 2023
Merged

Multiaddress refactor for issue #20 #58

merged 15 commits into from
Dec 6, 2023

Conversation

JuanuMusic
Copy link
Contributor

@JuanuMusic JuanuMusic commented Dec 2, 2023 •
edited
Loading

Replaced Multiaddr with Multiformats.Multiaddress project.
Chat sample working.
All tests passing.

SOLVES: #20

@JuanuMusic
Added multiaddr code
a0272fe 
Referenced on project and modified a bunch of files to reference it
@JuanuMusic
All tests passing
de646e8
@JuanuMusic
Added QUICv1 protocol
464a1d2 
QUIC marked as obsolete
Renamed MultiaddressBasedProtocolSelector
@JuanuMusic
Fixed tipo
e1329dc
@JuanuMusic
Changed Multiaddress target Framework to .NET 7.0
f97b5c3
@JuanuMusic
Update from main
4e1acac
@JuanuMusic JuanuMusic changed the title Multiaddress refactor Multiaddress refactor for issue #20 Dec 2, 2023
@flcl42
Copy link
Contributor

flcl42 commented Dec 4, 2023 •
edited
Loading

Thanks a lot for the contribution! Looks great!

Could you integrate cs-multiaddress as a git submodule? Feel free to add your fork
Could you remove that old implementation please? I mean MultiAddr

@flcl42 flcl42 requested a review from chertby December 4, 2023 09:26
@JuanuMusic
Copy link
Contributor Author

JuanuMusic commented Dec 4, 2023 via email

@JuanuMusic JuanuMusic marked this pull request as draft December 4, 2023 22:53
@JuanuMusic
Copy link
Contributor Author

JuanuMusic commented Dec 4, 2023 •
edited
Loading

Lets hold off on Merging. I found a couple issues running PubsubChat.

@flcl42 @rubo @chertby does the Libp2p.Protocols.Quic protocol support QUIC as well as QUICv1 ?

@flcl42
Copy link
Contributor

flcl42 commented Dec 5, 2023

Lets hold off on Merging. I found a couple issues running PubsubChat.

@flcl42 @rubo @chertby does the Libp2p.Protocols.Quic protocol support QUIC as well as QUICv1 ?

quic is obsolete, there are no plans to support it and it has never been tested.

@JuanuMusic
Copy link
Contributor Author

Understood. Im gettng errors actuallly by .net7 not having support for it.
Ill push the PR then and we can move forward from there.

@JuanuMusic JuanuMusic marked this pull request as ready for review December 5, 2023 11:39
flcl42
flcl42 reviewed Dec 5, 2023
View reviewed changes
src/libp2p/Libp2p/MultiaddressBasedSelectorProtocol.cs Outdated
{
protocol = channelFactory!.SubProtocols.FirstOrDefault(proto => proto.Id.Contains("quic")) ?? throw new ApplicationException("QUIC is not supported");
}
else if (context.LocalPeer.Address.Has<QUICv1>())
{
protocol = channelFactory!.SubProtocols.FirstOrDefault(proto => proto.Id.Contains("quic-v1")) ?? throw new ApplicationException("QUICv1 is not supported");
Copy link
Contributor

@flcl42 flcl42 Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

QUIC should be declined without an attempt to search for it.

@flcl42
Copy link
Contributor

flcl42 commented Dec 5, 2023 •
edited
Loading

Understood. Im gettng errors actuallly by .net7 not having support for it. Ill push the PR then and we can move forward from there.

It may work, but you need to customize your .NET: https://github.com/NethermindEth/dotnet-libp2p/blob/main/src/libp2p/Libp2p.Protocols.Quic/README.md. It's a pity that we need such manipulations, but for now it's the only way and a known .NET issue.

@JuanuMusic JuanuMusic requested a review from flcl42 December 5, 2023 16:16
@JuanuMusic
Copy link
Contributor Author

@flcl42 Looks like the tests failed because of a Server error on nuget. Might have been a temporary issue on their servers?
I am running it and it works perfectly.
Can you trigger the checks again?
image

flcl42
flcl42 approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@flcl42 flcl42 left a comment

Choose a reason for hiding this comment

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

Looks good. We can make submodule from cs-multiaddress in a separate pr

@JuanuMusic
Copy link
Contributor Author

Hmmm I added it as a git submodule, but looks like its just taking it as files from the same repo.
Do you need me to do anything or this is good to be merged?

@flcl42 flcl42 merged commit 0fb0046 into NethermindEth:main Dec 6, 2023
2 checks passed
@flcl42
Copy link
Contributor

flcl42 commented Dec 6, 2023

Merged! Thanks a lot!

@JuanuMusic JuanuMusic deleted the multiaddress-refactor branch December 6, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flcl42 flcl42 flcl42 approved these changes

@rubo rubo Awaiting requested review from rubo rubo is a code owner

@chertby chertby Awaiting requested review from chertby

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JuanuMusic @flcl42