-
Notifications
You must be signed in to change notification settings - Fork 238
fix: emergency poll should be finished if voting period is over #302
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
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the governance proposal handling in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #302 +/- ##
==========================================
+ Coverage 41.03% 41.04% +0.01%
==========================================
Files 268 268
Lines 25720 25728 +8
==========================================
+ Hits 10553 10559 +6
- Misses 13529 13532 +3
+ Partials 1638 1637 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/gov/abci_test.go (2)
288-299
: Consider enhancing test clarity with explicit vote assertions.While testing the zero-vote scenario is valid, consider:
- Adding explicit assertions for the vote count
- Improving the comment to clarify "not enough votes" means "zero votes"
- // not enough votes + // Verify proposal stays in voting period with zero votes cast + votingPower, err := app.GovKeeper.GetTotalVotingPower(ctx, proposal) + require.NoError(t, err) + require.Equal(t, math.NewInt(0), votingPower)
300-310
: LGTM! Consider adding assertions for emergency tally scheduling.The test effectively validates that emergency proposals are rejected when the voting period ends without sufficient votes, which directly addresses the PR objective.
Consider adding assertions to verify that no more emergency tallies are scheduled after rejection:
require.NoError(t, err) proposal, err = app.GovKeeper.Proposals.Get(ctx, 1) require.NoError(t, err) require.Equal(t, proposal.Status, v1.StatusRejected) + // Verify no more emergency tallies are scheduled + require.Nil(t, proposal.EmergencyNextTallyTime)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.github/workflows/lint.yml
is excluded by!**/*.yml
📒 Files selected for processing (3)
x/gov/abci.go
(1 hunks)x/gov/abci_test.go
(1 hunks)x/gov/common_test.go
(3 hunks)
🔇 Additional comments (6)
x/gov/common_test.go (4)
62-62
: LGTM! Appropriate removal of unused parameter.
The change to use _
instead of t *testing.T
is correct as the function doesn't perform any test assertions.
127-127
: LGTM! Appropriate removal of unused parameter.
The change to use _
instead of t *testing.T
is correct as the function is a simple message constructor without test assertions.
Line range hint 36-42
: Verify emergency proposal parameters for test scenarios.
The emergency-specific constants (emergencyMinDeposit
and emergencyTallyInterval
) are crucial for testing the emergency proposal functionality mentioned in the PR objectives. Please verify that:
- The
emergencyMinDeposit
amount (100,000) is appropriately higher than regularminDeposit
(10,000) - The
emergencyTallyInterval
(10 minutes) is suitable for testing various voting period scenarios
Let's check if these parameters align with production values:
#!/bin/bash
# Description: Compare test parameters with production configuration
# Search for emergency deposit configurations in production code
echo "Checking emergency deposit configurations..."
rg "EmergencyMinDeposit.*=.*" --type go --no-filename
# Search for emergency tally interval configurations
echo "Checking emergency tally interval configurations..."
rg "EmergencyTallyInterval.*=.*" --type go --no-filename
136-136
: LGTM! Appropriate removal of unused parameter.
The change to use _
instead of t *testing.T
is correct as the function is a simple message constructor without test assertions.
Let's verify the test coverage for emergency proposals, given this PR's focus on emergency proposal handling:
✅ Verification successful
Test coverage for emergency proposals is comprehensive
The codebase shows extensive test coverage for emergency proposals across multiple test files:
- Emergency proposal lifecycle tests in
abci_test.go
covering:- Emergency start time validation
- Emergency tally intervals
- Emergency state transitions
- Emergency proposal queries and storage in
custom_grpc_query_test.go
- Emergency deposit handling in
deposit_test.go
- Emergency proposal creation and validation in
proposal_test.go
The test coverage appears thorough, testing both the happy path and edge cases for emergency proposal functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for emergency proposal scenarios
# Look for emergency proposal test cases
echo "Searching for emergency proposal test cases..."
rg -l "TestEmergencyProposal"
# Look for expedited proposal handling in tests
echo "Searching for expedited proposal handling..."
rg "expedited.*true" --type go
# Check for emergency-related constants usage
echo "Checking usage of emergency-related constants..."
rg "emergencyMinDeposit|emergencyTallyInterval" --type go
Length of output: 3623
Script:
#!/bin/bash
# Let's search for emergency proposal related test cases more comprehensively
# Look for test files containing emergency/expedited proposal tests
echo "Searching test files for emergency/expedited proposals..."
rg -l "emergency.*proposal|expedited.*proposal" --type go --glob "*_test.go"
# Look for test functions and test setup related to emergency proposals
echo "Searching for test functions..."
ast-grep --pattern 'func Test$_($_) {
$$$
emergency$_
$$$
}'
# Check for emergency proposal setup in test utilities
echo "Searching test utilities..."
rg "emergency.*proposal.*=|expedited.*=.*true" -A 3 --type go --glob "*_test.go"
Length of output: 493
Script:
#!/bin/bash
# Let's try a different approach to find emergency/expedited proposal tests
# Search for proposal type definitions and usages in tests
echo "Searching for proposal type definitions in tests..."
rg "ProposalType.*=.*Emergency|IsExpedited.*=.*true" --type go -B 2 -A 2
# Look for emergency/expedited related test setup
echo "Searching for test setup with emergency/expedited flags..."
rg "NewProposal.*\(" -A 5 --type go --glob "*_test.go"
# Check for emergency/expedited related test assertions
echo "Searching for emergency/expedited related assertions..."
rg "assert.*Emergency|assert.*Expedited|require.*Emergency|require.*Expedited" --type go --glob "*_test.go"
Length of output: 6124
x/gov/abci_test.go (1)
263-287
: LGTM! Well-structured test setup with comprehensive assertions.
The test setup thoroughly validates the initial state of the emergency proposal, including all critical timestamps and status fields.
x/gov/abci.go (1)
188-189
: Logic for scheduling the next tally is correctly implemented
The condition appropriately checks if the quorum has not been reached and the proposal's voting period has not expired, ensuring that emergency proposals are re-tallied until the voting period ends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
x/gov/types/params_test.go (1)
33-34
: LGTM! Consider adding a descriptive comment.The test case correctly validates that
EmergencyTallyInterval
cannot be equal toVotingPeriod
, which aligns with the requirement that emergency proposals should have a distinct tally interval.Consider adding a comment to explain why this validation is necessary:
+ // EmergencyTallyInterval must be less than VotingPeriod to ensure emergency proposals + // are tallied before the voting period ends params.EmergencyTallyInterval = params.VotingPeriod require.Error(t, params.Validate(ac))x/gov/types/params.go (1)
Line range hint
217-219
: Enhance error message while maintaining correct validation.The validation logic correctly ensures that emergency proposals require at least as much deposit as expedited ones. However, the error message could be more informative.
Consider making the error message more specific:
- return fmt.Errorf("emergency minimum deposit must be greater than or equal to minimum deposit") + return fmt.Errorf("emergency minimum deposit %s must be greater than or equal to expedited minimum deposit %s", minEmergencyDeposit, p.ExpeditedMinDeposit)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
x/gov/abci.go
(1 hunks)x/gov/types/params.go
(1 hunks)x/gov/types/params_test.go
(1 hunks)
🔇 Additional comments (3)
x/gov/types/params.go (2)
213-215
: LGTM! Critical validation for emergency proposal timing.
This validation ensures that emergency tallying always occurs before the voting period ends, which is essential for the proper functioning of emergency proposals. The error message is clear and the comparison is implemented correctly.
Line range hint 213-219
: Verify integration with EndBlocker changes.
These parameter validations work in conjunction with the EndBlocker changes mentioned in the PR summary. Let's verify the integration to ensure consistent behavior.
✅ Verification successful
Integration with EndBlocker is properly implemented
Based on the code analysis, the parameter validations are correctly integrated with the EndBlocker implementation:
- The EndBlocker respects the EmergencyTallyInterval by scheduling the next tally time within the voting period bounds
- Emergency proposals are properly tracked through EmergencyProposalsQueue and have dedicated activation logic
- The implementation ensures that emergency tallying cannot occur after the voting end time, consistent with the parameter validation that requires EmergencyTallyInterval to be less than VotingPeriod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that EndBlocker respects these parameter constraints
# Search for EndBlocker implementation that uses these parameters
echo "Searching for EndBlocker implementation..."
rg -l "EmergencyTallyInterval|VotingPeriod" --type go
# Look for related test cases
echo "Searching for related test cases..."
rg -l "TestEndBlocker.*Emergency" --type go
Length of output: 838
Script:
#!/bin/bash
# Let's examine the EndBlocker implementation and related emergency proposal handling
# Check EndBlocker implementation in abci.go
echo "=== EndBlocker Implementation ==="
rg -A 10 "func.*EndBlocker" x/gov/abci.go
# Check emergency proposal handling in EndBlocker
echo -e "\n=== Emergency Proposal Handling ==="
rg -B 2 -A 5 "EmergencyTallyInterval" x/gov/abci.go
# Check test cases for emergency proposals
echo -e "\n=== Emergency Proposal Test Cases ==="
rg -B 2 -A 5 "EmergencyTallyInterval" x/gov/abci_test.go
Length of output: 1246
Script:
#!/bin/bash
# Let's check the proposal handling logic more thoroughly
# Check how emergency proposals are handled in proposal.go
echo "=== Emergency Proposal Implementation ==="
rg -B 2 -A 10 "EmergencyProposal" x/gov/keeper/proposal.go
# Check tally implementation
echo -e "\n=== Tally Implementation ==="
rg -B 2 -A 10 "func.*Tally" x/gov/keeper/tally_test.go
# Check if there are any emergency-specific test cases
echo -e "\n=== Emergency Test Cases ==="
rg -C 5 "emergency.*proposal" x/gov/keeper/tally_test.go x/gov/keeper/proposal_test.go
Length of output: 2175
x/gov/abci.go (1)
192-196
: LGTM! Well-structured emergency proposal handling.
The implementation correctly:
- Caps the next tally time at voting end time
- Maintains proper state updates
- Follows existing patterns for queue management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just need to change a few small things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/gov/abci.go (1)
188-189
: Enhance comment clarity for scheduling conditions.The comment should explicitly mention both conditions that determine when to schedule the next tally.
-// schedule the next tally only if quorum is not reached and voting period is not over +// schedule the next tally only if: +// 1. quorum has not been reached +// 2. current block time hasn't reached voting end time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/gov/abci.go
(1 hunks)x/gov/types/params.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/gov/types/params.go
🔇 Additional comments (2)
x/gov/abci.go (2)
192-195
: LGTM! Correctly handles tally scheduling at voting period end.
The logic properly ensures that emergency proposals are tallied at the voting end time by adjusting the next tally time if it would exceed the voting period. This fixes the issue of emergency proposals not being tallied at the normal voting end time.
188-195
: Verify emergency proposal handling across the codebase.
Let's ensure this change is consistent with other emergency proposal handling code.
✅ Verification successful
Let me analyze the code further to verify the emergency proposal handling implementation.
Let me analyze the emergency proposal handling implementation further.
Based on the comprehensive analysis of the codebase, I can now provide a final response.
Emergency proposal tally scheduling implementation is correct
The implementation in the code snippet is consistent with the rest of the codebase. Here's why:
- The emergency tally interval is properly validated in params.go to ensure it's positive and less than the voting period
- The next tally time check against voting end time is a necessary safeguard to prevent scheduling tallies beyond the voting period
- The implementation aligns with other emergency proposal handling code in the keeper and abci layers
The logic ensures that:
- Emergency proposals are tallied at regular intervals (EmergencyTallyInterval)
- The final tally happens no later than the voting end time
- Status transitions (Passed/Rejected) are handled consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other emergency proposal handling code that might need similar adjustments
# Search for emergency proposal related code
echo "Searching for emergency proposal related code..."
rg -l "Emergency.*Proposal" --type go
# Search for voting period end time checks
echo "Searching for voting end time checks..."
rg "VotingEndTime" --type go -A 3 -B 3
# Search for tally scheduling logic
echo "Searching for tally scheduling patterns..."
rg "EmergencyTallyInterval|EmergencyNextTallyTime" --type go
Length of output: 25843
Script:
#!/bin/bash
# Check for emergency proposal handling logic in keeper and tests
echo "Checking emergency proposal handling in keeper..."
rg "EmergencyStartTime|EmergencyProposal" --type go -A 3 -B 3
echo "Checking proposal status transitions..."
ast-grep --pattern 'func $_(ctx $_, proposal $_, $$$) {
$$$
proposal.Status = $status
$$$
}'
Length of output: 112072
Script:
#!/bin/bash
# Check for emergency proposal tally and voting period handling
echo "Checking emergency proposal tally and voting period handling..."
rg "EmergencyTallyInterval|VotingEndTime|VotingPeriod" --type go -A 2 -B 2 x/gov/types/params.go
echo "Checking emergency proposal status transitions..."
rg "StatusVotingPeriod|StatusPassed|StatusRejected" --type go -A 2 -B 2 x/gov/abci.go
Length of output: 3599
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
emergency proposals are not tallied in normal voting end time, but it should be tallied.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...