chore: cleanup comet dependence in tests #18239
Conversation
WalkthroughThe changes primarily revolve around the replacement of Changes
TipsChat with CodeRabbit Bot (
|
tac0turtle
changed the title
github-actions
bot
added
C:x/staking
C:x/slashing
C:x/distribution
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…cosmos-sdk into facu/module-test-cleanup
github-prbot
requested review from
a team,
kocubinski and
likhita-809
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- x/nft/go.mod
- x/protocolpool/go.mod
Files selected for processing (31)
- baseapp/test_helpers.go (1} hunks)
- tests/integration/bank/app_test.go (6} hunks)
- testutil/sims/tx_helpers.go (2} hunks)
- x/auth/ante/feegrant_test.go (2} hunks)
- x/auth/vesting/fuzz_test.go (2} hunks)
- x/auth/vesting/msg_server_test.go (2} hunks)
- x/auth/vesting/types/vesting_account_test.go (22} hunks)
- x/authz/keeper/genesis_test.go (2} hunks)
- x/authz/simulation/operations.go (3} hunks)
- x/authz/simulation/operations_test.go (6} hunks)
- x/bank/keeper/collections_test.go (2} hunks)
- x/bank/keeper/keeper_test.go (14} hunks)
- x/bank/simulation/operations.go (2} hunks)
- x/bank/simulation/operations_test.go (5} hunks)
- x/distribution/keeper/allocation_test.go (4} hunks)
- x/distribution/keeper/delegation_test.go (10} hunks)
- x/distribution/keeper/keeper_test.go (2} hunks)
- x/distribution/simulation/operations_test.go (4} hunks)
- x/evidence/genesis_test.go (2} hunks)
- x/evidence/keeper/keeper_test.go (2} hunks)
- x/feegrant/simulation/operations_test.go (4} hunks)
- x/genutil/types/genesis.go (2} hunks)
- x/gov/abci_test.go (8} hunks)
- x/gov/simulation/operations_test.go (7} hunks)
- x/group/keeper/keeper_test.go (2} hunks)
- x/group/simulation/operations_test.go (15} hunks)
- x/nft/simulation/operations_test.go (2} hunks)
- x/protocolpool/simulation/operations_test.go (2} hunks)
- x/slashing/app_test.go (3} hunks)
- x/slashing/simulation/operations_test.go (1} hunks)
- x/staking/app_test.go (5} hunks)
Files skipped from review due to trivial changes (21)
- baseapp/test_helpers.go
- tests/integration/bank/app_test.go
- testutil/sims/tx_helpers.go
- x/auth/ante/feegrant_test.go
- x/auth/vesting/fuzz_test.go
- x/auth/vesting/types/vesting_account_test.go
- x/authz/simulation/operations_test.go
- x/bank/keeper/keeper_test.go
- x/bank/simulation/operations_test.go
- x/distribution/keeper/delegation_test.go
- x/evidence/genesis_test.go
- x/evidence/keeper/keeper_test.go
- x/feegrant/simulation/operations_test.go
- x/genutil/types/genesis.go
- x/gov/abci_test.go
- x/gov/simulation/operations_test.go
- x/group/keeper/keeper_test.go
- x/group/simulation/operations_test.go
- x/nft/simulation/operations_test.go
- x/slashing/simulation/operations_test.go
- x/staking/app_test.go
Additional comments: 26
x/protocolpool/simulation/operations_test.go (2)
4-9: The import of
github.com/cometbft/cometbft/abci/typeshas been removed. Ensure that this does not affect any other parts of the code that might be using it. Also, verify that the replacement packages provide equivalent functionality.112-117: The call to
suite.App.FinalizeBlockhas been removed. This suggests a change in the block finalization process. Ensure that this does not affect the correctness of the tests and that the block finalization is handled elsewhere if necessary.x/bank/keeper/collections_test.go (2)
5-11: The import of
cometbftpackage has been replaced with standardtimepackage andcosmossdk.io/core/header. This change aligns with the goal of reducing reliance on external packages and using standard Go packages where possible. It also simplifies the code by using the standardtime.Now()function instead ofcmttime.Now().30-30: The context creation has been modified to use
header.Infoinstead ofcmtproto.Header. This change is consistent with the replacement ofcometbftpackage imports. It's important to ensure that the newheader.Infotype is compatible with all functions that use this context.x/auth/vesting/msg_server_test.go (2)
7-10: The import of
cometbftpackage has been replaced with the standardtimepackage andcosmossdk.io/core/header. This change aligns with the goal of reducing reliance on external packages and using standard Go packages where possible. Ensure that theheader.Infotype andtime.Now()function provide the same functionality as the replacedcmtproto.Headerandcmttime.Now().48-48: The context creation has been updated to use
header.Infoandtime.Now(). This change is consistent with the import changes. However, ensure that theheader.Infotype andtime.Now()function are compatible with theWithHeaderInfomethod and provide the same functionality as the replacedcmtproto.Headerandcmttime.Now().x/slashing/app_test.go (3)
9-9: The import of the
cometbftpackage has been replaced with thecosmossdk.io/core/headerpackage. This change aligns with the PR's goal of reducing dependency on external packages. Ensure that theheaderpackage provides equivalent functionality to thecometbftpackage.93-97: The
cmtproto.Headerhas been replaced withheader.Info. This change is consistent with the new import fromcosmossdk.io/core/header. Theapp.FinalizeBlockfunction call has been removed, suggesting a change in the block finalization process. Ensure that the removal ofapp.FinalizeBlockdoes not affect the test's functionality and that the block finalization process is handled elsewhere in the code.113-114: The
cmtproto.Headerhas been replaced withheader.Infoin this hunk as well. This change is consistent with the new import fromcosmossdk.io/core/header. Ensure that theheader.Infotype provides equivalent functionality to thecmtproto.Headertype.x/authz/keeper/genesis_test.go (2)
7-10: The import of
cometbftpackage has been replaced withcosmossdk.io/core/header. This change aligns with the PR's goal of reducing dependency on external packages. Ensure that theheaderpackage provides equivalent functionality to thecometbftpackage.49-49: The context creation has been modified to use
header.Infoinstead ofcmtproto.Header. This change is consistent with the import changes. Verify thatheader.Infoprovides the same functionality ascmtproto.Headerand that theHeightfield is correctly set.x/authz/simulation/operations.go (3)
- 141-147: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [144-148]
The function
app.SimTxFinalizeBlockhas been replaced withapp.SimDeliver. Ensure thatapp.SimDeliveris capable of handling the transaction delivery as expected. Ifapp.SimDeliverdoes not include the block finalization process, make sure that it is handled elsewhere if necessary.
- 218-224: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [221-225]
The function
app.SimTxFinalizeBlockhas been replaced withapp.SimDeliver. Ensure thatapp.SimDeliveris capable of handling the transaction delivery as expected. Ifapp.SimDeliverdoes not include the block finalization process, make sure that it is handled elsewhere if necessary.
- 318-324: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [321-325]
The function
app.SimTxFinalizeBlockhas been replaced withapp.SimDeliver. Ensure thatapp.SimDeliveris capable of handling the transaction delivery as expected. Ifapp.SimDeliverdoes not include the block finalization process, make sure that it is handled elsewhere if necessary.x/distribution/keeper/allocation_test.go (4)
7-12: The import of
cometbftpackage has been replaced withcosmossdk.io/core/header. This change aligns with the PR's goal of reducing dependency on external packages. Ensure that theheader.Infotype fromcosmossdk.io/core/headerprovides the same functionality ascmtproto.Headerfromcometbft.35-35: The
cmtproto.Headerhas been replaced withheader.Infoin the context creation. This change is consistent with the import changes. Ensure that theheader.Infotype provides the same functionality ascmtproto.Headerand that the context creation is correct.94-94: The
cmtproto.Headerhas been replaced withheader.Infoin the context creation. This change is consistent with the import changes. Ensure that theheader.Infotype provides the same functionality ascmtproto.Headerand that the context creation is correct.224-224: The
cmtproto.Headerhas been replaced withheader.Infoin the context creation. This change is consistent with the import changes. Ensure that theheader.Infotype provides the same functionality ascmtproto.Headerand that the context creation is correct.x/bank/simulation/operations.go (2)
- 174-180: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [177-181]
The function
app.SimTxFinalizeBlockhas been replaced withapp.SimDeliver. Ensure thatapp.SimDeliverperforms all the necessary operations thatapp.SimTxFinalizeBlockwas doing, especially in the context of transaction finalization. If there are any additional steps required for transaction finalization, they should be handled appropriately.
- 390-396: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [393-397]
The function
app.SimTxFinalizeBlockhas been replaced withapp.SimDeliver. As mentioned above, ensure thatapp.SimDeliverperforms all the necessary operations thatapp.SimTxFinalizeBlockwas doing, especially in the context of transaction finalization. If there are any additional steps required for transaction finalization, they should be handled appropriately.x/distribution/keeper/keeper_test.go (2)
7-10: The import of
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"has been replaced with"cosmossdk.io/core/header". This change aligns with the PR's goal of reducing dependency on thecometbftpackage. Ensure that theheaderpackage fromcosmossdk.io/coreprovides equivalent functionality to thecmtprotopackage fromcometbft.42-42: The context creation has been updated to use
header.Info{Time: time.Now()}instead ofcmtproto.Header{Time: time.Now()}. This change is consistent with the updated import. Ensure that all instances ofcmtproto.Headerhave been replaced withheader.Infothroughout the codebase.x/distribution/simulation/operations_test.go (4)
4-9: The import of
github.com/cometbft/cometbft/abci/typeshas been removed. Ensure that this does not affect the functionality of the code, as theabcipackage might have been used for its types and functions. If the functionality provided byabciis now covered by other packages, this change is fine.73-78: The call to
suite.app.FinalizeBlockhas been removed. This function was previously used to finalize the block before executing the operation. If the block finalization is now handled elsewhere or is no longer necessary due to changes in the block processing logic, this change is acceptable. However, ensure that removing this does not affect the state of the blockchain application during testing.113-118: Similar to the previous comment, the call to
suite.app.FinalizeBlockhas been removed. Ensure that this does not affect the state of the blockchain application during testing.174-179: Again, the call to
suite.app.FinalizeBlockhas been removed. Ensure that this does not affect the state of the blockchain application during testing.
| @@ -68,7 +67,7 @@ func (ag *AppGenesis) ValidateAndComplete() error { | |||
| } | |||
|
|
|||
| if ag.GenesisTime.IsZero() { | |||
| ag.GenesisTime = cmttime.Now() | |||
| ag.GenesisTime = time.Now().Round(0).UTC() | |||
There was a problem hiding this comment.
TM has this as a helper, should we just create it in the SDK instead and use it at the remaining places?
There was a problem hiding this comment.
na, we want to avoid more sdk deps 😄
There was a problem hiding this comment.
Then we should add a rule in https://github.com/crypto-com/cosmos-sdk-codeql 😅
This exists: https://github.com/crypto-com/cosmos-sdk-codeql/blob/main/src/system-time.ql, which is good, but it should not trigger with time.Now().Round(0).UTC() and advise to use time.Now().Round(0).UTC() otherwise.
There was a problem hiding this comment.
Created crypto-com/cosmos-sdk-codeql#8.
cc @yihuang.
Description
Related to: #17555 and #17425
Some of the cometbft package usages:
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
header.Infostruct instead of thecmtproto.Headerstruct, improving the consistency and readability of the code.cmttime.Now()with the standardtime.Now(), making the code more standard-compliant.SimTxFinalizeBlockfunction and replaced it withSimDeliverin simulation operations, streamlining the simulation process.These changes primarily improve the code's maintainability and readability, and they do not introduce new features or directly affect end-users.