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!: move all blob share commitment code to the inclusion package #2770

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

evan-forbes
Copy link
Member

Overview

We currently have code relating to creating and proving commitment across the inclusion, x/blob/types, and shares package. This api breaking refactor moves all of the commitment code to the inclusion package.

The reasoning behind this is that it will eventually allow for users to import commitment creation and proving code while not having to import the blob module, tendermint, or the sdk.

The reasoning behind keeping it in the inclusion instead of shares is that entire reason for making a commitment is to prove inclusion. So the name seems to fit better.

The reasoning behind note keeping it in the blob package was that it was difficult to not have an import cycle since this logic requires the share splitting logic for blobs.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@evan-forbes evan-forbes added warn:api breaking item will be break an API and require a major bump refactor optional label for items that are related to implementation work and do not change functionality labels Oct 25, 2023
@evan-forbes evan-forbes self-assigned this Oct 25, 2023
@celestia-bot celestia-bot requested a review from a team October 25, 2023 20:22
@evan-forbes evan-forbes mentioned this pull request Oct 25, 2023
5 tasks
rootulp
rootulp previously approved these changes Oct 25, 2023
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.

LGTM

@evan-forbes evan-forbes marked this pull request as draft October 26, 2023 10:18
@evan-forbes
Copy link
Member Author

evan-forbes commented Oct 26, 2023

converting to a draft to fix the CI. which we should actually not expect to break given we just moved things around. will report back after finding the fix

edit: see dcf2c5d for the fix. I only renamed one function and didn't also change the input so it was taking the square root twice, which broke all the tests

@evan-forbes evan-forbes changed the title refactor!(inclusion): move all blob share commitment code to the inclusion package refactor!: move all blob share commitment code to the inclusion package Oct 26, 2023
@evan-forbes evan-forbes marked this pull request as ready for review October 26, 2023 18:45
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.

This seems a logical change. I think further to this I would like to see inclusion and proof combined. Inclusion is simply a form of proof

@evan-forbes
Copy link
Member Author

I think further to this I would like to see inclusion and proof combined. Inclusion is simply a form of proof

yeah I agree. When writing the blob share commitment proofs, it becomes very clear that this is an issue

@evan-forbes evan-forbes requested a review from rootulp October 27, 2023 14:06
@evan-forbes evan-forbes enabled auto-merge (squash) October 27, 2023 14:07
@evan-forbes evan-forbes merged commit 73942bf into main Oct 27, 2023
28 of 29 checks passed
@evan-forbes evan-forbes deleted the evan/commitment-refactor branch October 27, 2023 18:07
@rootulp
Copy link
Collaborator

rootulp commented Oct 27, 2023

hmm I think conventional commit PR title check failed here because the ! was included prior to the scope and the spec states that it should follow the scope.

refactor(inclusion)!: I expect this to work
refactor!(inclusion): I expect this to fail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor optional label for items that are related to implementation work and do not change functionality warn:api breaking item will be break an API and require a major bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants