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

using nmt wrapper results in cyclic import in rsmt2d #229

Closed
staheri14 opened this issue Jul 5, 2023 · 5 comments
Closed

using nmt wrapper results in cyclic import in rsmt2d #229

staheri14 opened this issue Jul 5, 2023 · 5 comments
Assignees

Comments

@staheri14
Copy link
Contributor

Problem

To effectively test the expected behavior of rsmt2d when the underlying shares are out of order (to address the following issue), we must utilize the NMT wrapper as the TreeConstructorFn. However, this approach leads to a cyclic import issue. Specifically, the github.com/celestiaorg/celestia-app/pkg/wrapper imports github.com/celestiaorg/rsmt2d, and vice versa.

Proposal

The NMT wrapper relies on rsmt2d.TreeConstructorFn and rsmt2d.Tree in these lines of code, as well as rsmt2d.Axis in this part of the code.

To address the circular dependency issue, a possible solution is to create two new packages within rsmt2d. The first package would contain rsmt2d.TreeConstructorFn, rsmt2d.Tree, and their related functions. This package would essentially involve renaming the package name of tree.go to something like tree. The second package would host rsmt2d.Axis and its relevant methods, with a package name like axis.

@staheri14 staheri14 self-assigned this Jul 5, 2023
@staheri14
Copy link
Contributor Author

After looking into it further, I found another way to fix this issue. We can change the package of all the tests in rsmt2d to a new package called rsmt2d_test. However, keep in mind that this change might require us to make some private fields and methods accessible to accommodate the package change.

@staheri14
Copy link
Contributor Author

I am currently working on refactoring the tests into a distinct package, i.e., this approach . Will follow up with a PR.

@evan-forbes
Copy link
Member

is there any way that we could just use nmt or another existing merkle tree? I feel like including nmt or another existing tree as a dependency is one thing, but include the wrapper (and thus some portion of the app) should be avoided since that will bubble down to all other projects that use rsmt2d

@staheri14
Copy link
Contributor Author

Actually importing nmt is not sufficient in this case, we need the nmt wrapper since it takes care of adding parity namespaces to the erasure-coded portion of the EDS.

is there any way that we could just use nmt or another existing merkle tree?

I'll explore alternative options to see what could be viable.

@staheri14
Copy link
Contributor Author

Closing this issue as I figured out another way to test rsmt2d with nmt as the underlying tree without causing cyclic imports.

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

2 participants