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

refactor(share/ipld): Remove dependency on redundant namespaceID prepend #75

Closed
renaynay opened this issue Oct 17, 2022 · 0 comments
Closed

Comments

@renaynay
Copy link
Member

    After discussing with @Wondertan, we realized that https://github.com/celestiaorg/celestia-node/issues/183 can be tackled without modifying the NMT library at all (and instead by modifying this [visitor](https://github.com/celestiaorg/celestia-node/blob/ed4cdf03f2bd35c30d5b9874d4bc40bea6f5a061/ipld/nmt_adder.go#L34)). So we could put this on the back burner for now. 

I still think the suggested change makes sense. It would also allow us to delete the PrefixedData type entirely. But it might make sense to do this as part of a somewhat larger refactoring (can/should be split into several PRs) where we also tackle #15. In it's current form and because of the usage of this interface we still need to do a copy which merges the nid and the data. Hlib convinced me that it is better to hide this copying inside of the NMT though. Currently, it happens in the wrapper which is bad usability. Especially as Hlib put it:

The reason is; right now, we can avoid storing unnecessary data, but it's hacky. Meaning that on Push we need to prepend, and on Visit remove that we prepend so we don't store it

Originally posted by @liamsi in #55 (comment)

@renaynay renaynay reopened this Oct 17, 2022
@renaynay renaynay closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2022
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

1 participant