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(store): make storePrefix actually optional #69

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Jul 6, 2023

When we set the default value on the private config field while requiring it, we brake the serialization of the Parameters struct.
Particularly when we deserialize Parameters, the storePrefix is empty while the validation requires it. Because it's private, there is also no way to set it on the node, so this fix is necessary. This is also good learning that we should be cautious while introducing such optinonals

@codecov-commenter
Copy link

Codecov Report

Merging #69 (b389bbe) into main (0c1ebfb) will increase coverage by 0.24%.
The diff coverage is 70.73%.

@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
+ Coverage   65.57%   65.81%   +0.24%     
==========================================
  Files          35       35              
  Lines        2806     2832      +26     
==========================================
+ Hits         1840     1864      +24     
- Misses        811      813       +2     
  Partials      155      155              
Impacted Files Coverage Δ
store/options.go 54.05% <ø> (+2.83%) ⬆️
sync/sync.go 64.32% <ø> (ø)
sync/sync_head.go 62.40% <41.17%> (+0.36%) ⬆️
sync/sync_getter.go 90.47% <89.47%> (-9.53%) ⬇️
store/store.go 63.69% <100.00%> (+0.50%) ⬆️

... and 1 file with indirect coverage 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.

ugh

store/store.go Show resolved Hide resolved
@Wondertan Wondertan merged commit 0574305 into main Jul 6, 2023
2 checks passed
@Wondertan Wondertan deleted the store/store-prefix branch July 6, 2023 16:57
Wondertan added a commit that referenced this pull request Jul 10, 2023
If this test had existed, the bug fixed by #69 wouldn't happen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants