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 GetSharesByNamespace ordering #622

Merged
merged 4 commits into from
Apr 15, 2022
Merged

Fix GetSharesByNamespace ordering #622

merged 4 commits into from
Apr 15, 2022

Conversation

tzdybal
Copy link
Member

@tzdybal tzdybal commented Apr 14, 2022

Resolves #621.

@tzdybal
tests(service/share): add RandLightService and FillDag functions
bb7bba2 
Separation of Service creation and DAG filling adds extra flexibility
for tests. Test can use predefined shares, or modify random shares
before filling DAG.
@tzdybal
tests(service/share): add ordering assertion to TestService_GetShares…
f611dd6 
…ByNamespace
@tzdybal tzdybal added the kind:fix Attached to bug-fixing PRs label Apr 14, 2022
@tzdybal tzdybal self-assigned this Apr 14, 2022
@tzdybal
fix(service/share): make GetSharesByNamespace deterministic
324b4e2 
This change ensures that shares are always returned in same/correct order.
@tzdybal tzdybal marked this pull request as ready for review April 14, 2022 16:29
Wondertan
Wondertan reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix. And a special thank you for propper commit format!

Left two nits, otherwise LGTM

service/share/share.go Show resolved Hide resolved
service/share/share.go Outdated Show resolved Hide resolved
service/share/share.go Outdated Show resolved Hide resolved
@tzdybal tzdybal requested a review from Wondertan April 14, 2022 17:23
Wondertan
Wondertan approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

👍

renaynay
renaynay approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Thanks for finding + fixing :)

@codecov-commenter
Copy link

Codecov Report

Merging #622 (e262cf3) into main (16dba97) will decrease coverage by 0.20%.
The diff coverage is 68.60%.

@@            Coverage Diff             @@
##             main     #622      +/-   ##
==========================================
- Coverage   61.70%   61.49%   -0.21%     
==========================================
  Files          93       95       +2     
  Lines        4969     5020      +51     
==========================================
+ Hits         3066     3087      +21     
- Misses       1581     1606      +25     
- Partials      322      327       +5
Impacted Files Coverage Δ
node/config_opts.go 27.27% <0.00%> (-8.73%) ⬇️
node/node.go 60.86% <ø> (ø)
cmd/celestia/full.go 41.93% <33.33%> (-0.93%) ⬇️
cmd/celestia/light.go 41.93% <33.33%> (-0.93%) ⬇️
cmd/celestia/bridge.go 56.89% <50.00%> (-0.80%) ⬇️
cmd/flags_rpc.go 76.00% <76.00%> (ø)
service/share/share.go 80.00% <84.61%> (+0.23%) ⬆️
node/components.go 94.11% <100.00%> (-0.69%) ⬇️
node/config.go 88.88% <100.00%> (+0.51%) ⬆️
node/rpc/component.go 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28020da...e262cf3. Read the comment docs.

@tzdybal tzdybal merged commit c295ddc into main Apr 15, 2022
@tzdybal tzdybal deleted the tzdybal/fix_shares_order branch April 15, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liamsi liamsi liamsi approved these changes

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

Assignees

@tzdybal tzdybal

Labels
kind:fix Attached to bug-fixing PRs
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GetSharesByNamespace returns shares in non-deteministic order
5 participants
@tzdybal @codecov-commenter @liamsi @Wondertan @renaynay