-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(cron): add cron module for scheduled execution of wasm contracts #87
Conversation
aib rollapp custom modules
specs added for gasless and cron module
x/cron/keeper/grpc_query_params.go
Outdated
"google.golang.org/grpc/status" | ||
) | ||
|
||
func (k Keeper) Params(c context.Context, req *types.QueryParamsRequest) (*types.QueryParamsResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should be used as QueryServer, so we should move it to grpc_query.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
x/cron/keeper/keeper.go
Outdated
|
||
func (k Keeper) CheckSecurityAddress(ctx sdk.Context, from string) bool { | ||
params := k.GetParams(ctx) | ||
for _, addr := range params.SecurityAddress { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return slices.Contains(params.SecurityAddress, from)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the suggestion, its implemented now.
@@ -0,0 +1,94 @@ | |||
package keeper | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lack test for store, keeper, genesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, we've included a few test cases, and we plan to add more for store and query server
x/cron/abci.go
Outdated
) | ||
|
||
func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { | ||
allContracts := k.GetAllContract(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop through all contract here and execute msg can become the bottle neck of the chain if there are many contracts. It also could lead to some critical issue if we can't control the sudo msg. I suggest not follow this approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We specifically fetch and execute contracts that have been whitelisted within the module, ensuring that the process remains streamlined and doesn't pose a bottleneck to the chain's performance.
x/cron/types/params.go
Outdated
var _ paramtypes.ParamSet = (*Params)(nil) | ||
|
||
var ( | ||
DefaultSecurityAddress = []string{"aib1sdggcl7eanaanvcsmvars7l0unsge65wzjm3dc"} //assuming BECH32_PREFIX=aib, will need a Default admin to whitelist contract for cron operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one shouldn't be kept as aib1sdggcl7eanaanvcsmvars7l0unsge65wzjm3dc, the original rollapp-wasm is used for multiple rollapps and the contract prefixes is different. You can leave it on init genesis or on your own init scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thanks
x/cron/types/params.go
Outdated
var ( | ||
DefaultSecurityAddress = []string{"aib1sdggcl7eanaanvcsmvars7l0unsge65wzjm3dc"} //assuming BECH32_PREFIX=aib, will need a Default admin to whitelist contract for cron operations | ||
|
||
DefaultContractGasLimit uint64 = 1000000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one does not be used in any places, please remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
I leave a few comments. If you want to add this module into rollapp-wasm, I suggest adding enable param into the module that will decide the module is enable or not, so that we can choose to use it or not. |
One more thing is that do you want to query custom module in the contract, for example this module? If yes, then you should add wasmbinding into the chain. |
hey @trinitys7, |
app/app.go
Outdated
@@ -158,6 +169,7 @@ func getGovProposalHandlers() []govclient.ProposalHandler { | |||
ibcclientclient.UpdateClientProposalHandler, | |||
ibcclientclient.UpgradeProposalHandler, | |||
) | |||
govProposalHandlers = append(govProposalHandlers, gaslessclient.GaslessProposalHandler...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not append with new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be like:
ibcclientclient.UpgradeProposalHandler,
gaslessclient.GaslessProposalHandler,
proto/aib/gasless/v1beta1/gov.proto
Outdated
@@ -0,0 +1,8 @@ | |||
syntax = "proto3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see any definition for type here. can you explain why this file exist?
x/cron/client/cli/tx.go
Outdated
) | ||
|
||
var ( | ||
DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
x/cron/client/cli/tx.go
Outdated
DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds()) | ||
) | ||
|
||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
} | ||
|
||
// Validate all params | ||
func (p Params) Validate() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point for me where this function used?
@@ -0,0 +1,8 @@ | |||
package types | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used anywhere
x/cron/types/errors.go
Outdated
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
) | ||
|
||
// x/cron module sentinel errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used. Erase it
x/cron/types/codec.go
Outdated
|
||
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | ||
// this line is used by starport scaffolding # 2 | ||
cdc.RegisterConcrete(&MsgRegisterContract{}, "aib/cron/MsgRegisterContract", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be wasmrollapp
or rollapp
or something else not aib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
x/cron/types/codec.go
Outdated
} | ||
|
||
var ( | ||
Amino = codec.NewLegacyAmino() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var should be lower case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
x/cron/keeper/grpc_query.go
Outdated
} | ||
} | ||
|
||
func (q QueryServer) QueryWhitelistedContracts(c context.Context, req *types.QueryWhitelistedContractsRequest) (*types.QueryWhitelistedContractsResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add test for this
Also, we should add more unit tests |
module fixes + testcases post review
Thanks @lacsomot for the review, |
as of now we don't require wasmbindings, will add if the need arises in future |
can you please give more clarity on this or any existing example |
@rockstarRhino an example is erc20 in evmos, you can disable all function in keeper like this |
LGTM! One last thing, the module should be tested with e2e-test to make sure it worked well. Then we can merge it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I've a couple questions and requests please
Architecture
There is a lot of mention of gaming. To me this module seems to be specifically for all in bets to run contracts on begin block, theres isn't actually any generic cron functionality.
Please restructure to allow arbitrary contract calls, without any mention of gaming. It's fine if only begin block is supported for 'cron' but it should be extensible later on to a full cron module.
Docs
Please add a docstring to every new protobuf field.
In the spec, please add a section which does a chronological example walkthrough/flow
'When you do behavior X then you get behavior Y by steps A,B,C'
Code
Please fix the linter for the code and markdown. Please check every function to make sure it's used. Function params should have lower case first letter. Log statements should use key+values, not embedded formatting.
Security
Where is the code that checks that the security address was actually signed or something? You are just comparing addresses. Sorry if I missed it.
// Msg defines the Msg service. | ||
service Msg { | ||
rpc RegisterContract(MsgRegisterContract) returns(MsgRegisterContractResponse); | ||
rpc DeRegisterContract(MsgDeRegisterContract) returns(MsgDeRegisterContractResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deregister not DeRegister
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes applied
x/cron/keeper/genesis.go
Outdated
for _, item := range genState.WhitelistedContracts { | ||
if item.GameId > GameID { | ||
GameID = item.GameId | ||
} | ||
k.SetContract(ctx, item) | ||
} | ||
k.SetGameID(ctx, GameID) | ||
k.SetParams(ctx, genState.Params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs explanation comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about the id maxing
x/cron/keeper/keeper.go
Outdated
sdk "github.com/cosmos/cosmos-sdk/types" | ||
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" | ||
|
||
// "github.com/dymensionxyz/dymension-rdk/utils/logger" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no grey code please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
x/cron/keeper/msg_server.go
Outdated
} | ||
gameID := k.GetGameID(ctx) | ||
contract := types.WhitelistedContract{ | ||
GameId: gameID + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 needs explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gameID is a counter which is incremented whenever a new game contract is whitelisted.
x/cron/types/types.go
Outdated
func (m WhitelistedContract) Validate() error { | ||
if m.GameId == 0 { | ||
return fmt.Errorf("invalid GameId: %d", m.GameId) | ||
} | ||
// check if the security address is valid | ||
if _, err := sdk.AccAddressFromBech32(m.SecurityAddress); err != nil { | ||
return fmt.Errorf("invalid SecurityAddress: %s", m.SecurityAddress) | ||
} | ||
// check if the contract admin is valid | ||
if _, err := sdk.AccAddressFromBech32(m.ContractAdmin); err != nil { | ||
return fmt.Errorf("invalid ContractAdmin: %s", m.ContractAdmin) | ||
} | ||
if m.GameName == "" { | ||
return fmt.Errorf("invalid GameName: %s", m.GameName) | ||
} | ||
// check if the contract address is valid | ||
if _, err := sdk.AccAddressFromBech32(m.ContractAddress); err != nil { | ||
return fmt.Errorf("invalid ContractAddress: %s", m.ContractAddress) | ||
} | ||
// check if game type does not contain 1,2,3 | ||
if !slices.Contains([]uint64{1, 2, 3}, m.GameType) { | ||
return fmt.Errorf("invalid GameType: %d", m.GameType) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe wrap sdkerrors.InvalidType or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes applied
module refactored post review
WalkthroughThe updates introduce a new cron module to the application, allowing for the scheduling and execution of tasks, such as contract calls, at specified phases like begin and end blockers. Key additions include new protobufs for cron job management, CLI commands, and a comprehensive keeper structure for handling cron job operations and security checks. This greatly enhances the application's capability to automate and manage tasks effectively. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Keeper
participant Store
participant Block
User ->> CLI: Register/Update/Delete/Toggle Cron Job
CLI ->> Keeper: Process Transaction
Keeper ->> Store: Update Store
Store ->> Keeper: Confirmation
User ->> Block: Begin Block
Block ->> Keeper: Fetch Cron Jobs
Keeper ->> Block: Execute Cron Jobs
Block --> User: Return Results
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Outside diff range and nitpick comments (2)
x/cron/module.go (2)
36-48
: Ensure consistent module description in comments.The comments describe this as the "capability module," which seems to be a copy-paste error. It should be updated to correctly reflect that this is the "cron module."
97-175
: Review the use of module descriptions and ensure accuracy.Similar to the previous comment, ensure that all references to "capability module" are corrected to "cron module" to avoid confusion and maintain accuracy in the documentation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (7)
go.mod
is excluded by!**/*.mod
x/cron/types/cron.pb.go
is excluded by!**/*.pb.go
x/cron/types/genesis.pb.go
is excluded by!**/*.pb.go
x/cron/types/params.pb.go
is excluded by!**/*.pb.go
x/cron/types/query.pb.go
is excluded by!**/*.pb.go
x/cron/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
x/cron/types/tx.pb.go
is excluded by!**/*.pb.go
Files selected for processing (37)
- app/app.go (11 hunks)
- app/test_suite.go (1 hunks)
- proto/wasmrollapp/cron/v1beta1/cron.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/genesis.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/params.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/query.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/tx.proto (1 hunks)
- x/cron/abci.go (1 hunks)
- x/cron/client/cli/query.go (1 hunks)
- x/cron/client/cli/query_params.go (1 hunks)
- x/cron/client/cli/tx.go (1 hunks)
- x/cron/expected/keeper.go (1 hunks)
- x/cron/genesis.go (1 hunks)
- x/cron/genesis_test.go (1 hunks)
- x/cron/handler.go (1 hunks)
- x/cron/keeper/genesis.go (1 hunks)
- x/cron/keeper/grpc_query.go (1 hunks)
- x/cron/keeper/keeper.go (1 hunks)
- x/cron/keeper/keeper_test.go (1 hunks)
- x/cron/keeper/msg_server.go (1 hunks)
- x/cron/keeper/params.go (1 hunks)
- x/cron/keeper/params_test.go (1 hunks)
- x/cron/keeper/store.go (1 hunks)
- x/cron/keeper/store_test.go (1 hunks)
- x/cron/module.go (1 hunks)
- x/cron/spec/01_concepts.md (1 hunks)
- x/cron/spec/02_state.md (1 hunks)
- x/cron/spec/03_clients.md (1 hunks)
- x/cron/spec/README.md (1 hunks)
- x/cron/types/codec.go (1 hunks)
- x/cron/types/events.go (1 hunks)
- x/cron/types/genesis.go (1 hunks)
- x/cron/types/keys.go (1 hunks)
- x/cron/types/message_de_register_contract.go (1 hunks)
- x/cron/types/message_register_contract.go (1 hunks)
- x/cron/types/params.go (1 hunks)
- x/cron/types/types.go (1 hunks)
Files skipped from review due to trivial changes (4)
- x/cron/genesis_test.go
- x/cron/keeper/keeper_test.go
- x/cron/spec/03_clients.md
- x/cron/types/events.go
Additional context used
Markdownlint
x/cron/spec/README.md
26-26: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
27-27: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
27-27: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
28-28: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
28-28: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
29-29: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
29-29: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
30-30: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
27-27: null (MD023, heading-start-left)
Headings must start at the beginning of the line
29-29: null (MD023, heading-start-left)
Headings must start at the beginning of the line
30-30: null (MD047, single-trailing-newline)
Files should end with a single newline character
LanguageTool
x/cron/spec/01_concepts.md
[uncategorized] ~17-
17: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...me] [contract address] [game type]e.g
console foo@bar:
[misspelling] ~28-~28: This expression is normally spelled as one or with a hyphen. (EN_COMPOUNDS_MULTI_PLAYER)
Context: ...game type` - 1 for single player, 2 for multi player, 3 for both single and multiplayer > N...
[uncategorized] ~37-
37: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...ron de-register-contract [game id]e.g
console foo@bar:
[uncategorized] ~48-~48: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...dress and contract admin are authorized can de-register the contract
Additional comments not posted (50)
x/cron/keeper/params.go (2)
8-12
: The implementation ofGetParams
is clean and follows the Cosmos SDK conventions for parameter retrieval.
14-17
: The implementation ofSetParams
is straightforward and adheres to the Cosmos SDK conventions for parameter setting.x/cron/expected/keeper.go (1)
8-13
: TheContractOpsKeeper
interface is well-defined, encapsulating necessary contract operations which are crucial for the module's functionality.proto/wasmrollapp/cron/v1beta1/params.proto (1)
8-17
: TheParams
message in the protobuf file is well-structured, with appropriate annotations for JSON and YAML serialization, facilitating easy integration and configuration.x/cron/genesis.go (2)
9-13
: TheInitGenesis
function is implemented correctly, following the standard pattern for module initialization in the Cosmos SDK.
15-18
: TheExportGenesis
function is implemented correctly, adhering to the standard pattern for exporting module state in the Cosmos SDK.proto/wasmrollapp/cron/v1beta1/genesis.proto (1)
10-20
: TheGenesisState
message in the protobuf file is well-structured, with appropriate annotations for YAML serialization, ensuring clarity and ease of use in configuration.x/cron/keeper/params_test.go (1)
13-23
: TheTestGetParams
function effectively tests the getter and setter for the module parameters, ensuring they function as expected. Good use of therequire
package for assertions.proto/wasmrollapp/cron/v1beta1/cron.proto (1)
6-19
: The protobuf definition forWhitelistedContract
is well-structured and appropriately documented.x/cron/types/keys.go (1)
7-31
: The constants andContractKey
function are correctly implemented and follow best practices.x/cron/client/cli/query_params.go (1)
12-34
: TheCmdQueryParams
command is correctly implemented and integrates well with the Cosmos SDK.x/cron/types/genesis.go (1)
5-31
: The functions related to the genesis state (NewGenesisState
,DefaultGenesisState
,Validate
) are correctly implemented with comprehensive validation logic.x/cron/abci.go (1)
9-22
: TheBeginBlocker
function is efficiently implemented, handling different game types with appropriate logic.x/cron/keeper/genesis.go (1)
9-35
: TheInitGenesis
andExportGenesis
functions are correctly implemented, ensuring proper state initialization and export.x/cron/types/message_de_register_contract.go (1)
9-51
: Review the naming consistency and error handling inMsgDeregisterContract
.Verification successful
The error handling in
MsgDeregisterContract
is consistent with other message types in the codebase, as they all useErrInvalidAddress
witherrorsmod.Wrapf
.
x/cron/types/message_de_register_contract.go
:ErrInvalidAddress
used for "invalid security address".x/cron/types/message_register_contract.go
:ErrInvalidAddress
used for "invalid creator address" and "invalid contract address".Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of naming and error handling in message types. # Test: Search for similar message types and compare naming and error handling patterns. rg --type go $'func \(msg \*Msg.*\) ValidateBasic' --files-with-matches | xargs rg 'ErrInvalidAddress'Length of output: 505
x/cron/client/cli/query.go (2)
15-31
: Review the CLI command structure for consistency.The structure of the CLI commands is consistent with Cosmos SDK standards and should function as expected. Ensure that all necessary flags and parameters are correctly handled.
33-63
: Validate error handling in CLI commands.Error handling in the CLI commands is robust, covering scenarios like client context retrieval and pagination reading. This should ensure smooth operation under various conditions.
x/cron/types/message_register_contract.go (1)
14-65
: Ensure comprehensive validation in message registration.The validation logic in
MsgRegisterContract
is thorough, checking for valid addresses and game types. This should prevent most common errors related to message creation.x/cron/keeper/grpc_query.go (2)
26-60
: Review pagination and error handling in gRPC queries.The implementation of pagination and error handling in
QueryWhitelistedContracts
follows best practices and should work correctly under typical usage scenarios.
62-67
: Validate parameter fetching in gRPC server.The
Params
method correctly fetches and returns the module parameters, ensuring that clients can reliably retrieve configuration settings.proto/wasmrollapp/cron/v1beta1/query.proto (1)
12-44
: Review gRPC service and message definitions for clarity and completeness.The gRPC service and message definitions in
query.proto
are clear and complete, providing necessary endpoints for module interaction.x/cron/types/types.go (1)
16-52
: Ensure robust validation and initialization inWhitelistedContract
.The
NewWhitelistContract
constructor and theValidate
method intypes.go
are well-implemented, ensuring that contracts are correctly initialized and validated.x/cron/keeper/store.go (6)
9-21
: The implementation ofSetGameID
correctly serializes the game ID using protobuf and stores it efficiently. Good use of Cosmos SDK patterns.
23-38
: TheGetGameID
function is well-implemented with proper error handling for cases where the game ID does not exist.
40-48
: TheSetContract
function efficiently serializes and stores theWhitelistedContract
. Using the game ID as a key is a robust choice to ensure record uniqueness.
50-63
: TheGetWhitelistedContract
function is correctly implemented with comprehensive error handling for non-existent contracts.
65-72
: TheDeleteContract
function is straightforward and correctly implements the deletion of contracts based on the game ID.
74-93
: TheGetWhitelistedContracts
function is well-implemented, using an iterator to fetch all contracts and properly closing it to avoid resource leaks.x/cron/spec/02_state.md (1)
1-70
: The documentation in02_state.md
is clear and well-structured, providing a comprehensive overview of the state objects, genesis, and parameters for thex/cron
module.x/cron/types/params.go (5)
18-21
: TheParamKeyTable
function is correctly implemented, setting up the parameter key table for thex/cron
module using standard Cosmos SDK patterns.
23-28
: TheNewParams
function is straightforward and correctly constructs a newParams
object with the provided security addresses and enable flag.
30-33
: TheDefaultParams
function correctly provides default parameters for the module, effectively using theNewParams
function to avoid code duplication.
35-41
: TheParamSetPairs
function is well-implemented, setting up parameter set pairs with appropriate validation functions to ensure the integrity of the parameters.
67-82
: TheValidate
function is robust, using a structured approach to apply validation functions to each parameter field, ensuring all parameters are correctly validated.x/cron/client/cli/tx.go (3)
15-31
: TheGetTxCmd
function correctly sets up the transaction commands for the module, adding specific commands for contract registration and deregistration. Good use of Cobra and Cosmos SDK patterns.
33-65
: TheCmdRegisterContract
function is well-implemented, with comprehensive error handling and parameter parsing. It correctly uses Cosmos SDK functions for transaction generation and broadcasting.
67-97
: TheCmdDeRegisterContract
function is correctly implemented, providing robust error handling and using standard Cosmos SDK functions for transaction handling.x/cron/keeper/keeper.go (6)
28-48
: TheNewKeeper
function is correctly implemented, ensuring that the parameter store is properly initialized with a key table before setting up theKeeper
.
50-67
: TheSudoContractCall
function is robust, with comprehensive error handling and event emission, enhancing transparency and auditability.
69-72
: TheCheckSecurityAddress
function is efficiently implemented, using theslices.Contains
function to check if an address is a security address.
74-76
: TheStore
function is straightforward and correctly returns a KVStore instance for storing module data.
78-85
: TheSinglePlayer
function is well-implemented, handling contract calls for single-player games with appropriate logging for both success and error scenarios.
87-101
: TheMultiPlayer
function is effectively implemented, managing contract calls for multi-player games with detailed logging for various stages of game setup and resolution.x/cron/keeper/msg_server.go (2)
25-72
: TheRegisterContract
function is robust, handling contract registration with comprehensive checks for module enablement, parameter validation, and authorization. It also logs errors appropriately, enhancing debuggability.
74-108
: TheDeregisterContract
function is effectively implemented, handling contract deregistration with checks for module enablement and detailed error handling. It also correctly checks for contract existence based on the game ID.app/app.go (5)
198-198
: Ensure the cron module is correctly integrated into the module basics.The addition of
cron.AppModuleBasic{}
toModuleBasics
is correctly done, ensuring that the cron module is initialized properly with other modules.
575-575
: Confirm the newAppModule
integration for the cron module.The integration of
cron.NewAppModule
into the application's module manager is correctly done, ensuring that the cron module's functionalities are hooked into the application lifecycle properly.
606-606
: Ensure the cron module is correctly ordered in lifecycle events.The cron module is correctly added to the order lists for
BeginBlockers
,EndBlockers
, andInitGenesis
. This ensures that the cron module will participate correctly in the blockchain's lifecycle events.Also applies to: 631-631, 662-662
152-152
: Confirm the addition ofcrontypes.StoreKey
.Verification successful
The verification process confirms that
crontypes.StoreKey
is correctly added and used in theapp/app.go
file. It is included among other store keys and utilized in the initialization of theCronKeeper
. Additionally, the definition ofStoreKey
inx/cron/types/keys.go
aligns with its usage.
crontypes.StoreKey
is defined inx/cron/types/keys.go
.- It is imported and used appropriately in
app/app.go
.No issues were found with the addition of
crontypes.StoreKey
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `crontypes.StoreKey` is used appropriately across the application. # Test: Search for the usage of `crontypes.StoreKey`. Expect: Usage in relevant modules. rg --type go $'crontypes.StoreKey'Length of output: 109
Script:
#!/bin/bash # Extract the relevant code sections from app/app.go to review the context of crontypes.StoreKey usage. rg -A 5 -B 5 'crontypes.StoreKey' app/app.go # Verify the definition and import of crontypes.StoreKey. rg 'StoreKey' $(fd .go)Length of output: 5681
214-214
: Verify the permissions setup for the cron module.Verification successful
The cron module does not require specific permissions.
- The instance
crontypes.ModuleName: nil
confirms no specific permissions are set.- Other instances of
crontypes.ModuleName
in the context do not indicate any permissions setup.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the cron module does not require specific permissions. # Test: Check the permissions for `crontypes.ModuleName`. Expect: No specific permissions. echo "Permissions for cron module:"; grep 'crontypes.ModuleName' -A 1 app/app.goLength of output: 379
Script:
#!/bin/bash # Description: Extract and review the context around each instance of `crontypes.ModuleName` in app/app.go to ensure no permissions are set. # Extract 5 lines before and after each instance of `crontypes.ModuleName` to understand the context. echo "Context around crontypes.ModuleName instances:" grep -C 5 'crontypes.ModuleName' app/app.goLength of output: 2108
app/test_suite.go
Outdated
DefaultSecurityAddress := []string{"cosmos1xkxed7rdzvmyvgdshpe445ddqwn47fru24fnlp"} | ||
params := cronTypes.Params{SecurityAddress: DefaultSecurityAddress} | ||
cronGenesis := cronTypes.GenesisState{ | ||
Params: params, | ||
WhitelistedContracts: []cronTypes.WhitelistedContract{}, | ||
} | ||
genesisState[cronTypes.ModuleName] = app.AppCodec().MustMarshalJSON(&cronGenesis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add unit tests for the cron module initialization in the genesis state.
It seems like the initialization of the cron module's genesis state is handled here. Would you like me to help by writing some unit tests to ensure this initialization behaves as expected?
app/app.go
Outdated
app.CronKeeper = cronkeeper.NewKeeper( | ||
appCodec, | ||
app.keys[crontypes.StoreKey], | ||
app.keys[crontypes.MemStoreKey], | ||
app.GetSubspace(crontypes.ModuleName), | ||
&app.WasmKeeper, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the initialization parameters for the CronKeeper
.
The initialization of CronKeeper
seems to be missing the contractOpsKeeper
, which is essential for contract operations. This should be included to ensure the cron module can interact with contracts as expected.
x/cron/keeper/store_test.go
Outdated
func (s *KeeperTestSuite) TestSetContract() { | ||
|
||
cronKeeper, ctx := &s.app.CronKeeper, &s.ctx | ||
params := cronKeeper.GetParams(*ctx) | ||
|
||
for _, tc := range []struct { | ||
name string | ||
msg cronTypes.WhitelistedContract | ||
ExpErr error | ||
}{ | ||
{ | ||
"Single Player Registered Contract successfully", | ||
cronTypes.WhitelistedContract{ | ||
GameId: 1, | ||
SecurityAddress: params.GetSecurityAddress()[0], | ||
ContractAdmin: s.addr(2).String(), | ||
GameName: "coinflip", | ||
ContractAddress: s.addr(3).String(), | ||
GameType: 1, | ||
}, | ||
nil, | ||
}, | ||
{ | ||
"Multi Player Registered Contract successfully", | ||
cronTypes.WhitelistedContract{ | ||
GameId: 2, | ||
SecurityAddress: params.GetSecurityAddress()[0], | ||
ContractAdmin: s.addr(2).String(), | ||
GameName: "roulette", | ||
ContractAddress: s.addr(4).String(), | ||
GameType: 2, | ||
}, | ||
nil, | ||
}, | ||
} { | ||
s.Run(tc.name, func() { | ||
cronKeeper.SetContract(*ctx, tc.msg) | ||
|
||
res, found := cronKeeper.GetWhitelistedContract(*ctx, tc.msg.GameId) | ||
s.Require().True(found) | ||
s.Require().Equal(res.GameId, tc.msg.GameId) | ||
s.Require().Equal(res.SecurityAddress, tc.msg.SecurityAddress) | ||
s.Require().Equal(res.ContractAdmin, tc.msg.ContractAdmin) | ||
s.Require().Equal(res.GameName, tc.msg.GameName) | ||
s.Require().Equal(res.ContractAddress, tc.msg.ContractAddress) | ||
s.Require().Equal(res.GameType, tc.msg.GameType) | ||
|
||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure comprehensive test coverage for edge cases.
The current tests cover basic successful scenarios. Consider adding negative test cases and edge cases, such as attempting to set a contract with invalid parameters. Would you like me to help by adding these test cases?
x/cron/spec/README.md
Outdated
### Step 1: Upload the contract | ||
#### ex: `wasmrollappd tx wasm store "$DIR/../../x/wasm/keeper/testdata/coinflip.wasm` | ||
### Step 2: Register the contract according to the game type | ||
#### ex: `wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1` | ||
### result : cronjob for the specific contract is activated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper spacing around headings for readability.
+
#### ex: `wasmrollappd tx wasm store "$DIR/../../x/wasm/keeper/testdata/coinflip.wasm`
+
### Step 2: Register the contract according to the game type
+
#### ex: `wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1`
+
### result : cronjob for the specific contract is activated
+
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.
### Step 1: Upload the contract | |
#### ex: `wasmrollappd tx wasm store "$DIR/../../x/wasm/keeper/testdata/coinflip.wasm` | |
### Step 2: Register the contract according to the game type | |
#### ex: `wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1` | |
### result : cronjob for the specific contract is activated | |
### Step 1: Upload the contract | |
#### ex: `wasmrollappd tx wasm store "$DIR/../../x/wasm/keeper/testdata/coinflip.wasm` | |
### Step 2: Register the contract according to the game type | |
#### ex: `wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1` | |
### result : cronjob for the specific contract is activated | |
Tools
Markdownlint
26-26: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
27-27: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
27-27: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
28-28: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
28-28: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
29-29: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
29-29: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
30-30: Expected: 1; Actual: 0; Above (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
27-27: null (MD023, heading-start-left)
Headings must start at the beginning of the line
29-29: null (MD023, heading-start-left)
Headings must start at the beginning of the line
30-30: null (MD047, single-trailing-newline)
Files should end with a single newline character
x/cron/handler.go
Outdated
// NewHandler ... | ||
func NewHandler(k keeper.Keeper) sdk.Handler { | ||
// this line is used by starport scaffolding # handler/msgServer | ||
|
||
server := keeper.NewMsgServerImpl(k) | ||
|
||
return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { | ||
ctx = ctx.WithEventManager(sdk.NewEventManager()) | ||
|
||
switch msg := msg.(type) { | ||
case *types.MsgRegisterContract: | ||
res, err := server.RegisterContract(sdk.WrapSDKContext(ctx), msg) | ||
return sdk.WrapServiceResult(ctx, res, err) | ||
case *types.MsgDeregisterContract: | ||
res, err := server.DeregisterContract(sdk.WrapSDKContext(ctx), msg) | ||
return sdk.WrapServiceResult(ctx, res, err) | ||
// this line is used by starport scaffolding # 1 | ||
default: | ||
errMsg := fmt.Sprintf("unrecognized %s message type: %T", types.ModuleName, msg) | ||
return nil, errorsmod.Wrap(sdkerrors.ErrUnknownRequest, errMsg) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The verification process revealed that the message types MsgRegisterContractResponse
and MsgDeregisterContractResponse
are not handled in the switch statement within the NewHandler
function.
MsgRegisterContractResponse
is not handled in the switch statement.MsgDeregisterContractResponse
is not handled in the switch statement.
Please ensure that all message types are appropriately handled in the switch statement to avoid potential runtime errors.
Analysis chain
Review the error handling in the default case of the switch statement.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all possible message types are handled correctly in the switch statement.
# Test: Search for all message types and ensure they are handled in the switch statement.
rg --type go $'type Msg.* struct' --files-with-matches | xargs rg 'case \*types.Msg'
Length of output: 85
Script:
#!/bin/bash
# Description: Identify all message types and verify their handling in the switch statement.
# Step 1: Find all message type definitions in the codebase.
msg_types=$(rg --type go --no-filename --no-line-number 'type Msg.* struct' | awk '{print $2}')
# Step 2: Check if each message type is handled in the switch statement within the NewHandler function.
for msg_type in $msg_types; do
echo "Checking handling for message type: $msg_type"
rg --type go "case \*types.$msg_type" || echo "Message type $msg_type is not handled in the switch statement."
done
Length of output: 1514
// Msg defines the Msg service. | ||
service Msg { | ||
rpc RegisterContract(MsgRegisterContract) returns(MsgRegisterContractResponse); | ||
rpc DeregisterContract(MsgDeregisterContract) returns(MsgDeregisterContractResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure consistent naming conventions for RPC methods.
- rpc DeregisterContract(MsgDeregisterContract) returns(MsgDeregisterContractResponse);
+ rpc DeRegisterContract(MsgDeregisterContract) returns(MsgDeregisterContractResponse);
Committable suggestion was skipped due to low confidence.
x/cron/types/codec.go
Outdated
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | ||
// this line is used by starport scaffolding # 2 | ||
cdc.RegisterConcrete(&MsgRegisterContract{}, "wasmrollapp/cron/MsgRegisterContract", nil) | ||
cdc.RegisterConcrete(&MsgDeregisterContract{}, "wasmrollapp/cron/MsgDeregisterContract", nil) | ||
|
||
} | ||
|
||
func RegisterInterfaces(registry cdctypes.InterfaceRegistry) { | ||
// this line is used by starport scaffolding # 3 | ||
registry.RegisterImplementations( | ||
(*sdk.Msg)(nil), | ||
&MsgRegisterContract{}, | ||
&MsgDeregisterContract{}, | ||
) | ||
|
||
msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc) | ||
} | ||
|
||
var ( | ||
amino = codec.NewLegacyAmino() | ||
moduleCdc = codec.NewAminoCodec(amino) | ||
) | ||
|
||
func init() { | ||
RegisterLegacyAminoCodec(amino) | ||
cryptocodec.RegisterCrypto(amino) | ||
RegisterLegacyAminoCodec(authzcodec.Amino) | ||
amino.Seal() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure that all necessary types are registered in the codec.
- The following message types are missing registration:
MsgRegisterContractResponse
MsgDeregisterContractResponse
Please add the missing registrations to the RegisterLegacyAminoCodec
function.
Analysis chain
Ensure that all necessary types are registered in the codec.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all message types are registered in the codec.
# Test: Search for all message types and ensure they are registered in the codec.
rg --type go $'type Msg.* struct' --files-with-matches | xargs rg 'cdc.RegisterConcrete'
Length of output: 87
Script:
#!/bin/bash
# Description: Verify that all message types are registered in the codec.
# Step 1: Identify all message types in the codebase.
msg_types=$(rg --type go --no-heading --line-number 'type Msg.* struct' | awk -F: '{print $3}' | awk '{print $2}')
# Step 2: Check if each identified message type is registered in the codec.
for msg in $msg_types; do
echo "Checking registration for $msg"
rg --type go --no-heading --line-number "cdc.RegisterConcrete(&${msg}{}" || echo "Registration missing for $msg"
done
Length of output: 2280
Script:
#!/bin/bash
# Description: Verify that all message types are registered in the codec.
# Step 1: Identify all message types in the codebase.
msg_types=$(rg --type go --no-heading --line-number 'type Msg.* struct' | awk -F: '{print $3}' | awk '{print $2}')
# Step 2: Check if each identified message type is registered in the codec.
for msg in $msg_types; do
echo "Checking registration for $msg"
rg --type go --no-heading --line-number "cdc.RegisterConcrete(\&${msg}\{\}" || echo "Registration missing for $msg"
done
Length of output: 2072
Script:
#!/bin/bash
# Description: Verify that all message types are registered in the codec.
# Step 1: Identify all message types in the codebase.
msg_types=$(rg --type go --no-heading --line-number 'type Msg.* struct' | awk -F: '{print $3}' | awk '{print $2}')
# Step 2: Check if each identified message type is registered in the codec.
for msg in $msg_types; do
echo "Checking registration for $msg"
rg --type go --no-heading --line-number "cdc.RegisterConcrete\(\&${msg}\{\}" || echo "Registration missing for $msg"
done
Length of output: 1618
x/cron/spec/01_concepts.md
Outdated
## Cron | ||
|
||
The cron module provides functionality for scheduling and executing tasks, including executing sudo contract calls during specific phases, such as begin blockers. By integrating scheduled contract executions, `x/cron` enhances the functionality of smart contracts, ensuring critical operations are performed automatically and reliably. | ||
Developers can register their contracts if the address is whitelisted in the module parameters. Contracts can be deregistered if the cron job is no longer needed | ||
|
||
### Registering a contract | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron register-contract [game name] [contract address] [game type] | ||
``` | ||
|
||
e.g | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1 100000000awasm --from cooluser --chain-id test-1 | ||
``` | ||
|
||
In the above tx - | ||
|
||
- `game name` - name of the game contract | ||
- `contract address` - CosmWasm contract address. | ||
- `game type` - 1 for single player, 2 for multi player, 3 for both single and multiplayer | ||
|
||
> Note : only the security address authorized can register the contract | ||
|
||
### De-Registering a contract | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron de-register-contract [game id] | ||
``` | ||
|
||
e.g | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron de-register-contract 2 100000000awasm --from cooluser --chain-id test-1 | ||
``` | ||
|
||
In the above tx - | ||
|
||
- `game id` - id of the game contract to de-register | ||
|
||
> Note : only the security address and contract admin are authorized can de-register the contract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure clarity and correctness in the documentation of the cron
module.
- e.g
+ e.g.,
- e.g
+ e.g.,
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.
## Cron | |
The cron module provides functionality for scheduling and executing tasks, including executing sudo contract calls during specific phases, such as begin blockers. By integrating scheduled contract executions, `x/cron` enhances the functionality of smart contracts, ensuring critical operations are performed automatically and reliably. | |
Developers can register their contracts if the address is whitelisted in the module parameters. Contracts can be deregistered if the cron job is no longer needed | |
### Registering a contract | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-contract [game name] [contract address] [game type] | |
``` | |
e.g | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `game name` - name of the game contract | |
- `contract address` - CosmWasm contract address. | |
- `game type` - 1 for single player, 2 for multi player, 3 for both single and multiplayer | |
> Note : only the security address authorized can register the contract | |
### De-Registering a contract | |
```console | |
foo@bar:~$ wasmrollappd tx cron de-register-contract [game id] | |
``` | |
e.g | |
```console | |
foo@bar:~$ wasmrollappd tx cron de-register-contract 2 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `game id` - id of the game contract to de-register | |
> Note : only the security address and contract admin are authorized can de-register the contract | |
## Cron | |
The cron module provides functionality for scheduling and executing tasks, including executing sudo contract calls during specific phases, such as begin blockers. By integrating scheduled contract executions, `x/cron` enhances the functionality of smart contracts, ensuring critical operations are performed automatically and reliably. | |
Developers can register their contracts if the address is whitelisted in the module parameters. Contracts can be deregistered if the cron job is no longer needed | |
### Registering a contract | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-contract [game name] [contract address] [game type] | |
``` | |
e.g., | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-contract coin-flip rol14hj2tavq8f.... 1 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `game name` - name of the game contract | |
- `contract address` - CosmWasm contract address. | |
- `game type` - 1 for single player, 2 for multi player, 3 for both single and multiplayer | |
> Note : only the security address authorized can register the contract | |
### De-Registering a contract | |
```console | |
foo@bar:~$ wasmrollappd tx cron de-register-contract [game id] | |
``` | |
e.g., | |
```console | |
foo@bar:~$ wasmrollappd tx cron de-register-contract 2 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `game id` - id of the game contract to de-register | |
> Note : only the security address and contract admin are authorized can de-register the contract |
Tools
LanguageTool
[uncategorized] ~17-
17: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...me] [contract address] [game type]e.g
console foo@bar:
[misspelling] ~28-~28: This expression is normally spelled as one or with a hyphen. (EN_COMPOUNDS_MULTI_PLAYER)
Context: ...game type` - 1 for single player, 2 for multi player, 3 for both single and multiplayer > N...
[uncategorized] ~37-
37: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...ron de-register-contract [game id]e.g
console foo@bar:
[uncategorized] ~48-~48: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...dress and contract admin are authorized can de-register the contract
Architecture
this cron module is specifically designed according to the requirements of AIB.
done
While the flow remains straightforward, we have included it in the readme. Please inform me if more details are necessary.
We have addressed the lint issues; however, please let us know if anything was overlooked.
no issues, here we are validating the security address here is the logic where we validate whether the signer is authorized to perform the action or not. Thank you for the review @danwt. Please feel free to revert back if we missed anything. |
// security_address is the address of the security module | ||
string security_address = 1; | ||
// game_name is the name of the game | ||
string game_name = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string game_name = 2; | |
string name = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at my first comment at the very top
// contract_address is the address of the contract | ||
string contract_address = 3; | ||
// game_type is the type of the game 1 -> single, 2 -> multi, 3 -> both | ||
uint64 game_type = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint64 game_type = 4; | |
uint64 type = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at my first comment at the very top
// MsgDeregisterContract defines the Msg/DeregisterContract request type. | ||
message MsgDeregisterContract { | ||
string security_address = 1; | ||
uint64 game_id = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint64 game_id = 2; | |
uint64 id = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at my first comment at the very top
x/cron/expected/keeper.go
Outdated
@@ -0,0 +1,13 @@ | |||
package expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be in types/expected_keepers.go
to be consistent with our modules
x/cron/keeper/msg_server.go
Outdated
} | ||
|
||
// Get Game info from Game Id | ||
gameInfo, found := k.GetWhitelistedContract(ctx, msg.GameId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gameInfo, found := k.GetWhitelistedContract(ctx, msg.GameId) | |
info, found := k.GetWhitelistedContract(ctx, msg.Id) |
x/cron/keeper/keeper.go
Outdated
func (k Keeper) SinglePlayer(ctx sdk.Context, contractAddress string, resolveSinglePlayer []byte, gameName string) { | ||
err := k.SudoContractCall(ctx, contractAddress, resolveSinglePlayer) | ||
if err != nil { | ||
ctx.Logger().Error("Game %s contract call error for single-player", gameName) | ||
} else { | ||
ctx.Logger().Info("Game %s contract call for single-player success", gameName) | ||
} | ||
} | ||
|
||
func (k Keeper) MultiPlayer(ctx sdk.Context, contractAddress string, setupMultiPlayer []byte, resolveMultiPlayer []byte, gameName string) { | ||
err := k.SudoContractCall(ctx, contractAddress, setupMultiPlayer) | ||
if err != nil { | ||
ctx.Logger().Error("Game %s contract call error for setup multi-player", gameName) | ||
} else { | ||
ctx.Logger().Info("Game %s contract call for setup multi-player success", gameName) | ||
} | ||
|
||
err = k.SudoContractCall(ctx, contractAddress, resolveMultiPlayer) | ||
if err != nil { | ||
ctx.Logger().Error("Game %s contract call error for resolve multi-player", gameName) | ||
} else { | ||
ctx.Logger().Info("Game %s contract call for resolve multi-player success", gameName) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming you accept my proposal from the top, this code should be removed
x/cron/keeper/msg_server.go
Outdated
|
||
var _ types.MsgServer = msgServer{} | ||
|
||
func (k msgServer) RegisterContract(goCtx context.Context, msg *types.MsgRegisterContract) (*types.MsgRegisterContractResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, this should be RegisterCronJob
instead
x/cron/keeper/msg_server.go
Outdated
return &types.MsgRegisterContractResponse{}, nil | ||
} | ||
|
||
func (k msgServer) DeregisterContract(goCtx context.Context, msg *types.MsgDeregisterContract) (*types.MsgDeregisterContractResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, DeregisterCronJob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would be nice to have EnableCronJob
and DisableCronJob
x/cron/spec/02_state.md
Outdated
|
||
```go | ||
// this defines the details of the contract registered for cronjob | ||
message WhitelistedContract { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you take my approach as described at the top, the specs will need to be updated
@@ -0,0 +1,52 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at my topmost comment
cron module rework
Hi @zale144 , Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 13
Outside diff range and nitpick comments (4)
x/cron/types/types.go (1)
19-44
: The validation logic inValidate
is comprehensive and covers all necessary aspects. Consider adding comments specifying the maximum allowed lengths forname
anddescription
to improve code maintainability.x/cron/types/message_update_cron_job.go (1)
10-10
: Consider renamingTypeMsgUpdateCronJob
toupdate_cron_job
to follow standard naming conventions for message types.proto/wasmrollapp/cron/v1beta1/tx.proto (2)
30-40
: TheMsgUpdateCronJob
message should include documentation comments similar toMsgRegisterCron
for consistency and clarity.
45-54
: TheMsgDeleteCronJob
message is concise. Consider adding a brief description for thesecurity_address
field to clarify its role.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
x/cron/types/cron.pb.go
is excluded by!**/*.pb.go
x/cron/types/genesis.pb.go
is excluded by!**/*.pb.go
x/cron/types/query.pb.go
is excluded by!**/*.pb.go
x/cron/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
x/cron/types/tx.pb.go
is excluded by!**/*.pb.go
Files selected for processing (29)
- app/test_suite.go (1 hunks)
- proto/wasmrollapp/cron/v1beta1/cron.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/genesis.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/query.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/tx.proto (1 hunks)
- x/cron/abci.go (1 hunks)
- x/cron/client/cli/query.go (1 hunks)
- x/cron/client/cli/tx.go (1 hunks)
- x/cron/handler.go (1 hunks)
- x/cron/keeper/genesis.go (1 hunks)
- x/cron/keeper/grpc_query.go (1 hunks)
- x/cron/keeper/keeper.go (1 hunks)
- x/cron/keeper/keeper_test.go (1 hunks)
- x/cron/keeper/msg_server.go (1 hunks)
- x/cron/keeper/params_test.go (1 hunks)
- x/cron/keeper/store.go (1 hunks)
- x/cron/module.go (1 hunks)
- x/cron/spec/01_concepts.md (1 hunks)
- x/cron/spec/02_state.md (1 hunks)
- x/cron/spec/03_clients.md (1 hunks)
- x/cron/spec/README.md (1 hunks)
- x/cron/types/codec.go (1 hunks)
- x/cron/types/expected_keepers.go (1 hunks)
- x/cron/types/genesis.go (1 hunks)
- x/cron/types/keys.go (1 hunks)
- x/cron/types/message_delete_cron_job.go (1 hunks)
- x/cron/types/message_register_cron.go (1 hunks)
- x/cron/types/message_update_cron_job.go (1 hunks)
- x/cron/types/types.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/cron/spec/03_clients.md
Additional context used
LanguageTool
x/cron/spec/README.md
[formatting] ~28-~28: These punctuation marks differ from each other. Use only one if you are ending a sentence. (DOUBLE_PUNCTUATION_PREMIUM)
Context: ...### Step 1: Upload the contract #### ex., `wasmrollappd tx wasm store "$DIR/../.....
[formatting] ~32-~32: These punctuation marks differ from each other. Use only one if you are ending a sentence. (DOUBLE_PUNCTUATION_PREMIUM)
Context: ...# Step 2: Register the cron job #### ex., `wasmrollappd tx cron register-cron cro...x/cron/spec/01_concepts.md
[uncategorized] ~17-
17: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...ion] [contract_address] [json_msg]e.g
console foo@bar:
[uncategorized] ~38-
38: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...[id] [contract_address] [json_msg]e.g
console foo@bar:
[uncategorized] ~51-~51: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...dress and contract admin are authorized can update the cron job ### Update cron jo...
[uncategorized] ~58-
58: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...e-cron-job [id] [contract_address]e.g
console foo@bar:
[uncategorized] ~70-~70: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...dress and contract admin are authorized can delete the cron job
Additional comments not posted (40)
x/cron/types/expected_keepers.go (1)
8-13
: InterfaceContractOpsKeeper
looks well-defined for contract operations. Consider adding detailed security documentation for theSudo
method due to its privileged nature.proto/wasmrollapp/cron/v1beta1/genesis.proto (1)
10-20
: TheGenesisState
message is well-defined with appropriate use ofgogoproto
options for non-nullability and YAML tags. This ensures robust type handling and integration with configuration management.x/cron/abci.go (1)
8-20
: TheBeginBlocker
function handles cron job execution well with appropriate error handling and logging. Consider adding performance optimizations or limits to handle scenarios with a large number of cron jobs efficiently.x/cron/types/genesis.go (3)
5-10
: TheNewGenesisState
function is correctly implemented and initializes theGenesisState
struct as expected.
12-17
: TheDefaultGenesisState
function correctly provides a default state for the genesis, ensuring that it starts with no cron jobs and default parameters.
19-30
: TheValidate
method is well-implemented, ensuring that both the parameters and each cron job are individually validated. The use of error wrapping enhances the error messages, aiding in debugging.x/cron/types/keys.go (2)
7-22
: All constants are appropriately defined and named, providing clear and understandable configuration for the cron module.
29-31
: TheCronKey
function is correctly implemented, using big endian encoding to ensure byte-order consistency. This is crucial for the interoperability of data stored in different systems.proto/wasmrollapp/cron/v1beta1/cron.proto (2)
8-17
: TheCronJob
message is well-defined, with clear and necessary fields for the identification and description of cron jobs. The use ofrepeated
formsg_contract_cron
is appropriate, allowing flexibility in defining multiple contract calls per cron job.
19-24
: TheMsgContractCron
message is appropriately structured to specify the contract address and the JSON-encoded message for contract interaction. This facilitates clear and direct contract calls.x/cron/keeper/genesis.go (2)
8-24
: TheInitGenesis
function is correctly implemented, ensuring that the genesis state is validated before proceeding with the initialization. The use of panic on validation failure is appropriate in this context to prevent the chain from starting with an invalid configuration.
26-32
: TheExportGenesis
function is well-implemented, effectively gathering the current state of the cron module for export. This is crucial for chain upgrades and state migrations.x/cron/keeper/keeper_test.go (1)
26-32
: SetupTest function is well-structured and initializes the test environment correctly.x/cron/types/codec.go (2)
12-16
: Codec registration is correctly implemented for the cron module.
18-26
: Interface registration is correctly implemented for the cron module.x/cron/types/types.go (1)
10-17
: The functionNewWhitelistContract
is well-structured and correctly initializes aCronJob
object.x/cron/types/message_delete_cron_job.go (2)
13-23
: The constructorNewMsgDeleteCronJob
correctly initializes the message object with appropriate fields.
25-44
: Standard message handling methods (Route
,Type
,GetSigners
,GetSignBytes
) are implemented correctly and adhere to Cosmos SDK conventions.x/cron/keeper/store.go (2)
9-34
: MethodsSetCronID
andGetCronID
are implemented correctly, using standard Cosmos SDK store patterns.
36-75
: MethodsSetCronJob
,GetCronJob
, andGetCronJobs
are implemented correctly, efficiently handling the storage and retrieval ofCronJob
objects.x/cron/keeper/keeper.go (3)
25-43
: The constructorNewKeeper
is implemented correctly, ensuring that a key table is set before proceeding with initialization.
46-59
: The methodSudoContractCall
is implemented correctly, handling errors appropriately and emitting events that are crucial for auditability.
62-64
: The methodCheckSecurityAddress
efficiently checks for security addresses usingslices.Contains
, adhering to best practices.x/cron/types/message_update_cron_job.go (1)
14-25
: The constructorNewMsgUpdateCronJob
correctly initializes a newMsgUpdateCronJob
instance. Good use of structured initialization.x/cron/keeper/grpc_query.go (2)
25-34
: The methodCron
correctly handles nil requests and not found errors. Good use of gRPC status codes for error reporting.
66-69
: TheParams
method is concise and correctly unwraps the SDK context. Good implementation.proto/wasmrollapp/cron/v1beta1/tx.proto (1)
13-25
: TheMsgRegisterCron
message is well-defined with clear documentation for each field. Ensure all fields are necessary and no additional metadata is required for future features.Verification successful
The
MsgRegisterCron
message fields are all used and necessary in the codebase.
security_address
is used inx/cron/types/tx.pb.go
.name
is used inx/cron/types/tx.pb.go
andx/cron/types/types.go
.description
is used inx/cron/types/tx.pb.go
andx/cron/types/types.go
.contract_address
is used inx/cron/types/tx.pb.go
.json_msg
is used inx/cron/types/tx.pb.go
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of all fields in MsgRegisterCron across the codebase to ensure they are necessary. # Test: Search for field usage. Expect: Usage of all fields. rg --type go $'security_address|name|description|contract_address|json_msg'Length of output: 6906
proto/wasmrollapp/cron/v1beta1/query.proto (2)
12-26
: Ensure the gRPC service definitions are correctly implemented and match the expected functionality.
28-61
: Validate the message definitions for querying cron jobs and parameters. Ensure fields are appropriately annotated and types are correct.x/cron/types/message_register_cron.go (1)
14-86
: Review the implementation ofMsgRegisterCron
. Ensure that the validation logic is comprehensive and that the message serialization is correctly implemented.x/cron/client/cli/query.go (1)
13-102
: Ensure the CLI commands are correctly implemented and provide the expected functionality. Check for usability issues and ensure error handling is robust.x/cron/client/cli/tx.go (3)
30-55
: TheCmdRegisterCron
command is well-implemented with appropriate error handling and validation checks.
57-85
: TheCmdUpdateCronJob
command is correctly implemented with comprehensive error handling and validation.
87-114
: TheCmdDeleteCronJob
command is implemented correctly, with checks for the existence of the cron job and proper error handling.app/test_suite.go (1)
51-109
: The setup function and the handling of the genesis state for the cron module are correctly implemented with appropriate error handling and default parameters.x/cron/keeper/msg_server.go (3)
24-64
: TheRegisterCron
function is well-implemented with checks for feature enablement, request validation, and comprehensive error handling.
66-97
: TheUpdateCronJob
function correctly handles the updating of cron jobs with necessary validations and error handling.
99-133
: TheDeleteCronJob
function is implemented correctly, ensuring the cron job's existence and performing necessary security checks before deletion.x/cron/module.go (2)
35-90
: TheAppModuleBasic
structure is correctly implemented, handling module setup, genesis state management, and service registration effectively.
96-174
: TheAppModule
structure is well-implemented, extendingAppModuleBasic
with additional functionalities for handling ABCI messages and maintaining state.
```console | ||
foo@bar:~$ wasmrollappd tx cron delete-cron-job [id] [contract_address] | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the abbreviation for "example."
- e.g
+ e.g.,
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.
e.g., |
Tools
LanguageTool
[uncategorized] ~58-
58: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...e-cron-job [id] [contract_address]e.g
console foo@bar:
x/cron/spec/01_concepts.md
Outdated
- `contract address` - CosmWasm contract address. | ||
- `json_msg` - sudo msg of the contract in json format | ||
|
||
> Note : only the security address and contract admin are authorized can update the cron job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the sentence structure for better readability.
- only the security address and contract admin are authorized can update the cron job
+ only the security address and contract admin who are authorized can update the cron job
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.
> Note : only the security address and contract admin are authorized can update the cron job | |
> Note : only the security address and contract admin who are authorized can update the cron job |
Tools
LanguageTool
[uncategorized] ~51-~51: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...dress and contract admin are authorized can update the cron job ### Update cron jo...
func (msg *MsgDeleteCronJob) ValidateBasic() error { | ||
_, err := sdk.AccAddressFromBech32(msg.SecurityAddress) | ||
if err != nil { | ||
return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid security address (%s)", err) | ||
} | ||
if msg.Id == 0 { | ||
return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "cron id cannot be 0") | ||
} | ||
if len(msg.ContractAddress) == 0 { | ||
return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "contract address cannot be empty") | ||
} | ||
_, err = sdk.AccAddressFromBech32(msg.ContractAddress) | ||
if err != nil { | ||
return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid contract address (%s)", err) | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ValidateBasic
method correctly implements all necessary validation checks. Consider handling potential parsing failures in GetSigners
more gracefully to avoid runtime panics.
foo@bar:~$ wasmrollappd tx cron update-cron-job [id] [contract_address] [json_msg] | ||
``` | ||
|
||
e.g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the abbreviation for "example."
- e.g
+ e.g.,
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.
e.g | |
e.g., |
x/cron/spec/01_concepts.md
Outdated
- `id` - id of the cron job | ||
- `contract address` - CosmWasm contract address. | ||
|
||
> Note : only the security address and contract admin are authorized can delete the cron job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the sentence structure for better readability.
- only the security address and contract admin are authorized can delete the cron job
+ only the security address and contract admin who are authorized can delete the cron job
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.
> Note : only the security address and contract admin are authorized can delete the cron job | |
> Note : only the security address and contract admin who are authorized can delete the cron job |
Tools
LanguageTool
[uncategorized] ~70-~70: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...dress and contract admin are authorized can delete the cron job
func (msg *MsgUpdateCronJob) ValidateBasic() error { | ||
_, err := sdk.AccAddressFromBech32(msg.SecurityAddress) | ||
if err != nil { | ||
return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid security address (%s)", err) | ||
} | ||
if msg.Id == 0 { | ||
return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "cron id cannot be 0") | ||
} | ||
if len(msg.ContractAddress) == 0 { | ||
return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "contract address cannot be empty") | ||
} | ||
_, err = sdk.AccAddressFromBech32(msg.ContractAddress) | ||
if err != nil { | ||
return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid contract address (%s)", err) | ||
} | ||
if len(msg.JsonMsg) == 0 { | ||
return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "json msg cannot be empty") | ||
} | ||
if !json.Valid([]byte(msg.JsonMsg)) { | ||
return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "json msg is invalid") | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ValidateBasic
method thoroughly checks for common errors such as invalid addresses and empty fields. Consider adding a regex or similar validation for jsonMsg
to ensure it meets specific format requirements if applicable.
func (msg *MsgUpdateCronJob) GetSigners() []sdk.AccAddress { | ||
creator, err := sdk.AccAddressFromBech32(msg.SecurityAddress) | ||
if err != nil { | ||
panic(err) | ||
} | ||
return []sdk.AccAddress{creator} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the GetSigners
method handles the potential error from sdk.AccAddressFromBech32
gracefully instead of panicking. Consider returning an error to the caller.
- panic(err)
+ return nil, err
Committable suggestion was skipped due to low confidence.
func (q QueryServer) Crons(c context.Context, req *types.QueryCronsRequest) (*types.QueryCronsResponse, error) { | ||
if req == nil { | ||
return nil, status.Error(codes.InvalidArgument, "empty request") | ||
} | ||
var ( | ||
items []types.CronJob | ||
ctx = sdk.UnwrapSDKContext(c) | ||
) | ||
pagination, err := query.FilteredPaginate( | ||
prefix.NewStore(q.Store(ctx), types.CronJobKeyPrefix), | ||
req.Pagination, | ||
func(_, value []byte, accumulate bool) (bool, error) { | ||
var item types.CronJob | ||
if err := q.cdc.Unmarshal(value, &item); err != nil { | ||
return false, err | ||
} | ||
if accumulate { | ||
items = append(items, item) | ||
} | ||
return true, nil | ||
}, | ||
) | ||
if err != nil { | ||
return nil, status.Error(codes.Internal, err.Error()) | ||
} | ||
return &types.QueryCronsResponse{CronJobs: items, Pagination: pagination}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Crons
method implements pagination and error handling well. Ensure that the Unmarshal
function's error is logged for better traceability in production environments.
+ log.Error("Failed to unmarshal cron job", "error", err)
Committable suggestion was skipped due to low confidence.
x/cron/spec/02_state.md
Outdated
## State Objects | ||
|
||
The `x/cron` module keeps the following object in the state: CronJob. | ||
|
||
This object is used to store the state of a | ||
|
||
- `CronJob` - to store the details of the cron jobs | ||
|
||
```go | ||
// this defines the details of the cronjob | ||
message CronJob { | ||
// id is the unique identifier for the cron job | ||
uint64 id = 1; | ||
// name is the name of the cron job | ||
string name = 2; | ||
// description is the description of the cron job | ||
string description = 3; | ||
// Msgs that will be executed every period amount of time | ||
repeated MsgContractCron msg_contract_cron = 4 [(gogoproto.nullable) = false]; | ||
} | ||
``` | ||
|
||
```go | ||
message MsgContractCron { | ||
// Contract is the address of the smart contract | ||
string contract_address = 1; | ||
// Msg is json encoded message to be passed to the contract | ||
string json_msg = 2; | ||
} | ||
``` | ||
|
||
## Genesis & Params | ||
|
||
The `x/cron` module's `GenesisState` defines the state necessary for initializing the chain from a previously exported height. It contains the module Parameters and Cron jobs. The params are used to control the Security Address which is responsible to register cron operations. This value can be modified with a governance proposal. | ||
|
||
```go | ||
// GenesisState defines the cron module's genesis state. | ||
message GenesisState { | ||
Params params = 1 [ | ||
(gogoproto.moretags) = "yaml:\"params\"", | ||
(gogoproto.nullable) = false | ||
]; | ||
repeated CronJob cron_jobs = 2 [ | ||
(gogoproto.moretags) = "yaml:\"cron_jobs\"", | ||
(gogoproto.nullable) = false | ||
]; | ||
} | ||
``` | ||
|
||
```go | ||
// Params defines the parameters for the module. | ||
message Params { | ||
// Security address that can whitelist/delist contract | ||
repeated string security_address = 1 [ | ||
(gogoproto.jsontag) = "security_address,omitempty", | ||
(gogoproto.moretags) = "yaml:\"security_address\"" | ||
]; | ||
|
||
// set module enabled or not | ||
bool enable_cron = 2; | ||
} | ||
``` | ||
|
||
## State Transitions | ||
|
||
The following state transitions are possible: | ||
|
||
- Register the cron job | ||
- Update the cron job | ||
- Delete the cron job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The search for unit tests covering the state transitions RegisterCron
, UpdateCronJob
, and DeleteCronJob
did not yield any results. This indicates that there might be no direct or indirect unit tests for these methods in the codebase.
- Please ensure that unit tests are implemented for the following state transitions:
- Register the cron job
- Update the cron job
- Delete the cron job
Analysis chain
The documentation provides a comprehensive overview of the state objects and transitions within the x/cron
module. Ensure that all state transitions are covered by unit tests to maintain robustness.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all state transitions are covered by unit tests.
# Test: Search for tests covering state transitions. Expect: Tests for register, update, and delete.
rg --type go $'RegisterCron|UpdateCronJob|DeleteCronJob'
Length of output: 23966
Script:
#!/bin/bash
# Description: Verify that all state transitions are covered by unit tests.
# Test: Search for test files covering state transitions. Expect: Tests for register, update, and delete.
rg --type go --glob '*_test.go' $'RegisterCron|UpdateCronJob|DeleteCronJob'
Length of output: 76
Script:
#!/bin/bash
# Description: Verify that all state transitions are covered by unit tests.
# Test: Search for test files that might indirectly test state transitions or use different naming conventions.
rg --type go --glob '*_test.go' -e 'RegisterCron' -e 'UpdateCronJob' -e 'DeleteCronJob' -e 'register' -e 'update' -e 'delete'
Length of output: 115
foo@bar:~$ wasmrollappd tx cron register-cron [name] [description] [contract_address] [json_msg] | ||
``` | ||
|
||
e.g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the abbreviation for "example."
- e.g
+ e.g.,
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.
e.g | |
e.g., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! left some comments. Let's make sure we're on the same page for the way cron jobs are created/updated/deleted. We think it should be through governance proposals instead of permissioned messages
// description is the description of the cron job | ||
string description = 3; | ||
// Msgs that will be executed every period amount of time | ||
repeated MsgContractCron msg_contract_cron = 4 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add an enabled
field here, like suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
(gogoproto.moretags) = "yaml:\"security_address\"" | ||
]; | ||
// set module enabled or not | ||
bool enable_cron = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by adding the enabled
field to CronJob
I believe we can get rid of this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
x/cron/client/cli/query.go
Outdated
) | ||
|
||
// GetQueryCmd returns the cli query commands for this module | ||
func GetQueryCmd(queryRoute string) *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queryRoute
parameter not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
x/cron/keeper/genesis.go
Outdated
) | ||
for _, item := range genState.CronJobs { | ||
k.SetCronJob(ctx, item) | ||
// Set the cronID to the cronID in the whitelisted contract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rectified
x/cron/keeper/keeper.go
Outdated
Keeper struct { | ||
cdc codec.BinaryCodec | ||
storeKey storetypes.StoreKey | ||
memKey storetypes.StoreKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memKey
is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
x/cron/keeper/store.go
Outdated
|
||
func (k Keeper) GetCronJob(ctx sdk.Context, cronID uint64) (cron types.CronJob, found bool) { | ||
var ( | ||
store = k.Store(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
x/cron/keeper/store.go
Outdated
} | ||
|
||
func (k Keeper) GetCronJobs(ctx sdk.Context) (crons []types.CronJob) { | ||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
|
||
var _ types.MsgServer = msgServer{} | ||
|
||
func (k msgServer) RegisterCron(goCtx context.Context, msg *types.MsgRegisterCron) (*types.MsgRegisterCronResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please make RegisterCron
, UpdateCronJob
and DeleteCronJob
go through governance proposals instead of being permissioned, as suggested previously
x/cron/types/keys.go
Outdated
|
||
var ( | ||
CronJobKeyPrefix = []byte{0x11} | ||
CronIDKey = []byte{0x12} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be LastCronIDKey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
x/cron/types/types.go
Outdated
errors "github.com/cosmos/cosmos-sdk/types/errors" | ||
) | ||
|
||
func NewWhitelistContract(cronId uint64, name, description string, msgs []MsgContractCron) CronJob { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
hey @zale144, Regarding cron job management, we understand the suggestion for governance proposals. However, we believe maintaining a permissioned approach provides several advantages for our specific use case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
x/cron/types/cron.pb.go
is excluded by!**/*.pb.go
x/cron/types/params.pb.go
is excluded by!**/*.pb.go
x/cron/types/tx.pb.go
is excluded by!**/*.pb.go
Files selected for processing (22)
- app/app.go (11 hunks)
- proto/wasmrollapp/cron/v1beta1/cron.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/params.proto (1 hunks)
- proto/wasmrollapp/cron/v1beta1/tx.proto (1 hunks)
- x/cron/abci.go (1 hunks)
- x/cron/client/cli/query.go (1 hunks)
- x/cron/client/cli/tx.go (1 hunks)
- x/cron/handler.go (1 hunks)
- x/cron/keeper/genesis.go (1 hunks)
- x/cron/keeper/keeper.go (1 hunks)
- x/cron/keeper/msg_server.go (1 hunks)
- x/cron/keeper/params_test.go (1 hunks)
- x/cron/keeper/store.go (1 hunks)
- x/cron/module.go (1 hunks)
- x/cron/spec/01_concepts.md (1 hunks)
- x/cron/spec/02_state.md (1 hunks)
- x/cron/spec/03_clients.md (1 hunks)
- x/cron/types/codec.go (1 hunks)
- x/cron/types/keys.go (1 hunks)
- x/cron/types/message_toggle_cron_job.go (1 hunks)
- x/cron/types/params.go (1 hunks)
- x/cron/types/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (12)
- proto/wasmrollapp/cron/v1beta1/cron.proto
- proto/wasmrollapp/cron/v1beta1/tx.proto
- x/cron/abci.go
- x/cron/client/cli/query.go
- x/cron/keeper/genesis.go
- x/cron/keeper/keeper.go
- x/cron/keeper/params_test.go
- x/cron/keeper/store.go
- x/cron/spec/03_clients.md
- x/cron/types/codec.go
- x/cron/types/keys.go
- x/cron/types/types.go
Additional context used
LanguageTool
x/cron/spec/01_concepts.md
[uncategorized] ~17-
17: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...ion] [contract_address] [json_msg]e.g
console foo@bar:
[uncategorized] ~38-
38: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...[id] [contract_address] [json_msg]e.g
console foo@bar:
[uncategorized] ~58-
58: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...e-cron-job [id] [contract_address]e.g
console foo@bar:
[uncategorized] ~77-
77: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...lappd tx cron toggle-cron-job [id]e.g
console foo@bar:
[uncategorized] ~88-~88: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...nly the security address are authorized can toggle the cron job
Additional comments not posted (31)
proto/wasmrollapp/cron/v1beta1/params.proto (1)
8-15
: The definition of theParams
message with thesecurity_address
field is well-structured and correctly annotated with serialization tags.x/cron/handler.go (1)
13-35
: TheNewHandler
function is implemented correctly with comprehensive handling for different cron message types and appropriate error handling for unrecognized messages.x/cron/types/message_toggle_cron_job.go (1)
9-53
: TheMsgToggleCronJob
message and its associated methods are correctly implemented with robust validation checks in theValidateBasic
method.x/cron/types/params.go (1)
9-69
: TheParams
struct and its associated methods are well-implemented, providing robust parameter management and validation.x/cron/client/cli/tx.go (1)
32-144
: Ensure error handling and validation are robust.The CLI commands should robustly handle errors and validate inputs to prevent issues during runtime. Please verify that all error handling and validation patterns are comprehensive and consistent across different commands.
Verification successful
The review comment suggests ensuring that error handling and validation are robust in the CLI commands. The verification script output shows many instances of error handling (
if err != nil
) across various files, includingx/cron/client/cli/tx.go
.The specific functions in
x/cron/client/cli/tx.go
handle errors for client context retrieval, argument parsing, and message validation consistently. Each function usescobra.ExactArgs
to validate the number of arguments, and they all handle errors using the patternif err != nil
.Given this consistent pattern of error handling and validation across the functions, it appears that the review comment's concern has been addressed.
- : The error handling and validation in the CLI commands are robust and consistent across the different functions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the robustness of error handling and validation in CLI commands. # Test: Search for error handling and validation patterns. Expect: Comprehensive patterns. rg --type go $'if err != nil'Length of output: 7745
x/cron/module.go (17)
44-47
: The methodName
correctly returns the module name from the types package, ensuring consistency across the module's usage.
49-51
: Properly registers legacy Amino codec for backward compatibility.
53-56
: Interfaces are correctly registered, ensuring that the module's types are recognized by the Cosmos SDK interface registry.
58-61
: Default genesis state is correctly marshaled to JSON, ensuring that the module can be initialized correctly with default values.
63-69
: Validates the genesis state effectively, ensuring that the cron module's initial state adheres to expected structures and constraints.
77-79
: Properly registers gRPC Gateway routes, facilitating the exposure of module functionalities over HTTP/JSON.
83-85
: CLI transaction commands are correctly sourced from thecli
package, enhancing modularity and separation of concerns.
87-89
: CLI query commands are correctly set up, ensuring that users can interact with the module via command-line interfaces.
96-102
: TheAppModule
struct is properly defined with necessary dependencies injected, such asKeeper
andContractOpsKeeper
, ensuring that the module has all necessary tools for operation.
104-114
: The constructor forAppModule
is correctly implemented, initializing all properties with provided arguments, which promotes good encapsulation and initialization practices.
116-118
: Correctly retrieves the name of the module, ensuring consistency and proper integration with the Cosmos SDK's module management system.
121-129
: Properly registers message and query services for the module, ensuring that it can handle incoming transactions and queries.
134-143
: Initializes the genesis state for the module correctly and ensures no validator updates are needed at this stage, which is typical for most modules unless they directly affect consensus parameters.
146-150
: The export of the genesis state is correctly implemented, ensuring that the module's state can be exported and imported, which is crucial for blockchain upgrades and migrations.
155-158
: Correctly implements theBeginBlock
logic, which is crucial for the cron module's functionality to trigger scheduled tasks.
160-163
: EndBlock logic is correctly set to do nothing, which is acceptable as not all modules need to perform actions at the end of a block.
171-174
: Correctly sets up routing for module messages, ensuring that messages directed to the cron module are processed by the appropriate handler.app/app.go (9)
Line range hint
130-152
: The addition ofcrontypes.StoreKey
tokvstorekeys
is correctly implemented, ensuring that the cron module has its dedicated storage key for maintaining state.
198-198
: The cron module is correctly added to theModuleBasics
, which is crucial for initializing basic module functionalities.
214-214
: The module account permissions for the cron module are correctly set tonil
, indicating that it does not require any specific permissions. This is appropriate given the current scope of the module.
496-501
: The initialization ofCronKeeper
is correctly performed with all necessary dependencies, ensuring that the cron module can operate effectively within the app.
574-574
: The cron module is correctly added to the application's module manager, which is essential for its operation and integration into the app lifecycle.
605-605
: The cron module is correctly positioned in thebeginBlockersList
, ensuring that its tasks are executed at the beginning of each block as intended.
630-630
: The cron module is correctly included in theendBlockersList
. While it currently does not perform actions at the end of blocks, its inclusion is necessary for future extensibility.
661-661
: The cron module is correctly included in theinitGenesisList
, ensuring that its initial state is set up correctly when the blockchain is initialized.
994-994
: The parameter subspace for the cron module is correctly initialized, ensuring that it can manage its configuration parameters effectively.
## State Objects | ||
|
||
The `x/cron` module keeps the following object in the state: CronJob. | ||
|
||
This object is used to store the state of a | ||
|
||
- `CronJob` - to store the details of the cron jobs | ||
|
||
```go | ||
// this defines the details of the cronjob | ||
message CronJob { | ||
// id is the unique identifier for the cron job | ||
uint64 id = 1; | ||
// name is the name of the cron job | ||
string name = 2; | ||
// description is the description of the cron job | ||
string description = 3; | ||
// Msgs that will be executed every period amount of time | ||
repeated MsgContractCron msg_contract_cron = 4 [(gogoproto.nullable) = false]; | ||
// set cron enabled or not | ||
bool enable_cron = 5; | ||
} | ||
``` | ||
|
||
```go | ||
message MsgContractCron { | ||
// Contract is the address of the smart contract | ||
string contract_address = 1; | ||
// Msg is json encoded message to be passed to the contract | ||
string json_msg = 2; | ||
} | ||
``` | ||
|
||
## Genesis & Params | ||
|
||
The `x/cron` module's `GenesisState` defines the state necessary for initializing the chain from a previously exported height. It contains the module Parameters and Cron jobs. The params are used to control the Security Address which is responsible to register cron operations. This value can be modified with a governance proposal. | ||
|
||
```go | ||
// GenesisState defines the cron module's genesis state. | ||
message GenesisState { | ||
Params params = 1 [ | ||
(gogoproto.moretags) = "yaml:\"params\"", | ||
(gogoproto.nullable) = false | ||
]; | ||
repeated CronJob cron_jobs = 2 [ | ||
(gogoproto.moretags) = "yaml:\"cron_jobs\"", | ||
(gogoproto.nullable) = false | ||
]; | ||
} | ||
``` | ||
|
||
```go | ||
// Params defines the parameters for the module. | ||
message Params { | ||
// Security address that can whitelist/delist contract | ||
repeated string security_address = 1 [ | ||
(gogoproto.jsontag) = "security_address,omitempty", | ||
(gogoproto.moretags) = "yaml:\"security_address\"" | ||
]; | ||
} | ||
``` | ||
|
||
## State Transitions | ||
|
||
The following state transitions are possible: | ||
|
||
- Register the cron job | ||
- Update the cron job | ||
- Delete the cron job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure comprehensive test coverage for state transitions.
The documentation highlights key state transitions such as registering, updating, and deleting cron jobs. It's crucial to ensure these transitions are covered by unit tests to maintain robustness. Would you like assistance in writing these tests or should I open a GitHub issue to track this task?
// AppModuleBasic implements the AppModuleBasic interface for the capability module. | ||
type AppModuleBasic struct { | ||
cdc codec.Codec | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description in the comment for AppModuleBasic
incorrectly references the "capability module." This should be updated to accurately reflect the cron module.
The cron module provides functionality for scheduling and executing tasks, including executing sudo contract calls during specific phases, such as begin blockers. By integrating scheduled contract executions, `x/cron` enhances the functionality of smart contracts, ensuring critical operations are performed automatically and reliably. | ||
Developers can register their contracts as a cron job if the address is whitelisted in the module parameters. Cron job can be deleted/updated if is no longer needed | ||
|
||
### Registering a Cron | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron register-cron [name] [description] [contract_address] [json_msg] | ||
``` | ||
|
||
e.g | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron register-cron cronjob1 "the is the 1st cron job" rol14hj2tavq8f.... {"msg_cron":{}} 100000000awasm --from cooluser --chain-id test-1 | ||
``` | ||
|
||
In the above tx - | ||
|
||
- `name` - name of the cron job | ||
- `description` - description of the cron job | ||
- `contract address` - CosmWasm contract address. | ||
- `json_msg` - sudo msg of the contract in json format | ||
|
||
> Note : only the security address authorized can register the contract | ||
|
||
### Delete cron job | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron update-cron-job [id] [contract_address] [json_msg] | ||
``` | ||
|
||
e.g | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron update-cron-job 1 rol14hj2tavq8f.... {"msg_new_cron":{}} 100000000awasm --from cooluser --chain-id test-1 | ||
``` | ||
|
||
In the above tx - | ||
|
||
- `id` - id of the cron job | ||
- `contract address` - CosmWasm contract address. | ||
- `json_msg` - sudo msg of the contract in json format | ||
|
||
> Note : only the security address are authorized can update the cron job | ||
|
||
### Update cron job | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron delete-cron-job [id] [contract_address] | ||
``` | ||
|
||
e.g | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron delete-cron-job 1 rol14hj2tavq8f.... 100000000awasm --from cooluser --chain-id test-1 | ||
``` | ||
|
||
In the above tx - | ||
|
||
- `id` - id of the cron job | ||
- `contract address` - CosmWasm contract address. | ||
|
||
> Note : only the security address are authorized can delete the cron job | ||
|
||
### Toggle cron job | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron toggle-cron-job [id] | ||
``` | ||
|
||
e.g | ||
|
||
```console | ||
foo@bar:~$ wasmrollappd tx cron toggle-cron-job 1 100000000awasm --from cooluser --chain-id test-1 | ||
``` | ||
|
||
In the above tx - | ||
|
||
- `id` - id of the cron job | ||
|
||
> Note : only the security address are authorized can toggle the cron job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify authorization requirements and correct abbreviation usage.
- e.g
+ e.g.,
- only the security address are authorized can toggle the cron job
+ only the security address and contract admin who are authorized can toggle the cron job
The documentation should clearly state the authorization requirements for operations and use the correct abbreviation format for "e.g.,".
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.
The cron module provides functionality for scheduling and executing tasks, including executing sudo contract calls during specific phases, such as begin blockers. By integrating scheduled contract executions, `x/cron` enhances the functionality of smart contracts, ensuring critical operations are performed automatically and reliably. | |
Developers can register their contracts as a cron job if the address is whitelisted in the module parameters. Cron job can be deleted/updated if is no longer needed | |
### Registering a Cron | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-cron [name] [description] [contract_address] [json_msg] | |
``` | |
e.g | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-cron cronjob1 "the is the 1st cron job" rol14hj2tavq8f.... {"msg_cron":{}} 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `name` - name of the cron job | |
- `description` - description of the cron job | |
- `contract address` - CosmWasm contract address. | |
- `json_msg` - sudo msg of the contract in json format | |
> Note : only the security address authorized can register the contract | |
### Delete cron job | |
```console | |
foo@bar:~$ wasmrollappd tx cron update-cron-job [id] [contract_address] [json_msg] | |
``` | |
e.g | |
```console | |
foo@bar:~$ wasmrollappd tx cron update-cron-job 1 rol14hj2tavq8f.... {"msg_new_cron":{}} 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `id` - id of the cron job | |
- `contract address` - CosmWasm contract address. | |
- `json_msg` - sudo msg of the contract in json format | |
> Note : only the security address are authorized can update the cron job | |
### Update cron job | |
```console | |
foo@bar:~$ wasmrollappd tx cron delete-cron-job [id] [contract_address] | |
``` | |
e.g | |
```console | |
foo@bar:~$ wasmrollappd tx cron delete-cron-job 1 rol14hj2tavq8f.... 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `id` - id of the cron job | |
- `contract address` - CosmWasm contract address. | |
> Note : only the security address are authorized can delete the cron job | |
### Toggle cron job | |
```console | |
foo@bar:~$ wasmrollappd tx cron toggle-cron-job [id] | |
``` | |
e.g | |
```console | |
foo@bar:~$ wasmrollappd tx cron toggle-cron-job 1 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `id` - id of the cron job | |
> Note : only the security address are authorized can toggle the cron job | |
The cron module provides functionality for scheduling and executing tasks, including executing sudo contract calls during specific phases, such as begin blockers. By integrating scheduled contract executions, `x/cron` enhances the functionality of smart contracts, ensuring critical operations are performed automatically and reliably. | |
Developers can register their contracts as a cron job if the address is whitelisted in the module parameters. Cron job can be deleted/updated if is no longer needed | |
### Registering a Cron | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-cron [name] [description] [contract_address] [json_msg] | |
``` | |
e.g., | |
```console | |
foo@bar:~$ wasmrollappd tx cron register-cron cronjob1 "the is the 1st cron job" rol14hj2tavq8f.... {"msg_cron":{}} 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `name` - name of the cron job | |
- `description` - description of the cron job | |
- `contract address` - CosmWasm contract address. | |
- `json_msg` - sudo msg of the contract in json format | |
> Note : only the security address authorized can register the contract | |
### Delete cron job | |
```console | |
foo@bar:~$ wasmrollappd tx cron update-cron-job [id] [contract_address] [json_msg] | |
``` | |
e.g., | |
```console | |
foo@bar:~$ wasmrollappd tx cron update-cron-job 1 rol14hj2tavq8f.... {"msg_new_cron":{}} 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `id` - id of the cron job | |
- `contract address` - CosmWasm contract address. | |
- `json_msg` - sudo msg of the contract in json format | |
> Note : only the security address are authorized can update the cron job | |
### Update cron job | |
```console | |
foo@bar:~$ wasmrollappd tx cron delete-cron-job [id] [contract_address] | |
``` | |
e.g., | |
```console | |
foo@bar:~$ wasmrollappd tx cron delete-cron-job 1 rol14hj2tavq8f.... 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `id` - id of the cron job | |
- `contract address` - CosmWasm contract address. | |
> Note : only the security address are authorized can delete the cron job | |
### Toggle cron job | |
```console | |
foo@bar:~$ wasmrollappd tx cron toggle-cron-job [id] | |
``` | |
e.g., | |
```console | |
foo@bar:~$ wasmrollappd tx cron toggle-cron-job 1 100000000awasm --from cooluser --chain-id test-1 | |
``` | |
In the above tx - | |
- `id` - id of the cron job | |
> Note : only the security address and contract admin who are authorized can toggle the cron job |
Tools
LanguageTool
[uncategorized] ~17-
17: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...ion] [contract_address] [json_msg]e.g
console foo@bar:
[uncategorized] ~38-
38: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...[id] [contract_address] [json_msg]e.g
console foo@bar:
[uncategorized] ~58-
58: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...e-cron-job [id] [contract_address]e.g
console foo@bar:
[uncategorized] ~77-
77: The abbreviation “e.g.” (= for example) requires two periods. (E_G)$ wasmrollappd tx ...
Context: ...lappd tx cron toggle-cron-job [id]e.g
console foo@bar:
[uncategorized] ~88-~88: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ...nly the security address are authorized can toggle the cron job
} | ||
// create a struct of type MsgContractCron | ||
msgContractCron := types.MsgContractCron{ | ||
ContractAddress: msg.ContractAddress, | ||
JsonMsg: msg.JsonMsg, | ||
} | ||
cronId := k.GetCronID(ctx) | ||
cron := types.CronJob{ | ||
Id: cronId + 1, | ||
Name: msg.Name, | ||
Description: msg.Description, | ||
MsgContractCron: []types.MsgContractCron{msgContractCron}, | ||
EnableCron: true, | ||
} | ||
k.SetCronJob(ctx, cron) | ||
k.SetCronID(ctx, cronId+1) | ||
return &types.MsgRegisterCronResponse{}, nil | ||
} | ||
|
||
func (k msgServer) UpdateCronJob(goCtx context.Context, msg *types.MsgUpdateCronJob) (*types.MsgUpdateCronJobResponse, error) { | ||
ctx := sdk.UnwrapSDKContext(goCtx) | ||
if err := msg.ValidateBasic(); err != nil { | ||
ctx.Logger().Error(fmt.Sprintf("request invalid: %s", err)) | ||
return &types.MsgUpdateCronJobResponse{}, err | ||
} | ||
// check if sender is authorized | ||
exists := k.CheckSecurityAddress(ctx, msg.SecurityAddress) | ||
if !exists { | ||
return &types.MsgUpdateCronJobResponse{}, sdkerrors.ErrUnauthorized | ||
} | ||
// Get the cron job | ||
cronJob, found := k.GetCronJob(ctx, msg.Id) | ||
if !found { | ||
return &types.MsgUpdateCronJobResponse{}, errorsmod.Wrapf(sdkerrors.ErrNotFound, "cron job not found") | ||
} | ||
for _, cron := range cronJob.MsgContractCron { | ||
if cron.ContractAddress == msg.ContractAddress { | ||
return &types.MsgUpdateCronJobResponse{}, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "contract address already exists") | ||
} | ||
} | ||
cronJob.MsgContractCron = append(cronJob.MsgContractCron, types.MsgContractCron{ | ||
ContractAddress: msg.ContractAddress, | ||
JsonMsg: msg.JsonMsg, | ||
}) | ||
k.SetCronJob(ctx, cronJob) | ||
return &types.MsgUpdateCronJobResponse{}, nil | ||
} | ||
|
||
func (k msgServer) DeleteCronJob(goCtx context.Context, msg *types.MsgDeleteCronJob) (*types.MsgDeleteCronJobResponse, error) { | ||
ctx := sdk.UnwrapSDKContext(goCtx) | ||
if err := msg.ValidateBasic(); err != nil { | ||
ctx.Logger().Error(fmt.Sprintf("request invalid: %s", err)) | ||
return &types.MsgDeleteCronJobResponse{}, err | ||
} | ||
// check if sender is authorized | ||
exists := k.CheckSecurityAddress(ctx, msg.SecurityAddress) | ||
if !exists { | ||
return &types.MsgDeleteCronJobResponse{}, sdkerrors.ErrUnauthorized | ||
} | ||
// Get the cron job | ||
cronJob, found := k.GetCronJob(ctx, msg.Id) | ||
if !found { | ||
return &types.MsgDeleteCronJobResponse{}, errorsmod.Wrapf(sdkerrors.ErrNotFound, "cron job not found") | ||
} | ||
// check if contract address exists in the cron job | ||
var foundContract bool | ||
for i, cron := range cronJob.MsgContractCron { | ||
if cron.ContractAddress == msg.ContractAddress { | ||
cronJob.MsgContractCron = append(cronJob.MsgContractCron[:i], cronJob.MsgContractCron[i+1:]...) | ||
foundContract = true | ||
break | ||
} | ||
} | ||
if !foundContract { | ||
return &types.MsgDeleteCronJobResponse{}, errorsmod.Wrapf(sdkerrors.ErrNotFound, "contract address not found") | ||
} | ||
k.SetCronJob(ctx, cronJob) | ||
return &types.MsgDeleteCronJobResponse{}, nil | ||
} | ||
|
||
func (k msgServer) ToggleCronJob(goCtx context.Context, msg *types.MsgToggleCronJob) (*types.MsgToggleCronJobResponse, error) { | ||
ctx := sdk.UnwrapSDKContext(goCtx) | ||
if err := msg.ValidateBasic(); err != nil { | ||
ctx.Logger().Error(fmt.Sprintf("request invalid: %s", err)) | ||
return &types.MsgToggleCronJobResponse{}, err | ||
} | ||
// check if sender is authorized | ||
exists := k.CheckSecurityAddress(ctx, msg.SecurityAddress) | ||
if !exists { | ||
return &types.MsgToggleCronJobResponse{}, sdkerrors.ErrUnauthorized | ||
} | ||
// Get the cron job | ||
cronJob, found := k.GetCronJob(ctx, msg.Id) | ||
if !found { | ||
return &types.MsgToggleCronJobResponse{}, errorsmod.Wrapf(sdkerrors.ErrNotFound, "cron job not found") | ||
} | ||
cronJob.EnableCron = !cronJob.EnableCron | ||
k.SetCronJob(ctx, cronJob) | ||
return &types.MsgToggleCronJobResponse{}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review authorization logic and error messages.
The authorization checks and error messages need to be consistent and informative. Ensure that the error messages provide clear feedback to the user and that the authorization logic is securely implemented to prevent unauthorized access.
Hi, thank you for the fixes. So, for the cron job management, the issue is that the implications of having permissioned cron job management is that it can leave room for malicious behavior (deliberately or not). For instance, the "admin" is free to register any kind of smart contract as a cron job, potentially one harmful to the users. |
hey @zale144 , Thanks for the feedback
This dual approach balances security and responsiveness, ensuring that the system remains robust and user-centric. Awaiting for your take on this.... |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Description
Closes #XXX
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.
PR review checkboxes:
I have...
Unreleased
section inCHANGELOG.md
godoc
commentsSDK Checklist
map
time.Now()
sendCoin
and notSendCoins
Full security checklist here
----;
For Reviewer:
---;
After reviewer approval:
Summary by CodeRabbit
New Features
Documentation