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

Extract the namesys and the keystore submodules #7925

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

hsanjuan
Copy link
Contributor

Namesys is a very useful submodule. Given a ValueStore and a Datastore it can
resolve and publish /ipns/ paths.

This functionality does not need to be sequestered inside go-ipfs as it can
and should be used without IPFS, for example, for implementing lightweight
IPNS publishing services or for resolving /ipns/ paths.

"keystore" extraction was necessary, as there is a dependency to it in
namesys. Keystore is also a useful module by itself within the stack.

Fixes #6537

@hsanjuan
Copy link
Contributor Author

This is a draft. There are a number of linting problems in namesys repo that I want to fix.

Fun fact: I extracted go-ipfs-keystore 2 years ago. In the end it seems it was not needed and go-ipfs was never updated to use the extracted version. I have re-extracted it.

namesys has been extracted keeping the git history (git subtree split...). It had dependencies to go-ipfs/core in the republisher tests (see ipfs/go-namesys@cfd5097). These have been removed by manually creating libp2p-hosts+dht+datastore+keystore objects. Tests pass and Travis has been setup in the same way as other modules. go.mod dependency versions have been taken from current go-ipfs master.

@hsanjuan hsanjuan marked this pull request as ready for review February 19, 2021 23:01
@hsanjuan
Copy link
Contributor Author

This is ready from my side. go mod tidy has made some small additional changes but seems fine.

@aschmahmann
Copy link
Contributor

The extraction LGTM and thanks a ton for the better documentation 🙏. Although, I noticed that you changed how the republisher tests were working, what did we get out of reworking those tests?

Obviously there's more work to do here since the namesys interfaces aren't quite right and we can do that in follow-up PRs to go-ipfs + go-namesys. It's going to be annoying to keep doing duplicate PRs (which is why we kept things in the same repo), but I guess that's the tradeoff with the extraction here.

To me one of the main benefits of the extraction is that it becomes really easy to see and work towards what a clean interface for go-namesys should be. Some things I noticed that it'd be nice to do now, but we could do later if they're complex and will make this take too long to merge, are:

  • we should bump the go.mod and testing to Go 1.15
  • Remove the unused circular dependency on go-ipfs
  • InitializeKeyspace has a dependency on go-ipfs-pinner and just is very specific to go-ipfs and so should live in go-ipfs
  • The tests should not need dependencies on go-unixfs or go-libp2p-kad-dht
  • [harder] go-namesys has a dependency on interface-go-ipfs-code, but the point of the extraction is to make namesys separable from go-ipfs

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Mar 8, 2021

It is much better to merge and follow up on smaller PRs.

Although, I noticed that you changed how the republisher tests were working, what did we get out of reworking those tests?

Removed dependencies from go-namesys to go-ipfs helper functions. Essentially consisted in creating libp2p hosts manually iirc.

An extraction PR is not the place to refactor anything other than the minimal necessary for the extraction itself as it gets tangled right away. The longer it stays open, the more confusing it is and the more work it creates. I am happy creating follow up issues for the above points in their repositories and personally committing to move forward with them, including bringing things like InitializeKeyspace back to go-ipfs.

Namesys is a very useful submodule. Given a ValueStore and a Datastore it can
resolve and publish /ipns/ paths.

This functionality does not need to be sequestered inside go-ipfs as it can
and should be used without IPFS, for example, for implementing lightweight
IPNS publishing services or for resolving /ipns/ paths.

"keystore" extraction was necessary, as there is a dependency to it in
namesys. Keystore is also a useful module by itself within the stack.

Fixes #6537
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM. I did a go mod tidy and go fmt ./... and force pushed, assuming tests are still passing we'll merge and then handle changes in follow up PRs.

Thanks for doing this and thanks in advance with the help with the followup PRs🙏

@hsanjuan
Copy link
Contributor Author

then handle changes in follow up PRs.

I'll open issues and ping you.

@aschmahmann aschmahmann merged commit 3393b4a into master Mar 14, 2021
@hsanjuan hsanjuan deleted the feat/extract-namesys branch March 14, 2021 17:44
@hsanjuan
Copy link
Contributor Author

The following follow-ups have appeared:

@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
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.

Extracting IPNS + Namesys from IPFS
2 participants