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

fix: compact share Count #778

Merged
merged 2 commits into from
Sep 23, 2022
Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Sep 23, 2022

Closes #777

I choose to fix the implementation to match the doc comment because it seems like a leaky abstraction for this function to also return the # of available bytes in the pending share. By removing the second return value, the compact share Count more closely matches the sparse share Count function signature.

@rootulp rootulp added bug Something isn't working chore optional label for items that follow the `chore` conventional commit labels Sep 23, 2022
@rootulp rootulp self-assigned this Sep 23, 2022
@rootulp rootulp changed the title fix: compact share count fix: compact share Count Sep 23, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #778 (5eab874) into main (53d688e) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
+ Coverage   23.24%   23.29%   +0.05%     
==========================================
  Files          71       71              
  Lines        8796     8790       -6     
==========================================
+ Hits         2045     2048       +3     
+ Misses       6578     6570       -8     
+ Partials      173      172       -1     
Impacted Files Coverage Δ
app/estimate_square_size.go 82.89% <100.00%> (+1.24%) ⬆️
pkg/shares/split_compact_shares.go 83.21% <100.00%> (+4.37%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rootulp rootulp marked this pull request as ready for review September 23, 2022 17:34
evan-forbes
evan-forbes previously approved these changes Sep 23, 2022
Comment on lines 95 to 96
txCount := txSplitter.Count()
evdCount := evdSplitter.Count()
Copy link
Member

Choose a reason for hiding this comment

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

[optional]
combine with return statement

@rootulp rootulp merged commit 88ca988 into celestiaorg:main Sep 23, 2022
@rootulp rootulp deleted the rp/compact-share-count branch September 23, 2022 18:51
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore optional label for items that follow the `chore` conventional commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CompactShareSplitter Count() doesn't implement its doc comment
3 participants