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

Get rid of libp2p dependency in sc-authority-discovery #4859

Open
dmitry-markin opened this issue Jun 21, 2024 · 9 comments
Open

Get rid of libp2p dependency in sc-authority-discovery #4859

dmitry-markin opened this issue Jun 21, 2024 · 9 comments
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I4-refactor Code needs refactoring. T0-node This PR/Issue is related to the topic “node”.

Comments

@dmitry-markin
Copy link
Contributor

dmitry-markin commented Jun 21, 2024

libp2p types in authority-discovery should be replaced with network backend agnostic types from sc-network-types.

@dmitry-markin dmitry-markin added T0-node This PR/Issue is related to the topic “node”. I4-refactor Code needs refactoring. labels Jun 21, 2024
@bkchr bkchr added the C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. label Jun 24, 2024
@gotnoshoeson
Copy link

I'll have a go at this one

@gotnoshoeson
Copy link

So I can remove the libp2p dependency but I'm not quite sure how/where to replace with sc-network-types. I don't see libp2p being used anywhere in the sc-authority-discovery directory.

@bkchr
Copy link
Member

bkchr commented Jul 1, 2024

If it is not being used, then the pr would just be about removing the crate and that's it ;)

@alexggh
Copy link
Contributor

alexggh commented Jul 2, 2024

So I can remove the libp2p dependency but I'm not quite sure how/where to replace with sc-network-types. I don't see libp2p being used anywhere in the sc-authority-discovery directory.

You have to look for re-exported types as well for example the KademliaKey from here: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/authority-discovery/src/worker.rs#L45C30-L45C41.

@eagr
Copy link
Contributor

eagr commented Aug 19, 2024

https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/authority-discovery/src/worker.rs#L171-L190

Worker is a public interface of sc-authority-discovery, which is relying on Record from libp2p. It seems to me ridding libp2p and replacing the type would be a breaking change. That right?

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Aug 30, 2024

I don't see these types used in the public API, i.e., in sc_authority_discovery::service::Service.

As for the types used internally (for example, in DhtEvent::ValueFound) — those should be replaced with network-backend-agnostic types put into sc-network-types. The sc-network interface should be updated accordingly.

@ndkazu
Copy link
Contributor

ndkazu commented Sep 26, 2024

Hello! Started working on this issue, and would like to get a first feedback to see if I'm on the right track:
#5842

@ndkazu
Copy link
Contributor

ndkazu commented Sep 30, 2024

Working now on removing libp2pdependency from all the tests in sc-authority-discovery.

@ndkazu
Copy link
Contributor

ndkazu commented Oct 1, 2024

@dmitry-markin , @bkchr : removed libp2p dependencies from sc-authority-discovery. Review needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I4-refactor Code needs refactoring. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: No status
Development

No branches or pull requests

6 participants