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

x/signal: follow-ups from docs #3276

Closed
rootulp opened this issue Apr 8, 2024 · 1 comment · Fixed by #3277
Closed

x/signal: follow-ups from docs #3276

rootulp opened this issue Apr 8, 2024 · 1 comment · Fixed by #3277
Assignees
@rootulp
Labels
priority:low optional label to track the relative priority of planned items WS: V2 ✌️ lemongrass hardfork related

Comments

@rootulp
Copy link
Collaborator

rootulp commented Apr 8, 2024

Context

#3224 merged and there are some untracked follow ups. It seems like there's agreement on doing them.

Proposal

  1. Can we move some of the unit tests from tally_test.go to keeper_test.go?
  2. Why does ResetTally only delete versions less than the current version? Why not delete all versions in the map?
@rootulp rootulp added x/upgrade WS: V2 ✌️ lemongrass hardfork related labels Apr 8, 2024
@rootulp rootulp self-assigned this Apr 8, 2024
@rootulp rootulp mentioned this issue Apr 8, 2024
Merged
@rootulp rootulp added needs:triage priority:low optional label to track the relative priority of planned items and removed needs:triage labels Apr 9, 2024
@rootulp
Copy link
Collaborator Author

rootulp commented Apr 11, 2024

Can we move some of the unit tests from tally_test.go to keeper_test.go?

Was handled in #3290

Why does ResetTally only delete versions less than the current version? Why not delete all versions in the map?

Still needs to be handled.

@rootulp rootulp changed the title x/upgrade: follow-ups from docs x/signal: follow-ups from docs Apr 11, 2024
rootulp added a commit that referenced this issue Apr 16, 2024
@rootulp @cmwaters
refactor(x/signal): reset all versions via ResetTally (#3277)
541ddf1 
Closes #3276

---------

Co-authored-by: Callum Waters <[email protected]>
ninabarbakadze pushed a commit to ninabarbakadze/celestia-app that referenced this issue Apr 17, 2024
@rootulp @cmwaters @ninabarbakadze
refactor(x/signal): reset all versions via ResetTally (celestiaorg#…
3682779 
…3277)

Closes celestiaorg#3276

---------

Co-authored-by: Callum Waters <[email protected]>
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@rootulp @cmwaters
refactor(x/signal): reset all versions via ResetTally (#3277)
b430fa4 
Closes celestiaorg/celestia-app#3276

---------

Co-authored-by: Callum Waters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rootulp rootulp

Labels
priority:low optional label to track the relative priority of planned items WS: V2 ✌️ lemongrass hardfork related
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(x/signal): reset all versions via ResetTally rootulp/celestia-app
1 participant
@rootulp