这是indexloc提供的服务,不要输入任何密码
Skip to content

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

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

beer-1
Copy link
Member

@beer-1 beer-1 commented Nov 10, 2024

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

@beer-1 beer-1 self-assigned this Nov 10, 2024
@beer-1 beer-1 requested a review from a team as a code owner November 10, 2024 16:41
Copy link

coderabbitai bot commented Nov 10, 2024

📝 Walkthrough

Walkthrough

The pull request introduces significant modifications to the governance proposal handling in the gov package, specifically within the EndBlocker function. Key updates include enhanced checks for proposal statuses, improved error handling for unsupported proposals, and modifications to the tally result processing. Additionally, a new test function is added to verify the behavior of emergency proposals when the voting period expires. Changes in test functions streamline their signatures, reflecting a shift in testing practices without altering their core functionality.

Changes

File Change Summary
x/gov/abci.go - Modified EndBlocker to check voting end time before rescheduling emergency proposals.
- Refined error handling for unsupported proposals.
- Updated handleTallyResult for expedited proposals.
- Function signatures updated for EndBlocker, handleTallyResult, safeExecuteHandler, and failUnsupportedProposal.
x/gov/abci_test.go - Added TestEmergencyProposal_Rejected_VotingPeriodOver to test emergency proposal behavior after voting period ends.
x/gov/common_test.go - Updated signatures of createDefaultApp, createDepositMsg, and createVoteMsg to remove unused testing parameter.
x/gov/types/params.go - Enhanced Validate method to ensure EmergencyTallyInterval is less than VotingPeriod and updated checks for emergencyMinDeposit.
x/gov/types/params_test.go - Added validation case for EmergencyTallyInterval to ensure it cannot equal VotingPeriod.

Possibly related PRs

🐇 In the land of code where proposals bloom,
Changes were made to dispel the gloom.
With checks and balances in place,
Voting now runs at a smoother pace.
Errors are caught, no longer a fright,
Governance shines, oh what a delight! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

github-actions bot commented Nov 10, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

@beer-1 beer-1 changed the title fix: emergency poll still need to be tallied at normal schedule fix: emergency poll should be tallied at normal schedule Nov 10, 2024
@beer-1 beer-1 changed the title fix: emergency poll should be tallied at normal schedule fix: emergency poll should be finished if voting period is over Nov 10, 2024
Copy link

codecov bot commented Nov 10, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.04%. Comparing base (9141977) to head (3d7562e).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
x/gov/abci.go 0.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
x/gov/types/params.go 64.57% <100.00%> (+0.61%) ⬆️
x/gov/abci.go 27.95% <0.00%> (+0.50%) ⬆️

Copy link

@coderabbitai coderabbitai bot left a 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:

  1. Adding explicit assertions for the vote count
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between f7a3f0e and e77f700.

⛔ 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:

  1. The emergencyMinDeposit amount (100,000) is appropriately higher than regular minDeposit (10,000)
  2. 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.

Copy link

@coderabbitai coderabbitai bot left a 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 to VotingPeriod, 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

📥 Commits

Reviewing files that changed from the base of the PR and between e77f700 and 69ec9e5.

📒 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

Copy link
Contributor

@sh-cha sh-cha left a 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.

@beer-1 beer-1 requested a review from sh-cha November 12, 2024 09:28
Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 69ec9e5 and 3d7562e.

📒 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:

  1. Emergency proposals are tallied at regular intervals (EmergencyTallyInterval)
  2. The final tally happens no later than the voting end time
  3. 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

Copy link
Contributor

@sh-cha sh-cha left a comment

Choose a reason for hiding this comment

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

LGTM

@beer-1 beer-1 merged commit 329e6e3 into main Nov 12, 2024
10 of 11 checks passed
@beer-1 beer-1 deleted the fix/gov-emergency-proposal branch November 12, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants