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!: wrap ShareProof in ResultShareProof #1313

Merged
merged 3 commits into from
Apr 19, 2024
Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Apr 18, 2024

Description

This PR wraps the ShareProof into a ResultShareProof in the API. However, It doesn't change the custom query response since it's internal data and there is no need to wrap it.

Closes #1306

@rach-id rach-id self-assigned this Apr 18, 2024
@rach-id rach-id requested a review from a team as a code owner April 18, 2024 08:36
@rach-id rach-id requested review from staheri14 and evan-forbes and removed request for a team April 18, 2024 08:36
@rootulp
Copy link
Collaborator

rootulp commented Apr 18, 2024

Just flagging that AFAIK celestia-app has no immediate plans to upgrade to a release cut from the celestia-core main branch because main contains breaking changes to ABCI methods. In other words, I don't think this should target main. IMO we should make the fix in a non-breaking way and target v0.34.x-celestia branch, then cut a minor release of celestia-core, and bump to it in celestia-app main and v1.x. That way we can fix this way before celestia-app v2 is released.

@rach-id
Copy link
Member Author

rach-id commented Apr 18, 2024

We can wait until v2 is released for this fix, no one is asking for this urgently, that's why I am targetting main and going for the easy fix

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM. If there's no urgency and it's just a code hygiene change then we can leave it in main else we can also backport this to v0.34.x-celestia in a non-breaking way (as Rootul suggests)

As a side question, is this endpoint just for blobstream or are there other users of it. I assume celestia-node doesn't meed it

@rach-id
Copy link
Member Author

rach-id commented Apr 19, 2024

I assume celestia-node doesn't meed it

Yes, they don't. In fact, there is a work towards having similar proofs in node but it's not highly prioritized

@rach-id rach-id merged commit c89f6f9 into main Apr 19, 2024
18 checks passed
@rach-id rach-id deleted the introduce-result-share-proof branch April 19, 2024 16:35
@rootulp
Copy link
Collaborator

rootulp commented Apr 19, 2024

Just flagging this won't get included in celestia-app anytime soon because we're not planning on cutting a release from the main branch of this repo. @rach-id If you want this used by celestia-app, you need to backport this to v0.34.x-celestia and then cut a new release and bump to it in celestia-app

@rach-id
Copy link
Member Author

rach-id commented Apr 19, 2024

We're planning on having it in v2, so it can wait. Thanks

@rootulp
Copy link
Collaborator

rootulp commented Apr 19, 2024

celestia-core main won't be used for celestia-app v2. If you do nothing this PR won't be used in celestia-app v2.

@rach-id
Copy link
Member Author

rach-id commented Apr 19, 2024

Oh really? Won't v2 be cut from main when we want to bump app to v2?
Which branch would it be?

@rootulp
Copy link
Collaborator

rootulp commented Apr 22, 2024

Likely v0.34.x-celestia. There's a lot of confusion about release branches currently and we're still working on a strategy but I'm pretty confident main won't be used for the forseeable future. It may be used for celestia-app v3. A more applicable name for main may be experimental-unfork at least for now.

Ref:

@rach-id
Copy link
Member Author

rach-id commented Apr 22, 2024

Thanks for linking this 🙏 Once there is a decision around which branch will be used to cut v2, let's backport this PR there

@rootulp
Copy link
Collaborator

rootulp commented Apr 23, 2024

FYI we had another meeting about celestia-core release branches without a firm decision on which branch celestia-app will use for v2. See notes.

@rach-id
Copy link
Member Author

rach-id commented Apr 23, 2024

Thanks for keeping me in the loop 🙏 Let's hope to have a decision soon :D

rach-id added a commit that referenced this pull request May 8, 2024
## Description

This PR wraps the `ShareProof` into a `ResultShareProof` in the API.
However, It doesn't change the custom query response since it's internal
data and there is no need to wrap it.

Closes #1306

(cherry picked from commit c89f6f9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants