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

ipld: consider moving into separate repo #296

Closed
3 tasks
liamsi opened this issue Apr 22, 2021 · 11 comments
Closed
3 tasks

ipld: consider moving into separate repo #296

liamsi opened this issue Apr 22, 2021 · 11 comments

Comments

@liamsi
Copy link
Member

liamsi commented Apr 22, 2021

Summary

Optimint and other projects might use the current code under ipld:https://github.com/lazyledger/lazyledger-core/tree/40acb17283ebfe1c49b804a786464b93c2213a97/p2p/ipld

It makes sense to eventually move this out into a separate repository once the APIs stabilized a bit.

Action Items

  • Move ipld package into separate repo but keep the commit history in tact (possible)
  • make the package more self-contained (documentation, CI etc)
  • potentially abstract away some of the concrete APIs

Context / Original Openening Comment

Decide if we want to support using the IPLD plugin easily with vanilla IPFS. Previously, this was extensively used for testing and there is even a unit test, that required this logic (currently skipped):
https://github.com/lazyledger/lazyledger-core/blob/874df76f139b4a3effcd2a74d6078c9d08b6d1ba/p2p/ipld/plugin/nodes/nodes_test.go#L129-L130

Merging #294 will make this less easy to actually deploy an ipfs node with the plugin because building it with the Makefile will use some of the dependencies from the lazyledger-core go.sum. These dependencies most likely will differ from the deps used in the IPFS version (even if compiled locally). e.g. @evan-forbes noticed that after compiling the version in #294
plugin.Open("/redacted/.ipfs/plugins/lazyledger-plugin"): plugin was built with a different version of package golang.org/x/sys/cpu.

There certainly might be a way to get this working without moving the plugin into a separate repository but I think it would be easier and it also would make sense.

If we decide to go down that route, we should definitely keep the git history intact (which is possible).

@Wondertan
Copy link
Member

e.g. @evan-forbes noticed that after compiling the version in #294
plugin.Open("/redacted/.ipfs/plugins/lazyledger-plugin"): plugin was built with a different version of package golang.org/x/sys/cpu.

@liamsi, curious what info channel I missed where @evan-forbes reported that.

@liamsi
Copy link
Member Author

liamsi commented Apr 23, 2021

https://discord.com/channels/638338779505229824/804386443845042206/834807592060190760

@liamsi liamsi mentioned this issue Jun 2, 2021
17 tasks
@liamsi liamsi mentioned this issue Aug 13, 2021
31 tasks
@liamsi
Copy link
Member Author

liamsi commented Aug 16, 2021

ref: celestiaorg/celestia-node/issues/1
IMO, we probably still want to move the logic that both repositories will need into a separate repo.

@Wondertan
Copy link
Member

Wondertan commented Aug 17, 2021

@liamsi, agree. To do this we need to cherry-pick some of the changes done here here

@evan-forbes
Copy link
Member

Since we have to keep the DataAvailabilityHeader in celestia-core, and will already have to import that in celestia-node, I actually think we should keep logic needed by both repos in an independent package(s) in celestia-core. Just to have one less repo to keep track of versions.

@Wondertan
Copy link
Member

Having repo segregation is much cleaner in terms of code and the issue you create for that code. In long term, IMO, this only gives profits. The two problems with that are, as you've mentioned, versioning and CI. For the first on it is not a problem for me and I can handle that, for the second, we would need a unified CI, which we can postpone.

@evan-forbes
Copy link
Member

Per a sync discussion with @Wondertan I am now convinced that we actually should move the plugin, DAH, and common logic to a different repo, provided that we have a strict release policy. I think that's what was meant by

For the first on it is not a problem for me and I can handle that,

This policy needs to ensure that the same versions of each repo are being used across the board per each release. It should also be noted that we include this policy in our docs.

@Wondertan
Copy link
Member

Another thing is that we should always track that releases of top-level repos rely on the number releases of repo deeper in the tree, not on the specific commits. However, that's ok if we depend on specific commits in branches.

@evan-forbes
Copy link
Member

can we close this?

@liamsi
Copy link
Member Author

liamsi commented Sep 30, 2021

I think it is OK to close this. I think opening a super short issue in node which points to the deleted code could help to find the relevant portions.

@evan-forbes
Copy link
Member

IPLD code is linked in celestia-node issue #106, but just in case we need a convenient lookup later :
https://github.com/celestiaorg/celestia-core/tree/c4f2a98f4928d3b04517621c5792744c106ba306/pkg/da/ipld

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

No branches or pull requests

3 participants