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.

Sign up for GitHub

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

Jump to bottom

fix(share/getters)!: return nil shares from all getters in non-inclusion case #3454

Merged
merged 2 commits into from
Jun 3, 2024
Merged

fix(share/getters)!: return nil shares from all getters in non-inclusion case #3454

merged 2 commits into from
Jun 3, 2024

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Jun 3, 2024 •
edited
Loading

Make return of shares consistent accross tall getter, when encountering non-inclusion case
Improves unnecessary allocations of empty slice.

Resolves #3448

@walldiss walldiss added area:shares Shares and samples kind:fix Attached to bug-fixing PRs labels Jun 3, 2024
@walldiss walldiss self-assigned this Jun 3, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 44.52%. Comparing base (2469e7a) to head (74e933b).
Report is 88 commits behind head on main.

Files Patch % Lines
share/ipld/get_shares.go 33.33% 1 Missing and 1 partial ⚠️
share/getter.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3454      +/-   ##
==========================================
- Coverage   44.83%   44.52%   -0.31%     
==========================================
  Files         265      273       +8     
  Lines       14620    15346     +726     
==========================================
+ Hits         6555     6833     +278     
- Misses       7313     7719     +406     
- Partials      752      794      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cristaloleg
cristaloleg approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

SGTM

@christopherbrumm christopherbrumm mentioned this pull request Jun 3, 2024
Closed
@Wondertan
Copy link
Member

Technically breaking, so let's mark it as so.

@walldiss walldiss changed the title fix(share/getters): return nil shares from all getters in non-inclusion case !fix(share/getters): return nil shares from all getters in non-inclusion case Jun 3, 2024
@walldiss walldiss added the kind:break! Attached to breaking PRs label Jun 3, 2024
@walldiss walldiss enabled auto-merge (squash) June 3, 2024 12:08
@Wondertan Wondertan changed the title !fix(share/getters): return nil shares from all getters in non-inclusion case fix(share/getters)!: return nil shares from all getters in non-inclusion case Jun 3, 2024
@walldiss walldiss merged commit c727d58 into celestiaorg:main Jun 3, 2024
26 checks passed
ramin pushed a commit that referenced this pull request Jun 6, 2024
@walldiss @ramin
!fix(share/getters): return nil shares from all getters in non-inclus…
ca3c8f1 
…ion case (#3454)

Make return of shares consistent accross tall getter, when encountering
non-inclusion case
Improves unnecessary allocations of empty slice.

Resolves #3448
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristaloleg cristaloleg cristaloleg approved these changes

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay Awaiting requested review from renaynay renaynay is a code owner

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs is a code owner

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

@ramin ramin Awaiting requested review from ramin

Assignees

@walldiss walldiss

Labels
area:shares Shares and samples kind:break! Attached to breaking PRs kind:fix Attached to bug-fixing PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

share.GetSharesByNamespace: non-deterministic response
4 participants
@walldiss @codecov-commenter @Wondertan @cristaloleg