refactor(tests/integrations): Port gov integration tests with server v2 setup #22850
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve enhancements to service management within the blockchain application, particularly through the introduction of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| func (e *eventService) EventManager(ctx context.Context) event.Manager { | ||
| iCtx, ok := ctx.Value(contextKey).(*integrationContext) | ||
| if !ok { | ||
| panic("context is not an integration context") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning test
sontrinh16
changed the title
sontrinh16
requested review from
a team,
julienrbrt and
hieuvubk
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (13)
tests/integration/v2/gov/common_test.go (2)
69-82: Simplify 'SortAddresses' function using 'sort.Slice'The
SortAddressesfunction can be simplified by usingsort.Sliceinstead of custom sorting logic, improving readability and maintainability.Apply this diff to refactor the function:
-func SortAddresses(addrs []sdk.AccAddress) { - byteAddrs := make([][]byte, len(addrs)) - for i, addr := range addrs { - byteAddrs[i] = addr.Bytes() - } - SortByteArrays(byteAddrs) - for i, byteAddr := range byteAddrs { - addrs[i] = byteAddr - } -} - -// Implement sort.Interface for sortByteArrays. -type sortByteArrays [][]byte - -func (b sortByteArrays) Len() int { - return len(b) -} - -func (b sortByteArrays) Less(i, j int) bool { - return bytes.Compare(b[i], b[j]) == -1 -} - -func (b sortByteArrays) Swap(i, j int) { - b[i], b[j] = b[j], b[i] -} - -// SortByteArrays sorts the provided byte array. -func SortByteArrays(src [][]byte) [][]byte { - sort.Sort(sortByteArrays(src)) - return src -} +import "sort" + +func SortAddresses(addrs []sdk.AccAddress) { + sort.Slice(addrs, func(i, j int) bool { + return bytes.Compare(addrs[i], addrs[j]) == -1 + }) +}
91-102: Simplify the 'Less' method in 'sortByteArrays'The
Lessmethod insortByteArrayscan be simplified by directly returning the comparison result, enhancing clarity.Apply this diff:
func (b sortByteArrays) Less(i, j int) bool { - switch bytes.Compare(b[i], b[j]) { - case -1: - return true - case 0, 1: - return false - default: - log.Panic("unreachable code in bytes.Compare") - return false - } + return bytes.Compare(b[i], b[j]) == -1 }tests/integration/v2/services.go (1)
163-163: Format the code with 'gofumpt -extra'The file is not formatted according to
gofumptwith the-extraflag. Please format the code to ensure consistency and adherence to style guidelines.🧰 Tools
🪛 golangci-lint (1.62.2)
163-163: File is not
gofumpt-ed with-extra(gofumpt)
tests/integration/v2/gov/keeper/grpc_test.go (2)
48-48: Extract magic numbers into named constantsThe values
5(validator power) and1000000(multiplication factor) should be defined as named constants at the package level for better maintainability and clarity.+const ( + defaultValidatorPower = 5 + powerMultiplier = 1000000 +) + // Use in test: -Yes: math.NewInt(3 * 5 * 1000000), +Yes: math.NewInt(3 * defaultValidatorPower * powerMultiplier),
65-65: Error handling in vote delegationThe errors from vote delegation operations are being ignored. While this might be acceptable in a test context, it's better to assert that no errors occurred to ensure test validity.
-_, _ = f.stakingKeeper.Delegate(...) +delegationRes, err := f.stakingKeeper.Delegate(...) +assert.NilError(t, err)tests/integration/v2/gov/keeper/common_test.go (1)
38-72: Improve validator creation utilitySeveral improvements needed:
- Extract magic numbers into constants
- Handle delegation errors
- Add documentation for the function parameters and return values
+const ( + numValidators = 5 + initBalance = 30000000 +) + +// createValidators creates test validators with the specified powers. +// It returns the validator addresses and their corresponding operator addresses. +// powers: slice of validator powers to assign to each validator func createValidators(t *testing.T, f *fixture, powers []int64) ([]sdk.AccAddress, []sdk.ValAddress) { t.Helper() - addrs := simtestutil.AddTestAddrsIncremental(f.bankKeeper, f.stakingKeeper, f.ctx, 5, math.NewInt(30000000)) + addrs := simtestutil.AddTestAddrsIncremental(f.bankKeeper, f.stakingKeeper, f.ctx, numValidators, math.NewInt(initBalance)) // ... rest of the function // Handle delegation errors - _, _ = f.stakingKeeper.Delegate(...) + _, err = f.stakingKeeper.Delegate(...) + assert.NilError(t, err)tests/integration/v2/gov/keeper/fixture_test.go (3)
107-109: Implement query router service or add TODO commentThe query router service implementation is empty. Either implement it or add a TODO comment explaining why it's empty.
func (f *fixture) registerQueryRouterService(router *integration.RouterService) { - + // TODO: Implement query router service when query handling is needed }
94-102: Enhance error handling in message handlerThe message handler could provide more detailed error information and type assertions.
govSubmitProposalHandler := func(ctx context.Context, req transaction.Msg) (transaction.Msg, error) { msg, ok := req.(*v1.MsgExecLegacyContent) if !ok { - return nil, integration.ErrInvalidMsgType + return nil, fmt.Errorf("%w: expected %T, got %T", + integration.ErrInvalidMsgType, + (*v1.MsgExecLegacyContent)(nil), + req) } msgServer := govkeeper.NewMsgServerImpl(f.govKeeper) resp, err := msgServer.ExecLegacyContent(ctx, msg) + if err != nil { + return nil, fmt.Errorf("failed to execute legacy content: %w", err) + } return resp, err }
32-43: Consider adding cleanup method to fixtureThe fixture struct should implement cleanup to ensure proper test isolation.
type fixture struct { ctx context.Context app *integration.App // ... other fields + + t *testing.T // for cleanup helper } + +// cleanup performs necessary cleanup after tests +func (f *fixture) cleanup() { + f.t.Helper() + // Add necessary cleanup logic here + // For example: clear any stored proposals, reset validator states, etc. +}tests/integration/v2/gov/genesis_test.go (2)
63-104: Consider adding validation for exported state.While the state export and import flow is correct, consider adding assertions to validate the structure and content of the exported state before importing it.
stateBytes, err := json.MarshalIndent(genesisState, "", " ") assert.NilError(t, err) + +// Validate exported state +var exportedState map[string]json.RawMessage +err = json.Unmarshal(stateBytes, &exportedState) +assert.NilError(t, err) +assert.Assert(t, len(exportedState[types.ModuleName]) > 0, "gov state should not be empty")
1-31: Consider organizing imports.The imports are mixed between standard library, external packages, and internal packages. Consider grouping them for better readability.
-import ( - "crypto/sha256" - "encoding/json" - "testing" - "time" - - "github.com/stretchr/testify/require" - "gotest.tools/v3/assert" - - "cosmossdk.io/core/header" +import ( + // Standard library + "crypto/sha256" + "encoding/json" + "testing" + "time" + + // External packages + "github.com/stretchr/testify/require" + "gotest.tools/v3/assert" + + // Cosmos SDK packages + "cosmossdk.io/core/header"tests/integration/v2/gov/keeper/tally_test.go (2)
1-15: Add package documentation for better maintainability.Consider adding a package-level documentation comment that describes the purpose and scope of these integration tests.
package keeper + +// Package keeper_test provides integration tests for the governance module's tallying functionality. +// It covers various voting scenarios including validator voting, delegation overrides, and special cases.
478-519: Consider adding edge case tests for comprehensive coverage.The test suite covers the main voting scenarios well, but consider adding the following edge cases:
- Test with extremely large voting power (near max uint)
- Test with minimum voting power (1)
- Test with maximum allowed number of validators
- Test with zero voting power validators
Example test case structure:
func TestTallyWithExtremeVotingPower(t *testing.T) { t.Parallel() f := initFixture(t) ctx := f.ctx // Test with max voting power maxPower := math.NewInt(^uint64(0)) addrs, valAddrs := createValidators(t, f, []int64{maxPower.Int64()}) // ... rest of the test }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
runtime/v2/module.go(5 hunks)tests/go.mod(1 hunks)tests/integration/v2/app.go(5 hunks)tests/integration/v2/genesis.go(1 hunks)tests/integration/v2/gov/abci_test.go(1 hunks)tests/integration/v2/gov/common_test.go(1 hunks)tests/integration/v2/gov/genesis_test.go(1 hunks)tests/integration/v2/gov/keeper/common_test.go(1 hunks)tests/integration/v2/gov/keeper/fixture_test.go(1 hunks)tests/integration/v2/gov/keeper/grpc_test.go(1 hunks)tests/integration/v2/gov/keeper/tally_test.go(1 hunks)tests/integration/v2/gov/module_test.go(1 hunks)tests/integration/v2/services.go(6 hunks)testutil/sims/address_helpers.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
tests/integration/v2/genesis.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
runtime/v2/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
testutil/sims/address_helpers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/gov/module_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/keeper/grpc_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/genesis_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/keeper/fixture_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/app.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/keeper/common_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/common_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/services.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/keeper/tally_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/gov/abci_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/v2/services.go
163-163: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (19)
tests/integration/v2/gov/common_test.go (1)
54-61: LGTM!
The mkTestLegacyContent function correctly creates a MsgExecLegacyContent message for testing purposes.
tests/integration/v2/gov/abci_test.go (6)
26-52: LGTM!
The test TestUnregisteredProposal_InactiveProposalFails is correctly implemented and verifies that inactive unregistered proposals fail as expected.
54-83: LGTM!
The test TestUnregisteredProposal_ActiveProposalFails appropriately validates that active unregistered proposals fail.
173-207: LGTM!
The test TestTickPassedDepositPeriod is well-implemented and checks the deposit period passing scenario correctly.
209-259: LGTM!
The test TestProposalDepositRefundFailEndBlocker effectively verifies the behavior when the proposal deposit refund fails during the end blocker.
412-470: LGTM!
The test TestEndBlockerProposalHandlerFailed correctly checks the scenario where a proposal handler fails during the end blocker.
645-660: LGTM!
The createValidators helper function is implemented correctly to set up validators for testing.
tests/integration/v2/gov/module_test.go (1)
17-23: LGTM!
The test TestItCreatesModuleAccountOnInitBlock correctly verifies the creation of the module account during the init block.
tests/integration/v2/genesis.go (1)
150-154: LGTM! Clean and idiomatic constructor implementation.
The constructor follows Go's best practices for struct initialization.
tests/integration/v2/gov/genesis_test.go (2)
33-62: LGTM! Well-structured test setup with clear assertions.
The test setup properly initializes the test environment and creates two proposals with different states, providing good coverage of different scenarios.
136-162: LGTM! Good error case coverage.
The test properly validates the handling of inconsistent state during genesis initialization. It includes both the error case and successful initialization with default state.
runtime/v2/module.go (2)
218-218: LGTM: Gas service integration looks correct
The gas service is properly integrated into the environment, maintaining consistency with other services.
Also applies to: 227-227
301-301: LGTM: Gas service initialization and supply
The gas service is correctly initialized using stf.NewGasMeterService() and supplied to the dependency injection system.
Also applies to: 311-311
tests/go.mod (1)
54-54:
Security: Consider updating gogo/protobuf version
The gogo/protobuf v1.3.2 version has known security vulnerabilities. Consider updating to a newer version if available.
Let's verify if there are any security advisories:
tests/integration/v2/app.go (4)
103-104: LGTM: Gas service field addition
The GasService field is properly added to the StartupConfig struct.
135-135: LGTM: Gas service initialization
The GasService is correctly initialized in DefaultStartUpConfig using stf.NewGasMeterService().
200-200: LGTM: Gas service dependency injection
The GasService is properly included in the dependency injection configuration.
349-349:
Non-deterministic behavior: Using time.Now()
Using time.Now() can lead to non-deterministic behavior in tests. Consider using a deterministic time source.
-iCtx.header.Time = time.Now()
+iCtx.header.Time = time.Unix(1234567890, 0) // Use a fixed timestamp for deterministic behaviortests/integration/v2/gov/keeper/tally_test.go (1)
17-39: Verify test isolation for parallel execution.
While using t.Parallel() is good for performance, ensure that the initFixture function creates completely isolated test environments to prevent potential race conditions between parallel tests.
Also applies to: 41-67
| res1, err := govMsgSvr.Deposit(ctx, newDepositMsg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res1) | ||
|
|
||
| newHeader = integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*params.ExpeditedVotingPeriod) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | ||
| require.Nil(t, err) | ||
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | ||
|
|
||
| if tc.expeditedPasses { | ||
| // Validator votes YES, letting the expedited proposal pass. | ||
| err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "metadata") | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| // Here the expedited proposal is converted to regular after expiry. | ||
| err = suite.GovKeeper.EndBlocker(ctx) | ||
| require.NoError(t, err) | ||
| if tc.expeditedPasses { | ||
| proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | ||
| require.Nil(t, err) | ||
|
|
||
| require.Equal(t, v1.StatusPassed, proposal.Status) | ||
|
|
||
| submitterEventualBalance := suite.BankKeeper.GetAllBalances(ctx, addrs[0]) | ||
| depositorEventualBalance := suite.BankKeeper.GetAllBalances(ctx, addrs[1]) | ||
|
|
||
| eventualModuleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | ||
|
|
||
| // Module account has refunded the deposit | ||
| require.Equal(t, initialModuleAccCoins, eventualModuleAccCoins) | ||
|
|
||
| require.Equal(t, submitterInitialBalance, submitterEventualBalance) | ||
| require.Equal(t, depositorInitialBalance, depositorEventualBalance) | ||
| return | ||
| } | ||
|
|
||
| // Expedited proposal should be converted to a regular proposal instead. | ||
| proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | ||
| require.Nil(t, err) | ||
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | ||
| require.False(t, proposal.ProposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED) | ||
| require.Equal(t, proposal.VotingStartTime.Add(*params.VotingPeriod), *proposal.VotingEndTime) | ||
|
|
||
| // We also want to make sure that the deposit is not refunded yet and is still present in the module account | ||
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | ||
| require.NotNil(t, macc) | ||
| intermediateModuleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | ||
| require.NotEqual(t, initialModuleAccCoins, intermediateModuleAccCoins) | ||
|
|
||
| // Submit proposal deposit + 1 extra top up deposit | ||
| expectedIntermediateMofuleAccCoings := initialModuleAccCoins.Add(proposalCoins...).Add(proposalCoins...) | ||
| require.Equal(t, expectedIntermediateMofuleAccCoings, intermediateModuleAccCoins) | ||
|
|
||
| // block header time at the voting period | ||
| newHeader = integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| if tc.regularEventuallyPassing { | ||
| // Validator votes YES, letting the converted regular proposal pass. | ||
| err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "metadata") | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| // Here we validate the converted regular proposal | ||
| err = suite.GovKeeper.EndBlocker(ctx) | ||
| require.NoError(t, err) | ||
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | ||
| require.NotNil(t, macc) | ||
| eventualModuleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | ||
|
|
||
| submitterEventualBalance := suite.BankKeeper.GetAllBalances(ctx, addrs[0]) | ||
| depositorEventualBalance := suite.BankKeeper.GetAllBalances(ctx, addrs[1]) | ||
|
|
||
| proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | ||
| require.Nil(t, err) | ||
|
|
||
| if tc.regularEventuallyPassing { | ||
| // Module account has refunded the deposit | ||
| require.Equal(t, initialModuleAccCoins, eventualModuleAccCoins) | ||
| require.Equal(t, submitterInitialBalance, submitterEventualBalance) | ||
| require.Equal(t, depositorInitialBalance, depositorEventualBalance) | ||
|
|
||
| require.Equal(t, v1.StatusPassed, proposal.Status) | ||
| return | ||
| } | ||
|
|
||
| // Not enough votes - module account has returned the deposit | ||
| require.Equal(t, initialModuleAccCoins, eventualModuleAccCoins) | ||
| require.Equal(t, submitterInitialBalance, submitterEventualBalance) | ||
| require.Equal(t, depositorInitialBalance, depositorEventualBalance) | ||
|
|
||
| require.Equal(t, v1.StatusRejected, proposal.Status) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the error returned by Params.Get
In TestExpeditedProposal_PassAndConversionToRegular, the error from suite.GovKeeper.Params.Get(ctx) is ignored. Please handle the error accordingly.
Apply this diff:
- params, err := suite.GovKeeper.Params.Get(ctx)
+ params, err := suite.GovKeeper.Params.Get(ctx)
+ require.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
| func TestTickExpiredDepositPeriod(t *testing.T) { | ||
| suite := createTestSuite(t, integration.Genesis_COMMIT) | ||
| ctx := suite.ctx | ||
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) | ||
|
|
||
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | ||
|
|
||
| newProposalMsg, err := v1.NewMsgSubmitProposal( | ||
| []sdk.Msg{mkTestLegacyContent(t)}, | ||
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | ||
| addrs[0].String(), | ||
| "", | ||
| "Proposal", | ||
| "description of proposal", | ||
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res) | ||
|
|
||
| newHeader := integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(time.Duration(1) * time.Second) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| params, _ := suite.GovKeeper.Params.Get(ctx) | ||
| newHeader = integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| err = suite.GovKeeper.EndBlocker(ctx) | ||
| require.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Handle the error returned by Params.Get
In TestTickExpiredDepositPeriod, the error returned by suite.GovKeeper.Params.Get(ctx) is ignored. To ensure robustness, handle the error to catch any potential issues.
Apply this diff:
- params, _ := suite.GovKeeper.Params.Get(ctx)
+ params, err := suite.GovKeeper.Params.Get(ctx)
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestTickExpiredDepositPeriod(t *testing.T) { | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| newProposalMsg, err := v1.NewMsgSubmitProposal( | |
| []sdk.Msg{mkTestLegacyContent(t)}, | |
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | |
| addrs[0].String(), | |
| "", | |
| "Proposal", | |
| "description of proposal", | |
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| ) | |
| require.NoError(t, err) | |
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(1) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| params, _ := suite.GovKeeper.Params.Get(ctx) | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| err = suite.GovKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| } | |
| func TestTickExpiredDepositPeriod(t *testing.T) { | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| newProposalMsg, err := v1.NewMsgSubmitProposal( | |
| []sdk.Msg{mkTestLegacyContent(t)}, | |
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | |
| addrs[0].String(), | |
| "", | |
| "Proposal", | |
| "description of proposal", | |
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| ) | |
| require.NoError(t, err) | |
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(1) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| params, err := suite.GovKeeper.Params.Get(ctx) | |
| require.NoError(t, err) | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| err = suite.GovKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| } |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | ||
| ctx := suite.ctx | ||
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) | ||
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | ||
|
|
||
| newProposalMsg, err := v1.NewMsgSubmitProposal( | ||
| []sdk.Msg{mkTestLegacyContent(t)}, | ||
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | ||
| addrs[0].String(), | ||
| "", | ||
| "Proposal", | ||
| "description of proposal", | ||
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res) | ||
|
|
||
| newHeader := integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(time.Duration(2) * time.Second) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| newProposalMsg2, err := v1.NewMsgSubmitProposal( | ||
| []sdk.Msg{mkTestLegacyContent(t)}, | ||
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | ||
| addrs[0].String(), | ||
| "", | ||
| "Proposal", | ||
| "description of proposal", | ||
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err = govMsgSvr.SubmitProposal(ctx, newProposalMsg2) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res) | ||
|
|
||
| newHeader = integration.HeaderInfoFromContext(ctx) | ||
| params, _ := suite.GovKeeper.Params.Get(ctx) | ||
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(time.Duration(-1) * time.Second) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| require.NoError(t, suite.GovKeeper.EndBlocker(ctx)) | ||
|
|
||
| newHeader = integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(time.Duration(5) * time.Second) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
| require.NoError(t, suite.GovKeeper.EndBlocker(ctx)) | ||
| } |
There was a problem hiding this comment.
Handle the error returned by Params.Get
In TestTickMultipleExpiredDepositPeriod, the error from suite.GovKeeper.Params.Get(ctx) is ignored. It's important to handle this error.
Apply this diff:
- params, _ := suite.GovKeeper.Params.Get(ctx)
+ params, err := suite.GovKeeper.Params.Get(ctx)
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| newProposalMsg, err := v1.NewMsgSubmitProposal( | |
| []sdk.Msg{mkTestLegacyContent(t)}, | |
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | |
| addrs[0].String(), | |
| "", | |
| "Proposal", | |
| "description of proposal", | |
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| ) | |
| require.NoError(t, err) | |
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(2) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| newProposalMsg2, err := v1.NewMsgSubmitProposal( | |
| []sdk.Msg{mkTestLegacyContent(t)}, | |
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | |
| addrs[0].String(), | |
| "", | |
| "Proposal", | |
| "description of proposal", | |
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| ) | |
| require.NoError(t, err) | |
| res, err = govMsgSvr.SubmitProposal(ctx, newProposalMsg2) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| params, _ := suite.GovKeeper.Params.Get(ctx) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(time.Duration(-1) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| require.NoError(t, suite.GovKeeper.EndBlocker(ctx)) | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(5) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| require.NoError(t, suite.GovKeeper.EndBlocker(ctx)) | |
| } | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| newProposalMsg, err := v1.NewMsgSubmitProposal( | |
| []sdk.Msg{mkTestLegacyContent(t)}, | |
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | |
| addrs[0].String(), | |
| "", | |
| "Proposal", | |
| "description of proposal", | |
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| ) | |
| require.NoError(t, err) | |
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(2) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| newProposalMsg2, err := v1.NewMsgSubmitProposal( | |
| []sdk.Msg{mkTestLegacyContent(t)}, | |
| sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 100000)}, | |
| addrs[0].String(), | |
| "", | |
| "Proposal", | |
| "description of proposal", | |
| v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| ) | |
| require.NoError(t, err) | |
| res, err = govMsgSvr.SubmitProposal(ctx, newProposalMsg2) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| params, err := suite.GovKeeper.Params.Get(ctx) | |
| require.NoError(t, err) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(time.Duration(-1) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| require.NoError(t, suite.GovKeeper.EndBlocker(ctx)) | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(5) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| require.NoError(t, suite.GovKeeper.EndBlocker(ctx)) | |
| } |
| func TestTickPassedVotingPeriod(t *testing.T) { | ||
| testcases := []struct { | ||
| name string | ||
| proposalType v1.ProposalType | ||
| }{ | ||
| { | ||
| name: "regular - deleted", | ||
| }, | ||
| { | ||
| name: "expedited - converted to regular", | ||
| proposalType: v1.ProposalType_PROPOSAL_TYPE_EXPEDITED, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testcases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| suite := createTestSuite(t, integration.Genesis_COMMIT) | ||
| ctx := suite.ctx | ||
| depositMultiplier := getDepositMultiplier(tc.proposalType) | ||
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens.Mul(math.NewInt(depositMultiplier))) | ||
|
|
||
| SortAddresses(addrs) | ||
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | ||
|
|
||
| proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 5*depositMultiplier))} | ||
| newProposalMsg, err := v1.NewMsgSubmitProposal([]sdk.Msg{mkTestLegacyContent(t)}, proposalCoins, addrs[0].String(), "", "Proposal", "description of proposal", tc.proposalType) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res) | ||
|
|
||
| proposalID := res.ProposalId | ||
|
|
||
| newHeader := integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(time.Duration(1) * time.Second) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| addr1Str, err := suite.AuthKeeper.AddressCodec().BytesToString(addrs[1]) | ||
| require.NoError(t, err) | ||
| newDepositMsg := v1.NewMsgDeposit(addr1Str, proposalID, proposalCoins) | ||
|
|
||
| res1, err := govMsgSvr.Deposit(ctx, newDepositMsg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res1) | ||
|
|
||
| params, _ := suite.GovKeeper.Params.Get(ctx) | ||
| votingPeriod := params.VotingPeriod | ||
| if tc.proposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { | ||
| votingPeriod = params.ExpeditedVotingPeriod | ||
| } | ||
|
|
||
| newHeader = integration.HeaderInfoFromContext(ctx) | ||
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*votingPeriod) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | ||
| require.NoError(t, err) | ||
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | ||
|
|
||
| err = suite.GovKeeper.EndBlocker(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| if tc.proposalType != v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { | ||
| return | ||
| } | ||
|
|
||
| // If expedited, it should be converted to a regular proposal instead. | ||
| proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | ||
| require.Nil(t, err) | ||
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | ||
| require.False(t, proposal.ProposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED) | ||
| require.Equal(t, proposal.VotingStartTime.Add(*params.VotingPeriod), *proposal.VotingEndTime) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the error returned by Params.Get
In TestTickPassedVotingPeriod, the error from suite.GovKeeper.Params.Get(ctx) is ignored. Please handle the error to prevent unnoticed failures.
Apply this diff:
- params, _ := suite.GovKeeper.Params.Get(ctx)
+ params, err := suite.GovKeeper.Params.Get(ctx)
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestTickPassedVotingPeriod(t *testing.T) { | |
| testcases := []struct { | |
| name string | |
| proposalType v1.ProposalType | |
| }{ | |
| { | |
| name: "regular - deleted", | |
| }, | |
| { | |
| name: "expedited - converted to regular", | |
| proposalType: v1.ProposalType_PROPOSAL_TYPE_EXPEDITED, | |
| }, | |
| } | |
| for _, tc := range testcases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| depositMultiplier := getDepositMultiplier(tc.proposalType) | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens.Mul(math.NewInt(depositMultiplier))) | |
| SortAddresses(addrs) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 5*depositMultiplier))} | |
| newProposalMsg, err := v1.NewMsgSubmitProposal([]sdk.Msg{mkTestLegacyContent(t)}, proposalCoins, addrs[0].String(), "", "Proposal", "description of proposal", tc.proposalType) | |
| require.NoError(t, err) | |
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| proposalID := res.ProposalId | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(1) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| addr1Str, err := suite.AuthKeeper.AddressCodec().BytesToString(addrs[1]) | |
| require.NoError(t, err) | |
| newDepositMsg := v1.NewMsgDeposit(addr1Str, proposalID, proposalCoins) | |
| res1, err := govMsgSvr.Deposit(ctx, newDepositMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res1) | |
| params, _ := suite.GovKeeper.Params.Get(ctx) | |
| votingPeriod := params.VotingPeriod | |
| if tc.proposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { | |
| votingPeriod = params.ExpeditedVotingPeriod | |
| } | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*votingPeriod) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | |
| require.NoError(t, err) | |
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | |
| err = suite.GovKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| if tc.proposalType != v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { | |
| return | |
| } | |
| // If expedited, it should be converted to a regular proposal instead. | |
| proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | |
| require.Nil(t, err) | |
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | |
| require.False(t, proposal.ProposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED) | |
| require.Equal(t, proposal.VotingStartTime.Add(*params.VotingPeriod), *proposal.VotingEndTime) | |
| }) | |
| } | |
| } | |
| func TestTickPassedVotingPeriod(t *testing.T) { | |
| testcases := []struct { | |
| name string | |
| proposalType v1.ProposalType | |
| }{ | |
| { | |
| name: "regular - deleted", | |
| }, | |
| { | |
| name: "expedited - converted to regular", | |
| proposalType: v1.ProposalType_PROPOSAL_TYPE_EXPEDITED, | |
| }, | |
| } | |
| for _, tc := range testcases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| depositMultiplier := getDepositMultiplier(tc.proposalType) | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens.Mul(math.NewInt(depositMultiplier))) | |
| SortAddresses(addrs) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 5*depositMultiplier))} | |
| newProposalMsg, err := v1.NewMsgSubmitProposal([]sdk.Msg{mkTestLegacyContent(t)}, proposalCoins, addrs[0].String(), "", "Proposal", "description of proposal", tc.proposalType) | |
| require.NoError(t, err) | |
| res, err := govMsgSvr.SubmitProposal(ctx, newProposalMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| proposalID := res.ProposalId | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(time.Duration(1) * time.Second) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| addr1Str, err := suite.AuthKeeper.AddressCodec().BytesToString(addrs[1]) | |
| require.NoError(t, err) | |
| newDepositMsg := v1.NewMsgDeposit(addr1Str, proposalID, proposalCoins) | |
| res1, err := govMsgSvr.Deposit(ctx, newDepositMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res1) | |
| params, err := suite.GovKeeper.Params.Get(ctx) | |
| require.NoError(t, err) | |
| votingPeriod := params.VotingPeriod | |
| if tc.proposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { | |
| votingPeriod = params.ExpeditedVotingPeriod | |
| } | |
| newHeader = integration.HeaderInfoFromContext(ctx) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*votingPeriod) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| proposal, err := suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | |
| require.NoError(t, err) | |
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | |
| err = suite.GovKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| if tc.proposalType != v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { | |
| return | |
| } | |
| // If expedited, it should be converted to a regular proposal instead. | |
| proposal, err = suite.GovKeeper.Proposals.Get(ctx, res.ProposalId) | |
| require.Nil(t, err) | |
| require.Equal(t, v1.StatusVotingPeriod, proposal.Status) | |
| require.False(t, proposal.ProposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED) | |
| require.Equal(t, proposal.VotingStartTime.Add(*params.VotingPeriod), *proposal.VotingEndTime) | |
| }) | |
| } | |
| } |
| func TestProposalPassedEndblocker(t *testing.T) { | ||
| testcases := []struct { | ||
| name string | ||
| proposalType v1.ProposalType | ||
| }{ | ||
| { | ||
| name: "regular", | ||
| proposalType: v1.ProposalType_PROPOSAL_TYPE_STANDARD, | ||
| }, | ||
| { | ||
| name: "expedited", | ||
| proposalType: v1.ProposalType_PROPOSAL_TYPE_EXPEDITED, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testcases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| suite := createTestSuite(t, integration.Genesis_COMMIT) | ||
| ctx := suite.ctx | ||
| depositMultiplier := getDepositMultiplier(tc.proposalType) | ||
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens.Mul(math.NewInt(depositMultiplier))) | ||
|
|
||
| SortAddresses(addrs) | ||
|
|
||
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | ||
| stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper) | ||
| valAddr := sdk.ValAddress(addrs[0]) | ||
| proposer := addrs[0] | ||
| acc := suite.AuthKeeper.NewAccountWithAddress(ctx, addrs[0]) | ||
| suite.AuthKeeper.SetAccount(ctx, acc) | ||
|
|
||
| createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10}) | ||
| _, err := suite.StakingKeeper.EndBlocker(ctx) | ||
| require.NoError(t, err) | ||
| macc := suite.GovKeeper.GetGovernanceAccount(ctx) | ||
| require.NotNil(t, macc) | ||
| initialModuleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | ||
|
|
||
| proposal, err := suite.GovKeeper.SubmitProposal(ctx, []sdk.Msg{mkTestLegacyContent(t)}, "", "title", "summary", proposer, tc.proposalType) | ||
| require.NoError(t, err) | ||
|
|
||
| proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 10*depositMultiplier))} | ||
| addr0Str, err := suite.AuthKeeper.AddressCodec().BytesToString(addrs[0]) | ||
| require.NoError(t, err) | ||
| newDepositMsg := v1.NewMsgDeposit(addr0Str, proposal.Id, proposalCoins) | ||
|
|
||
| res, err := govMsgSvr.Deposit(ctx, newDepositMsg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res) | ||
|
|
||
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | ||
| require.NotNil(t, macc) | ||
| moduleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | ||
|
|
||
| deposits := initialModuleAccCoins.Add(proposal.TotalDeposit...).Add(proposalCoins...) | ||
| require.True(t, moduleAccCoins.Equal(deposits)) | ||
|
|
||
| err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "") | ||
| require.NoError(t, err) | ||
|
|
||
| newHeader := integration.HeaderInfoFromContext(ctx) | ||
| params, _ := suite.GovKeeper.Params.Get(ctx) | ||
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod) | ||
| ctx = integration.SetHeaderInfo(ctx, newHeader) | ||
|
|
||
| err = suite.GovKeeper.EndBlocker(ctx) | ||
| require.NoError(t, err) | ||
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | ||
| require.NotNil(t, macc) | ||
| require.True(t, suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()).Equal(initialModuleAccCoins)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the error returned by Params.Get
In TestProposalPassedEndblocker, the error from suite.GovKeeper.Params.Get(ctx) is ignored. Ensure to handle this error for robustness.
Apply this diff:
- params, _ := suite.GovKeeper.Params.Get(ctx)
+ params, err := suite.GovKeeper.Params.Get(ctx)
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestProposalPassedEndblocker(t *testing.T) { | |
| testcases := []struct { | |
| name string | |
| proposalType v1.ProposalType | |
| }{ | |
| { | |
| name: "regular", | |
| proposalType: v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| }, | |
| { | |
| name: "expedited", | |
| proposalType: v1.ProposalType_PROPOSAL_TYPE_EXPEDITED, | |
| }, | |
| } | |
| for _, tc := range testcases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| depositMultiplier := getDepositMultiplier(tc.proposalType) | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens.Mul(math.NewInt(depositMultiplier))) | |
| SortAddresses(addrs) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper) | |
| valAddr := sdk.ValAddress(addrs[0]) | |
| proposer := addrs[0] | |
| acc := suite.AuthKeeper.NewAccountWithAddress(ctx, addrs[0]) | |
| suite.AuthKeeper.SetAccount(ctx, acc) | |
| createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10}) | |
| _, err := suite.StakingKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| macc := suite.GovKeeper.GetGovernanceAccount(ctx) | |
| require.NotNil(t, macc) | |
| initialModuleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | |
| proposal, err := suite.GovKeeper.SubmitProposal(ctx, []sdk.Msg{mkTestLegacyContent(t)}, "", "title", "summary", proposer, tc.proposalType) | |
| require.NoError(t, err) | |
| proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 10*depositMultiplier))} | |
| addr0Str, err := suite.AuthKeeper.AddressCodec().BytesToString(addrs[0]) | |
| require.NoError(t, err) | |
| newDepositMsg := v1.NewMsgDeposit(addr0Str, proposal.Id, proposalCoins) | |
| res, err := govMsgSvr.Deposit(ctx, newDepositMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | |
| require.NotNil(t, macc) | |
| moduleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | |
| deposits := initialModuleAccCoins.Add(proposal.TotalDeposit...).Add(proposalCoins...) | |
| require.True(t, moduleAccCoins.Equal(deposits)) | |
| err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "") | |
| require.NoError(t, err) | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| params, _ := suite.GovKeeper.Params.Get(ctx) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| err = suite.GovKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | |
| require.NotNil(t, macc) | |
| require.True(t, suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()).Equal(initialModuleAccCoins)) | |
| }) | |
| } | |
| } | |
| func TestProposalPassedEndblocker(t *testing.T) { | |
| testcases := []struct { | |
| name string | |
| proposalType v1.ProposalType | |
| }{ | |
| { | |
| name: "regular", | |
| proposalType: v1.ProposalType_PROPOSAL_TYPE_STANDARD, | |
| }, | |
| { | |
| name: "expedited", | |
| proposalType: v1.ProposalType_PROPOSAL_TYPE_EXPEDITED, | |
| }, | |
| } | |
| for _, tc := range testcases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| suite := createTestSuite(t, integration.Genesis_COMMIT) | |
| ctx := suite.ctx | |
| depositMultiplier := getDepositMultiplier(tc.proposalType) | |
| addrs := simtestutil.AddTestAddrs(suite.BankKeeper, suite.StakingKeeper, ctx, 10, valTokens.Mul(math.NewInt(depositMultiplier))) | |
| SortAddresses(addrs) | |
| govMsgSvr := keeper.NewMsgServerImpl(suite.GovKeeper) | |
| stakingMsgSvr := stakingkeeper.NewMsgServerImpl(suite.StakingKeeper) | |
| valAddr := sdk.ValAddress(addrs[0]) | |
| proposer := addrs[0] | |
| acc := suite.AuthKeeper.NewAccountWithAddress(ctx, addrs[0]) | |
| suite.AuthKeeper.SetAccount(ctx, acc) | |
| createValidators(t, stakingMsgSvr, ctx, []sdk.ValAddress{valAddr}, []int64{10}) | |
| _, err := suite.StakingKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| macc := suite.GovKeeper.GetGovernanceAccount(ctx) | |
| require.NotNil(t, macc) | |
| initialModuleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | |
| proposal, err := suite.GovKeeper.SubmitProposal(ctx, []sdk.Msg{mkTestLegacyContent(t)}, "", "title", "summary", proposer, tc.proposalType) | |
| require.NoError(t, err) | |
| proposalCoins := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, suite.StakingKeeper.TokensFromConsensusPower(ctx, 10*depositMultiplier))} | |
| addr0Str, err := suite.AuthKeeper.AddressCodec().BytesToString(addrs[0]) | |
| require.NoError(t, err) | |
| newDepositMsg := v1.NewMsgDeposit(addr0Str, proposal.Id, proposalCoins) | |
| res, err := govMsgSvr.Deposit(ctx, newDepositMsg) | |
| require.NoError(t, err) | |
| require.NotNil(t, res) | |
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | |
| require.NotNil(t, macc) | |
| moduleAccCoins := suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()) | |
| deposits := initialModuleAccCoins.Add(proposal.TotalDeposit...).Add(proposalCoins...) | |
| require.True(t, moduleAccCoins.Equal(deposits)) | |
| err = suite.GovKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "") | |
| require.NoError(t, err) | |
| newHeader := integration.HeaderInfoFromContext(ctx) | |
| params, err := suite.GovKeeper.Params.Get(ctx) | |
| require.NoError(t, err) | |
| newHeader.Time = newHeader.Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod) | |
| ctx = integration.SetHeaderInfo(ctx, newHeader) | |
| err = suite.GovKeeper.EndBlocker(ctx) | |
| require.NoError(t, err) | |
| macc = suite.GovKeeper.GetGovernanceAccount(ctx) | |
| require.NotNil(t, macc) | |
| require.True(t, suite.BankKeeper.GetAllBalances(ctx, macc.GetAddress()).Equal(initialModuleAccCoins)) | |
| }) | |
| } | |
| } |
| testCases := []struct { | ||
| msg string | ||
| malleate func() | ||
| expPass bool | ||
| expErrMsg string | ||
| }{ | ||
| { | ||
| "request tally after few votes", | ||
| func() { | ||
| proposal, err := f.govKeeper.SubmitProposal(ctx, TestProposal, "", "test", "description", addrs[0], v1.ProposalType_PROPOSAL_TYPE_STANDARD) | ||
| assert.NilError(t, err) | ||
| proposal.Status = v1.StatusVotingPeriod | ||
| err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | ||
| assert.NilError(t, err) | ||
| assert.NilError(t, f.govKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "")) | ||
| assert.NilError(t, f.govKeeper.AddVote(ctx, proposal.Id, addrs[1], v1.NewNonSplitVoteOption(v1.OptionYes), "")) | ||
| assert.NilError(t, f.govKeeper.AddVote(ctx, proposal.Id, addrs[2], v1.NewNonSplitVoteOption(v1.OptionYes), "")) | ||
|
|
||
| req = &v1beta1.QueryTallyResultRequest{ProposalId: proposal.Id} | ||
|
|
||
| expRes = &v1beta1.QueryTallyResultResponse{ | ||
| Tally: v1beta1.TallyResult{ | ||
| Yes: math.NewInt(3 * 5 * 1000000), | ||
| No: math.NewInt(0), | ||
| Abstain: math.NewInt(0), | ||
| NoWithVeto: math.NewInt(0), | ||
| }, | ||
| } | ||
| }, | ||
| true, | ||
| "", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases
The test suite currently only covers the happy path. Consider adding test cases for:
- Invalid proposal IDs
- Proposals in different states (e.g., DepositPeriod, Completed)
- Edge cases with different voting patterns
- Error scenarios
Example additional test case:
testCases := []struct {
msg string
malleate func()
expPass bool
expErrMsg string
}{
+ {
+ "invalid proposal id",
+ func() {
+ req = &v1beta1.QueryTallyResultRequest{ProposalId: 999}
+ },
+ false,
+ "proposal 999 doesn't exist",
+ },
// existing test case...
}Committable suggestion skipped: line range outside the PR's diff.
| func getTestProposal() []sdk.Msg { | ||
| legacyProposalMsg, err := v1.NewLegacyContent(v1beta1.NewTextProposal("Title", "description"), authtypes.NewModuleAddress(types.ModuleName).String()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| testProposal := v1beta1.NewTextProposal("Proposal", "testing proposal") | ||
| legacyProposalMsg2, err := v1.NewLegacyContent(testProposal, authtypes.NewModuleAddress(types.ModuleName).String()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return []sdk.Msg{ | ||
| legacyProposalMsg, | ||
| legacyProposalMsg2, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panic with error return in test utility
Using panic in test utilities makes it harder to debug test failures. Consider returning an error instead.
-func getTestProposal() []sdk.Msg {
+func getTestProposal() ([]sdk.Msg, error) {
legacyProposalMsg, err := v1.NewLegacyContent(...)
if err != nil {
- panic(err)
+ return nil, fmt.Errorf("failed to create first legacy content: %w", err)
}
// ... similar changes for second message
- return []sdk.Msg{
+ return []sdk.Msg{
legacyProposalMsg,
legacyProposalMsg2,
- }
+ }, nil
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -35,7 +35,7 @@ func AddTestAddrsFromPubKeys(bankKeeper BankKeeper, stakingKeeper StakingKeeper, | |||
|
|
|||
| // AddTestAddrs constructs and returns accNum amount of accounts with an | |||
| // initial balance of accAmt in random order | |||
| func AddTestAddrs(bankKeeper BankKeeper, stakingKeeper StakingKeeper, ctx sdk.Context, accNum int, accAmt math.Int) []sdk.AccAddress { | |||
| func AddTestAddrs(bankKeeper BankKeeper, stakingKeeper StakingKeeper, ctx context.Context, accNum int, accAmt math.Int) []sdk.AccAddress { | |||
There was a problem hiding this comment.
💡 Codebase verification
Update required: Caller still using sdk.Context
The function AddTestAddrsIncremental in simapp/test_helpers.go still uses sdk.Context when calling AddTestAddrs. This needs to be updated to use context.Context to maintain consistency with the modified function signature.
simapp/test_helpers.go: Update the context type inAddTestAddrsIncrementalandaddTestAddrsfunctions fromsdk.Contexttocontext.Context
🔗 Analysis chain
LGTM! Good migration to standard context.
The change from sdk.Context to context.Context aligns with Go's standard practices and maintains consistency with other functions in the file.
Let's verify that all callers have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to AddTestAddrs with sdk.Context
rg -l "AddTestAddrs.*sdk\.Context"
Length of output: 59
Script:
#!/bin/bash
# Let's check the actual usage in simapp/test_helpers.go
rg -A 5 "AddTestAddrs.*sdk\.Context" simapp/test_helpers.go
Length of output: 436
| func TestTallyJailedValidator(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| f := initFixture(t) | ||
|
|
||
| ctx := f.ctx | ||
|
|
||
| addrs, valAddrs := createValidators(t, f, []int64{25, 6, 7}) | ||
|
|
||
| delTokens := f.stakingKeeper.TokensFromConsensusPower(ctx, 10) | ||
| val2, found := f.stakingKeeper.GetValidator(ctx, valAddrs[1]) | ||
| assert.Assert(t, found) | ||
| val3, found := f.stakingKeeper.GetValidator(ctx, valAddrs[2]) | ||
| assert.Assert(t, found) | ||
|
|
||
| _, err := f.stakingKeeper.Delegate(ctx, addrs[3], delTokens, stakingtypes.Unbonded, val2, true) | ||
| assert.NilError(t, err) | ||
| _, err = f.stakingKeeper.Delegate(ctx, addrs[3], delTokens, stakingtypes.Unbonded, val3, true) | ||
| assert.NilError(t, err) | ||
|
|
||
| _, err = f.stakingKeeper.EndBlocker(ctx) | ||
| assert.NilError(t, err) | ||
| consAddr, err := val2.GetConsAddr() | ||
| assert.NilError(t, err) | ||
| assert.NilError(t, f.stakingKeeper.Jail(ctx, sdk.ConsAddress(consAddr))) | ||
|
|
||
| tp := TestProposal | ||
| proposal, err := f.govKeeper.SubmitProposal(ctx, tp, "", "test", "description", addrs[0], v1.ProposalType_PROPOSAL_TYPE_STANDARD) | ||
| assert.NilError(t, err) | ||
| proposalID := proposal.Id | ||
| proposal.Status = v1.StatusVotingPeriod | ||
| err = f.govKeeper.Proposals.Set(ctx, proposal.Id, proposal) | ||
| assert.NilError(t, err) | ||
| assert.NilError(t, f.govKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "")) | ||
| assert.NilError(t, f.govKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), "")) | ||
| assert.NilError(t, f.govKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionNo), "")) | ||
|
|
||
| proposal, ok := f.govKeeper.Proposals.Get(ctx, proposalID) | ||
| assert.Assert(t, ok) | ||
| passes, burnDeposits, tallyResults, _ := f.govKeeper.Tally(ctx, proposal) | ||
|
|
||
| assert.Assert(t, passes) | ||
| assert.Assert(t, burnDeposits == false) | ||
| assert.Assert(t, tallyResults.Equals(v1.EmptyTallyResult()) == false) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling test coverage.
While the happy path is well tested, consider adding more explicit error case tests:
- Test with invalid proposal status transitions
- Test with duplicate votes
- Test with invalid vote options
Example test case:
func TestTallyWithInvalidVotes(t *testing.T) {
t.Parallel()
f := initFixture(t)
ctx := f.ctx
addrs, _ := createValidators(t, f, []int64{5, 5, 5})
proposal, err := f.govKeeper.SubmitProposal(ctx, tp, "", "test", "description", addrs[0], v1.ProposalType_PROPOSAL_TYPE_STANDARD)
assert.NilError(t, err)
// Test duplicate vote
err = f.govKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "")
assert.NilError(t, err)
err = f.govKeeper.AddVote(ctx, proposal.Id, addrs[0], v1.NewNonSplitVoteOption(v1.OptionNo), "")
assert.ErrorContains(t, err, "duplicate vote")
}
|
@sontrinh16 , please delete the v1 files. |
…osmos/cosmos-sdk into son/port_gov_integration_tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/integration/v2/app.go (2)
103-104: Add validation for GasService in NewAppThe GasService integration looks good, but consider adding validation to ensure GasService is not nil in NewApp to prevent potential runtime panics.
Add this validation at the beginning of NewApp:
func NewApp( appConfig depinject.Config, startupConfig StartupConfig, extraOutputs ...interface{}, ) (*App, error) { + if startupConfig.GasService == nil { + return nil, fmt.Errorf("gas service cannot be nil") + }Also applies to: 135-135, 200-200
Line range hint
345-349: Improve block metadata update logicThe current implementation has several areas for improvement:
- Block time is not updated despite being mentioned in the changes
- Context type assertion could be more idiomatic
- Missing error handling for context type assertion
Consider applying this improvement:
- iCtx, ok := ctx.Value(contextKey).(*integrationContext) - if ok { - iCtx.header.Height = int64(a.lastHeight) - } + if iCtx, ok := ctx.Value(contextKey).(*integrationContext); ok { + iCtx.header.Height = int64(a.lastHeight) + iCtx.header.Time = req.Time // Update block time from request + } else { + t.Logf("warning: integration context not found, skipping block metadata update") + }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/v2/app.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/app.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
| @@ -129,6 +132,7 @@ func DefaultStartUpConfig(t *testing.T) StartupConfig { | |||
| stf.NewMsgRouterService, stf.NewQueryRouterService(), | |||
| ), | |||
| HeaderService: services.NewGenesisHeaderService(stf.HeaderService{}), | |||
| GasService: stf.NewGasMeterService(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for GasService integration
While the GasService integration looks good, consider adding specific test cases to verify:
- Gas consumption tracking
- Gas limit enforcement
- Error handling for out-of-gas scenarios
Would you like me to help create these test cases or open a GitHub issue to track this task?
| @@ -35,7 +35,7 @@ func AddTestAddrsFromPubKeys(bankKeeper BankKeeper, stakingKeeper StakingKeeper, | |||
|
|
|||
| // AddTestAddrs constructs and returns accNum amount of accounts with an | |||
| // initial balance of accAmt in random order | |||
| func AddTestAddrs(bankKeeper BankKeeper, stakingKeeper StakingKeeper, ctx sdk.Context, accNum int, accAmt math.Int) []sdk.AccAddress { | |||
| func AddTestAddrs(bankKeeper BankKeeper, stakingKeeper StakingKeeper, ctx context.Context, accNum int, accAmt math.Int) []sdk.AccAddress { | |||
There was a problem hiding this comment.
Can you backport this file manually in 0.52?
There was a problem hiding this comment.
yeah i can open a PR to v0.52.x
There was a problem hiding this comment.
Thanks, let's backport that whole file I think, as more changes happen in previous PRs I see (https://github.com/cosmos/cosmos-sdk/pull/22754/files#diff-930b2d983970f416204d5439b3ddd15e9b8f32b0c080b7fa8d84f34245d43191)
There was a problem hiding this comment.
Oh and this too: #22667 (review) :D
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
tests/integration/v2/services.go (1)
163-163: Format the file usinggofumpt -extraAccording to the static analysis hint, the file is not formatted according to
gofumpt -extra. Please run:gofumpt -w -extra services.goto ensure consistent formatting.
🧰 Tools
🪛 golangci-lint (1.62.2)
163-163: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/go.mod(1 hunks)tests/integration/gov/common_test.go(0 hunks)tests/integration/gov/keeper/keeper_test.go(0 hunks)tests/integration/gov/module_test.go(0 hunks)tests/integration/v2/services.go(6 hunks)
💤 Files with no reviewable changes (3)
- tests/integration/gov/module_test.go
- tests/integration/gov/keeper/keeper_test.go
- tests/integration/gov/common_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/services.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/v2/services.go
163-163: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (1)
tests/integration/v2/services.go (1)
10-10: Use the correct gogoproto package
Ensure you are importing github.com/cosmos/gogoproto/jsonpb from the correct module path. It should be cosmossdk.io/gogoproto/jsonpb to align with the project's import paths.
| func (g *GasService) GasConfig(ctx context.Context) gas.GasConfig { | ||
| return gas.GasConfig{} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide appropriate GasConfig implementation
The GasConfig method currently returns an empty gas.GasConfig{}. If specific gas configurations are required for accurate gas consumption and limits, consider returning the appropriate configuration.
Apply this diff to return a proper GasConfig:
func (g *GasService) GasConfig(ctx context.Context) gas.GasConfig {
- return gas.GasConfig{}
+ return gas.GasConfig{
+ Limit: 1000000, // set appropriate gas limit
+ Prices: []sdk.DecCoin{
+ {Denom: "stake", Amount: sdk.NewDec(1)}, // set appropriate gas prices
+ },
+ }
}Adjust the values to match the desired gas settings for your application.
Committable suggestion skipped: line range outside the PR's diff.
| return GasMeterFromContext(ctx) | ||
| } |
There was a problem hiding this comment.
Handle potential nil Gas Meter
In the GasMeter method of GasService, if GasMeterFromContext(ctx) returns nil, it could lead to a nil pointer dereference when the gas meter is used. Consider adding a check to handle cases where the gas meter is not present in the context.
Apply this diff to handle the potential nil:
func (g *GasService) GasMeter(ctx context.Context) gas.Meter {
gm := GasMeterFromContext(ctx)
+ if gm == nil {
+ return stfgas.NewMeter(0) // or handle the error as appropriate
+ }
return gm
}Ensure that the fallback behavior aligns with the application's requirements.
Committable suggestion skipped: line range outside the PR's diff.
| func (e *eventService) EventManager(ctx context.Context) event.Manager { | ||
| iCtx, ok := ctx.Value(contextKey).(*integrationContext) | ||
| if !ok { | ||
| panic("context is not an integration context") |
There was a problem hiding this comment.
Avoid using panic; return an error instead
In the EventManager method, panicking when the context is not an integration context can lead to unexpected crashes. Instead, consider returning an error to allow callers to handle the situation gracefully.
Apply this diff to handle the error:
-func (e *eventService) EventManager(ctx context.Context) event.Manager {
+func (e *eventService) EventManager(ctx context.Context) (event.Manager, error) {
iCtx, ok := ctx.Value(contextKey).(*integrationContext)
if !ok {
- panic("context is not an integration context")
+ return nil, errors.New("context is not an integration context")
}
- return &eventManager{ctx: iCtx}
+ return &eventManager{ctx: iCtx}, nil
}This change will require updating the method signature to return (event.Manager, error) and handling the error where EventManager is called.
Committable suggestion skipped: line range outside the PR's diff.
Description
ref: #20799
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 changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
GasServicefor enhanced gas management.Bug Fixes
Tests
Chores