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

chore: explicitly reject share commitments that are not == hash len #1867

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

evan-forbes
Copy link
Member

Overview

Noticed this while I was writing the block validity rules. This was still failing as expected since the commitment is check, but now we're being more explicit.

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 self-assigned this Jun 1, 2023
@MSevey MSevey requested a review from a team June 1, 2023 19:58
pkg/appconsts/global_consts.go Outdated Show resolved Hide resolved
x/blob/types/payforblob_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2023

Codecov Report

Merging #1867 (a794531) into main (348f795) will increase coverage by 21.80%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main    #1867       +/-   ##
=========================================
+ Coverage      0   21.80%   +21.80%     
=========================================
  Files         0      117      +117     
  Lines         0    13306    +13306     
=========================================
+ Hits          0     2901     +2901     
- Misses        0    10114    +10114     
- Partials      0      291      +291     
Impacted Files Coverage Δ
x/blob/types/payforblob.go 71.42% <100.00%> (ø)

... and 116 files with indirect coverage changes

Co-authored-by: Rootul P <rootulp@gmail.com>
@MSevey MSevey requested a review from a team June 1, 2023 21:20
@evan-forbes evan-forbes enabled auto-merge (squash) June 1, 2023 21:21
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. I don't know what our stance is towards clearer error messages. I can imagine returning ErrInvalidShareCommitment in this scenario would be relatively confusing to users. It would be also helpful now we have multiple share commitments to return which one was invalid by wrapping the error

@evan-forbes evan-forbes merged commit 431c817 into main Jun 7, 2023
18 checks passed
@evan-forbes evan-forbes deleted the evan/explicit-share-commitment-rejection branch June 7, 2023 09:15
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

Successfully merging this pull request may close these issues.

None yet

4 participants