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

feat: add share index to malleated (wrapped) txs #819

Merged
merged 4 commits into from
Aug 18, 2022

Conversation

evan-forbes
Copy link
Member

Description

Adds a share index to malleated txs so that we can perform sub tree root based message inclusion checks in the app.

Closes: #462

pass the share index when wrapping
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

The changes look good.
However, I still don't understand the reasoning behind. Perhaps when we have an ADR I will.

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

code change LGTM but I don't understand the rationale so can't provide a more helpful review

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK. An ADR would definitely be helpful in understanding why it's needed in this specific data structure. Or just more documentation.

@evan-forbes
Copy link
Member Author

will have an ADR soon. Since we are already wrapping malleated txs, we're adding the share index (messageStartIndex in the specs) to that existing wrapper.

https://celestiaorg.github.io/celestia-specs/latest/specs/data_structures.html#wrappedtransaction

@evan-forbes evan-forbes merged commit 60930ab into v0.34.x-celestia Aug 18, 2022
@evan-forbes evan-forbes deleted the evan/wrapped-tx branch August 18, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Use Wrapped Txs
4 participants