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

Delete share related code #842

Closed
rootulp opened this issue Aug 22, 2022 · 3 comments · Fixed by #850
Closed

Delete share related code #842

rootulp opened this issue Aug 22, 2022 · 3 comments · Fixed by #850
Assignees
Labels

Comments

@rootulp
Copy link
Collaborator

rootulp commented Aug 22, 2022

Description

Share related code was copied (and refactored) from celestia-core to celestia-app in celestiaorg/celestia-app#627

We'd like to remove the share related code from celestia-core to avoid maintaining two copies of the same code.

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 23, 2022

Notes from attempting to resolve this issue on this branch:

  1. celestia-core shouldn't need to ComputeShares itself. This was previously used to generate the DataHash but celestia-core should instead use the DataHash returned from celestia-app in ResponsePrepareProposal.
  2. celestia-core performs transaction inclusion logic that seems non-trivial to move to celestia-app. Further investigation is needed here to determine how to keep this functionality after deleting the files in chore: move share encoding logic from core -> app celestia-app#627 (which include types and functions used by TxInclusion).
    func TxInclusion(codec rsmt2d.Codec, data types.Data, txIndex uint64) (types.TxProof, error) {

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 29, 2022

Given celestia-core and celestia-app need to access the share encoding code, we're considering extracting a separate repo for share related code. The new repo can be depended on by celestia-core and celestia-app.

The current plan is to extract a share repo after non-interactive defaults merges into celestia-app. cc @evan-forbes

@rootulp rootulp changed the title Delete share related code Migrate share related code to new share repo Aug 30, 2022
@evan-forbes evan-forbes self-assigned this Sep 9, 2022
@evan-forbes evan-forbes moved this to TODO in Celestia Node Sep 9, 2022
@evan-forbes evan-forbes moved this from TODO to In Progress in Celestia Node Sep 9, 2022
@rootulp rootulp changed the title Migrate share related code to new share repo Delete share related code Sep 12, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 12, 2022

Discussed with @evan-forbes :

  • We're going to move TxInclusion code from celestia-core to celestia-app (medium-term) or celestia-node (long-term)
  • Since share related code was already moved to celestia-app, it seems safe to delete it from celestia-core

@rootulp rootulp assigned rootulp and unassigned evan-forbes Sep 12, 2022
@rootulp rootulp removed S:blocked Status Blocked T:investigate Further investigation needed labels Sep 12, 2022
Repository owner moved this from In Progress to Done in Celestia Node Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants