feat(x/accounts): wire x/accounts to simapp #18253
Conversation
# Conflicts: # x/accounts/go.mod # x/accounts/go.sum
WalkthroughThe changes primarily revolve around the introduction of a new module called "accounts" and the modification of existing modules to accommodate this new addition. The "accounts" module provides functionality for managing user accounts, including creating and querying accounts. The changes also involve the introduction of new interfaces, types, and functions, as well as modifications to existing ones to improve the overall structure and functionality of the codebase. Changes
TipsChat with CodeRabbit Bot (
|
|
@coderabbitai review |
| @@ -0,0 +1,34 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
this is a test account protofile, to use for simd testing
| @@ -23,3 +27,40 @@ message AccountQueryResponse { | |||
| // response defines the query response of the account. | |||
| bytes response = 1; | |||
| } | |||
|
|
|||
| // SchemaResponse is the response type for the Query/Schema RPC method. | |||
| message SchemaRequest { | |||
There was a problem hiding this comment.
these are approximate schema definitions which represent the init/query/exec messages an account can process.
| appCodec.InterfaceRegistry().SigningContext(), | ||
| app.MsgServiceRouter(), | ||
| app.GRPCQueryRouter(), | ||
| accountstd.AddAccount("counter", counter.NewAccount), |
There was a problem hiding this comment.
this is what we do to wire an account, we give it a name and pass in an account creator function in this case counter.NewAccount
There was a problem hiding this comment.
would it be better to have the creation of the keeper and the adding of accounts as two separate steps?
There was a problem hiding this comment.
This would only be needed if say you have keeperA depending on accountsKeeper and then accountA depending on keeperA, this is a pattern that I think we want to remove from the SDK.
| var _ accountstd.Interface = (*Account)(nil) | ||
|
|
||
| // NewAccount creates a new account. | ||
| func NewAccount(d accountstd.Dependencies) (Account, error) { |
There was a problem hiding this comment.
this is a test account implementation, used in simapp for some manual testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files ignored due to filter (7)
- api/cosmos/accounts/v1/query_grpc.pb.go
- simapp/go.mod
- simapp/go.sum
- x/accounts/go.mod
- x/accounts/go.sum
- x/accounts/v1/query.pb.go
- x/accounts/v1/tx.pb.go
Files selected for processing (26)
- api/cosmos/accounts/v1/tx.pulsar.go (2 hunks)
- proto/cosmos/accounts/testing/counter/v1/counter.proto (1 hunks)
- proto/cosmos/accounts/v1/query.proto (2 hunks)
- proto/cosmos/accounts/v1/tx.proto (3 hunks)
- simapp/app.go (11 hunks)
- simapp/app_test.go (1 hunks)
- x/accounts/account_test.go (2 hunks)
- x/accounts/accountstd/exports.go (1 hunks)
- x/accounts/cli/cli.go (1 hunks)
- x/accounts/genesis_test.go (1 hunks)
- x/accounts/internal/implementation/api_builder.go (3 hunks)
- x/accounts/internal/implementation/context.go (4 hunks)
- x/accounts/internal/implementation/context_test.go (3 hunks)
- x/accounts/internal/implementation/implementation.go (4 hunks)
- x/accounts/internal/implementation/implementation_test.go (2 hunks)
- x/accounts/internal/implementation/intermodule.go (1 hunks)
- x/accounts/internal/implementation/protoaccount.go (5 hunks)
- x/accounts/keeper.go (4 hunks)
- x/accounts/keeper_test.go (6 hunks)
- x/accounts/module.go (1 hunks)
- x/accounts/msg_server.go (4 hunks)
- x/accounts/msg_server_test.go (1 hunks)
- x/accounts/query_server.go (2 hunks)
- x/accounts/query_server_test.go (1 hunks)
- x/accounts/testing/counter/counter.go (1 hunks)
- x/accounts/v1/schema.go (1 hunks)
Files skipped from review due to trivial changes (4)
- api/cosmos/accounts/v1/tx.pulsar.go
- x/accounts/accountstd/exports.go
- x/accounts/genesis_test.go
- x/accounts/internal/implementation/intermodule.go
Additional comments: 75
x/accounts/account_test.go (2)
7-7: The import
"cosmossdk.io/x/accounts/accountstd"has been added. Ensure that this package is being used in the code and that it is necessary for the functionality of the program.19-22: The function signature of
NewTestAccounthas been changed to include anaccountstd.Dependenciesparameter and it now returns an error as well. Make sure that all calls to this function throughout the codebase have been updated to match the new signature. Also, ensure that the error returned by this function is being handled properly where this function is called.- func NewTestAccount(sb *collections.SchemaBuilder) *TestAccount { + func NewTestAccount(d accountstd.Dependencies) (*TestAccount, error) {proto/cosmos/accounts/testing/counter/v1/counter.proto (1)
- 1-34: The protobuf definitions for the messages and responses look good. They are well-structured and the field names are descriptive. The comments provide a clear understanding of what each message and response does. However, it would be beneficial to add more comments to the
MsgInit,MsgIncreaseCounter, andQueryCounterRequestmessages to explain what they do and how they are used. This will improve the maintainability of the code by making it easier for other developers to understand the purpose of these messages.For example, you could add comments like these:
// MsgInit is used to initialize the counter with a specific value. message MsgInit { // initial_value is the initial amount to set the counter to. uint64 initial_value = 1; } // MsgIncreaseCounter is used to increase the counter by a specific amount. message MsgIncreaseCounter { // amount is the amount to increase the counter by. uint64 amount = 1; } // QueryCounterRequest is used to request the current value of the counter. message QueryCounterRequest {}x/accounts/internal/implementation/api_builder.go (3)
31-32: The
decodeRequestandencodeResponsefunctions have been removed and replaced with aschemafield of typeHandlerSchema. This change simplifies the code by removing the need to manually encode and decode requests and responses. However, it's important to ensure that theHandlerSchematype and its methods are correctly implemented to handle the encoding and decoding of requests and responses. Also, verify that all places whereInitBuilderis used have been updated to reflect this change.47-48: The
ExecuteBuilderstruct now includes aHandlerSchemafield. This is a map that associates message types with their corresponding schemas. This change allows for more flexible handling of different message types, as each type can have its own schema. However, ensure that the schemas are correctly defined and associated with their corresponding message types.57-59: The
ExecuteBuilderstruct now includes aHandlerSchemafield. This is a map that associates message types with their corresponding schemas. This change allows for more flexible handling of different message types, as each type can have its own schema. However, ensure that the schemas are correctly defined and associated with their corresponding message types.proto/cosmos/accounts/v1/tx.proto (2)
7-12: The import statement for "cosmos/msg/v1/msg.proto" and the service option
(cosmos.msg.v1.service) = true;have been added. This indicates that the service is now using the Cosmos SDK's service module, which provides a more streamlined way to define gRPC services and handlers. Ensure that the service handlers are properly implemented in the corresponding Go code.22-23: The option
(cosmos.msg.v1.signer) = "sender";has been added to theMsgInitmessage. This indicates that thesenderfield is the signer of the message. This is a part of the Cosmos SDK's service module and is used for authentication. Ensure that thesenderfield is properly used in the corresponding Go code.44:
The option(cosmos.msg.v1.signer) = "sender";has been added to theMsgExecutemessage. This indicates that thesenderfield is the signer of the message. This is a part of the Cosmos SDK's service module and is used for authentication. Ensure that thesenderfield is properly used in the corresponding Go code.proto/cosmos/accounts/v1/query.proto (5)
11-14: The new RPC methods
SchemaandAccountTypehave been added to theQueryservice. Ensure that these methods are implemented in the server and that they handle errors correctly. Also, verify that the client-side code has been updated to call these new methods as needed.32-35: The
SchemaRequestmessage has been added. It contains a single fieldaccount_typewhich is used to specify the account type for which the schema is being requested. Ensure that the account type is validated in the server-side implementation of theSchemaRPC method.37-54: The
SchemaResponsemessage has been added. It contains fields for the init schema, execute handlers, and query handlers. Each handler is represented by aHandlermessage which contains the request and response names. Ensure that these fields are populated correctly in the server-side implementation of theSchemaRPC method.57-60: The
AccountTypeRequestmessage has been added. It contains a single fieldaddresswhich is used to specify the address for which the account type is being requested. Ensure that the address is validated in the server-side implementation of theAccountTypeRPC method.63-66: The
AccountTypeResponsemessage has been added. It contains a single fieldaccount_typewhich is used to specify the account type for the requested address. Ensure that this field is populated correctly in the server-side implementation of theAccountTypeRPC method.x/accounts/cli/cli.go (3)
151-162: The
getSchemaForAccountfunction is clear and concise. Good job!204-216: The
encodeJSONToProtofunction is clear and concise. Good job!218-235: The
decodeProtoToJSONfunction is clear and concise. Good job!x/accounts/keeper_test.go (7)
7-10: The import
"cosmossdk.io/x/accounts/accountstd"has been added. Ensure that this package is being used in the code and that it is necessary for the functionality of the tests.29-35: The function signature for
newKeeperhas been changed. It now accepts a variadic parameter of typeimplementation.AccountCreatorFuncinstead of a map ofimplementation.Account. Ensure that all calls tonewKeeperhave been updated to match the new function signature.38-44: The
queryModuleFuncnow requires two parameters:reqandresp. Ensure that this change is reflected in all places wherequeryModuleFuncis used.73-74: The
queryModuleFuncnow requires two parameters:reqandresp. Ensure that this change is reflected in all places wherequeryModuleFuncis used.93-99: The
execModuleFuncnow requires two parameters:msgandmsgResp. Ensure that this change is reflected in all places whereexecModuleFuncis used.114-116: The
queryModuleFuncnow requires two parameters:reqandresp. Ensure that this change is reflected in all places wherequeryModuleFuncis used.138-148: The
queryModuleFuncnow requires two parameters:reqandresp. Ensure that this change is reflected in all places wherequeryModuleFuncis used.x/accounts/internal/implementation/context_test.go (3)
9-9: The import
"google.golang.org/protobuf/runtime/protoiface"is new. Ensure that it is used in the code and is not an unused import.58-62: The function signature for the module call has been changed. The return type of the function has been changed from
(proto.Message, error)toerror. The function now modifies themsgRespargument in-place instead of returning a newproto.Message. This change could potentially improve performance by avoiding unnecessary memory allocations, but it also introduces the risk of side effects ifmsgRespis used elsewhere after this function call. Ensure that this change does not introduce any unintended side effects.- func(ctx context.Context, msg proto.Message) (proto.Message, error) { + func(ctx context.Context, msg, msgResp protoiface.MessageV1) error {
- 71-74: Similar to the previous comment, the function signature for the query call has been changed. The return type of the function has been changed from
(proto.Message, error)toerror. The function now modifies therespargument in-place instead of returning a newproto.Message. This change could potentially improve performance by avoiding unnecessary memory allocations, but it also introduces the risk of side effects ifrespis used elsewhere after this function call. Ensure that this change does not introduce any unintended side effects.- func(ctx context.Context, msg proto.Message) (proto.Message, error) { + func(ctx context.Context, req, resp protoiface.MessageV1) error {simapp/app_test.go (1)
- 61-61: The
AccountKeeperhas been replaced withAuthKeeperfor getting the module address. Ensure that theAuthKeeperis properly initialized and has the same functionality as theAccountKeeperfor getting the module address.- addr = app.AccountKeeper.GetModuleAddress(acc) + addr = app.AuthKeeper.GetModuleAddress(acc)Committable suggestion (Beta)
if modAddr, err := sdk.AccAddressFromBech32(acc); err == nil { addr = modAddr } else { addr = app.AuthKeeper.GetModuleAddress(acc) }x/accounts/internal/implementation/implementation.go (6)
11-15: The
Dependenciesstruct is introduced to encapsulate dependencies that are passed to the constructor of a smart account. This is a good practice as it improves code readability and maintainability.17-33: The
AddAccountfunction is introduced as a helper function to add a smart account to the list of smart accounts. It takes a name and a constructor function as arguments and returns a function that creates an account and its implementation. This is a good use of higher-order functions and improves code modularity.35-61: The
MakeAccountsMapfunction is introduced to create a map of account names to their implementations. It takes an address codec and a slice of account creator functions as arguments. The function iterates over the account creator functions, creates an account and its implementation for each one, builds the state schema, and adds the account to the map. This function improves code modularity and readability.92-95: The
Implementationstruct is updated to include schemas for collections, init handlers, query handlers, and execute handlers. This is a good practice as it improves code readability and maintainability.112-119: The
Implementationstruct is updated to include schemas for collections, init handlers, query handlers, and execute handlers. This is a good practice as it improves code readability and maintainability.134-159: The
MessageSchemaandHandlerSchemastructs are introduced to define the schemas of a message and a handler respectively. This is a good practice as it improves code readability and maintainability.x/accounts/internal/implementation/implementation_test.go (3)
73-79: The test case "decode init request - ok" has been modified to use the
InitHandlerSchema.ResponseSchema.TxEncodeandInitHandlerSchema.RequestSchema.TxDecodemethods instead ofproto.Marshalandimpl.DecodeInitRequest. Ensure that these changes are intentional and that the new methods provide the same functionality as the old ones. Also, verify that theInitHandlerSchemaand itsResponseSchemaandRequestSchemaare properly initialized before this test is run.70-87: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [81-90]
The test case "encode init response - ok" has been modified to use the
InitHandlerSchema.ResponseSchema.TxEncodemethod instead ofimpl.EncodeInitResponse. Ensure that this change is intentional and that the new method provides the same functionality as the old one. Also, verify that theInitHandlerSchemaand itsResponseSchemaare properly initialized before this test is run.
- 93-95: The test case "decode execute request - ok" remains unchanged. However, ensure that the
anypb.Newfunction is still appropriate for creating a newanypb.Anymessage fromwantReq, and thatwantReqis correctly initialized.x/accounts/keeper.go (6)
10-13: The new imports
cosmossdk.io/x/accounts/accountstd,github.com/cosmos/cosmos-sdk/codec/types, andgoogle.golang.org/protobuf/runtime/protoifacehave been added. Ensure that these packages are used in the code and are necessary for the new changes.30-46: New interfaces
QueryRouter,MsgRouter, andSignerProviderhave been introduced. These interfaces seem to provide a more modular approach to routing queries and messages, and getting signers from a message. This is a good practice as it allows for better separation of concerns and easier testing.48-102: The
NewKeeperfunction has been modified to take the new interfaces as parameters and use them in theKeeperstruct. This is a significant change and it's important to ensure that all calls toNewKeeperthroughout the codebase have been updated to match the new signature. Also, the error handling in this function seems to be appropriate, as it returns errors when they occur and doesn't suppress them.111-112: The types of
execModuleFuncandqueryModuleFunchave been changed toimplementation.ModuleExecFuncandimplementation.ModuleQueryFuncrespectively. Ensure that these new types are compatible with the rest of the codebase and that they provide the necessary functionality.259-264: The
queryModuleFuncis now being used in theNewQueryServerfunction. This seems to be a good change as it allows for more flexibility in handling queries. However, ensure that thequeryModuleFuncis properly initialized before it's used in this function.266-268: The new function
getMessageNamehas been added. This function seems to be used to get the name of a message from its type URL. This could be a useful utility function, but ensure that it's used correctly throughout the codebase.x/accounts/internal/implementation/context.go (5)
8-9: The import
"google.golang.org/protobuf/runtime/protoiface"is new. Ensure that this import is used in the code and is necessary. If it's not used, it should be removed to keep the code clean.21-33: The function signatures for
ModuleExecFuncandModuleQueryFunchave been changed. The return type of these functions has been changed from(proto.Message, error)toerror. This change simplifies the function signatures and makes them easier to use, but it also means that the functions no longer return aproto.Message. Ensure that this change does not break any existing code that relies on the returnedproto.Message.49-50: The function signatures for
moduleExecandmoduleQueryinMakeAccountContexthave been updated to match the newModuleExecFuncandModuleQueryFunctypes. This is consistent with the changes made to thecontextValuestruct.77-84: The
ExecModulefunction has been updated to match the newModuleExecFunctype. The function now uses therespvariable to store the response fromv.moduleExec, and it no longer checks whetherrespis of typeRespProto. This simplifies the function and makes it easier to use, but it also means that the function no longer checks the type of the response. Ensure that this change does not introduce any type-related errors.87-96: The
QueryModulefunction has been updated to match the newModuleQueryFunctype. The function now uses therespvariable to store the response fromv.moduleQuery, and it no longer checks whetherrespis of typeRespProto. This simplifies the function and makes it easier to use, but it also means that the function no longer checks the type of the response. Ensure that this change does not introduce any type-related errors.x/accounts/msg_server_test.go (2)
20-20: The
newKeeperfunction now takes anaccountstd.AddAccountfunction as an argument instead of a map. This change simplifies the process of adding accounts for testing. However, ensure that theAddAccountfunction is correctly implemented and that it properly adds the account to the keeper.21-25: The
queryModuleFunchas been modified to use theprotoiface.MessageV1interface for the request and response parameters instead ofproto.Message. This change provides more flexibility asprotoiface.MessageV1includes methods for working with protocol buffer messages. However, ensure that theproto.Mergefunction is used correctly to merge the response message with an instance ofbankv1beta1.QueryBalanceResponse. The function should not return any errors if the merge is successful.x/accounts/msg_server.go (4)
7-7: The line
var _ v1.MsgServer = msgServer{}is a compile-time check to ensure thatmsgServerimplements thev1.MsgServerinterface. IfmsgServerdoes not implement the interface, the code will not compile. This is a good practice to ensure that the necessary methods are implemented.32-35: The method
InitHandlerSchema.RequestSchema.TxDecode(request.Message)is used to decode the request message. This is a change from the previous methodimpl.DecodeInitRequest(request.Message). Ensure that the new method correctly decodes the request message and that the decoded message is used correctly in the subsequent code.44-47: The method
InitHandlerSchema.ResponseSchema.TxEncode(resp)is used to encode the response. This is a change from the previous methodimpl.EncodeInitResponse(resp). Ensure that the new method correctly encodes the response and that the encoded response is used correctly in the subsequent code.55-59: The event
account_creationis emitted with the attributeaddressset toaccAddrString. This is a new addition and it's a good practice to emit events for significant actions in the code. However, ensure that the event is handled correctly in the rest of the codebase.x/accounts/query_server_test.go (2)
19-20: The
newKeeperfunction now takes anaccountstd.AddAccountfunction as an argument instead of a map. This change seems to be in line with the newx/accountsmodule's design. However, ensure that theAddAccountfunction is correctly implemented and that it properly initializes theKeeperstruct.20-21: The
queryModuleFuncnow returnsnilinstead of aQueryBalanceResponsemessage. This change might be due to the new design of thex/accountsmodule. However, ensure that this change does not affect the functionality of thequeryModuleFuncand that it still correctly handles queries.x/accounts/query_server.go (1)
- 72-84: The
AccountTypemethod retrieves the account type for a given address. It converts the address from a string to bytes, retrieves the account type from theAccountsByTypestorage, and returns it. If any of these operations fail, it returns an error. This method seems to be implemented correctly.x/accounts/internal/implementation/protoaccount.go (5)
7-11: The import
"google.golang.org/protobuf/encoding/protojson"has been added. This package provides functionality for marshalling and unmarshalling protobuf messages to and from JSON. This is likely used for the newNewProtoMessageSchemafunction which includes JSON encoding and decoding.23-28: The
respNamevariable has been removed from theRegisterInitHandlerfunction. This variable was previously used to get the full name of the response protobuf message. It seems like the response name is no longer needed in this context.35-37: The
router.decodeRequestandrouter.encodeResponsefunctions have been removed and replaced with arouter.schemaassignment. Therouter.schemais assigned aHandlerSchemastruct which includes aRequestSchemaandResponseSchema. These schemas are created using the newNewProtoMessageSchemafunction. This change seems to be a part of a larger refactoring to use schemas instead of separate decode and encode functions.61-64: The
router.HandlerSchemamap is being updated with a newHandlerSchemafor the request name. This is similar to the change in theRegisterInitHandlerfunction where aHandlerSchemais assigned torouter.schema. It seems like theHandlerSchemais being used to store the request and response schemas for different types of requests.75-115: The
NewProtoMessageSchemafunction has been added. This function creates a newMessageSchemafor a given protobuf message type. TheMessageSchemaincludes the name of the message and functions for encoding and decoding the message to and from bytes and JSON. The function uses theproto.MarshalOptionsandproto.UnmarshalOptionsstructs from thegoogle.golang.org/protobuf/protopackage for byte encoding and decoding, and theprotojson.MarshalOptionsandprotojson.UnmarshalOptionsstructs from thegoogle.golang.org/protobuf/encoding/protojsonpackage for JSON encoding and decoding. TheDiscardUnknownoption is set totruefor both byte and JSON unmarshalling, which means that unknown fields in the input will be ignored. This might be a potential issue if the input is expected to strictly adhere to the message schema.78:
TheDiscardUnknownoption is set totruefor byte unmarshalling. This means that unknown fields in the input will be ignored. If the input is expected to strictly adhere to the message schema, this might be a potential issue. The comment on this line suggests that this is a point of uncertainty. It would be good to clarify this point and make a decision based on the requirements of the application.85:
TheDiscardUnknownoption is set totruefor JSON unmarshalling. This means that unknown fields in the input will be ignored. If the input is expected to strictly adhere to the message schema, this might be a potential issue. It would be good to clarify this point and make a decision based on the requirements of the application.simapp/app.go (11)
18-20: The new imports
cosmossdk.io/x/accounts,cosmossdk.io/x/accounts/accountstd, andcosmossdk.io/x/accounts/testing/counterare added. Ensure that these packages are used in the code and are necessary for the new functionality.151-152: The
AccountKeeperis replaced withAccountsKeeperandAuthKeeper. Make sure that all references toAccountKeeperin the code are updated to useAccountsKeeperorAuthKeeperas appropriate.262-262: The
accounts.StoreKeyis added to the list of store keys. Ensure that this key is used in the code and is necessary for the new functionality.285-299: The
AuthKeeperandAccountsKeeperare initialized. TheAuthKeeperis initialized with the same parameters as the oldAccountKeeper. TheAccountsKeeperis initialized with a newaccountstd.AddAccountfunction. Ensure that this function is defined and works as expected.366-368: The
AuthKeeperis used instead of theAccountKeeperto initialize thegovKeeper. Ensure that theAuthKeeperprovides the same functionality as theAccountKeeperin this context.379-383: The
AuthKeeperis used instead of theAccountKeeperto initialize theevidenceKeeper. Ensure that theAuthKeeperprovides the same functionality as theAccountKeeperin this context.398-419: The
accounts.NewAppModule(app.AccountsKeeper)andauth.NewAppModule(appCodec, app.AuthKeeper, authsims.RandomGenesisAccounts)are added to theModuleManager. Ensure that these modules are necessary and work as expected.467-468: The
accounts.ModuleNameandauthtypes.ModuleNameare added to thegenesisModuleOrder. Ensure that these modules are necessary and work as expected.481-483: The error handling for
app.ModuleManager.RegisterServices(app.configurator)is changed. The error is now handled immediately instead of being returned by the function. This change is acceptable if the function does not need to return an error.506-508: The
auth.NewAppModule(app.appCodec, app.AuthKeeper, authsims.RandomGenesisAccounts)is used instead ofauth.NewAppModule(app.appCodec, app.AccountKeeper, authsims.RandomGenesisAccounts)to initialize theoverrideModules. Ensure that theAuthKeeperprovides the same functionality as theAccountKeeperin this context.563-566: The
AuthKeeperis used instead of theAccountKeeperto initialize theanteHandler. Ensure that theAuthKeeperprovides the same functionality as theAccountKeeperin this context.x/accounts/testing/counter/counter.go (3)
- 37-47: func (a Account) Init(ctx context.Context, msg *counterv1.MsgInit) (*counterv1.MsgInitResponse, error) {
err := a.Owner.Set(ctx, accountstd.Sender(ctx))
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to set owner: %w", err)
err = a.Counter.Set(ctx, msg.InitialValue)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to set counter: %w", err)
return &counterv1.MsgInitResponse{}, nil
}
49-70: func (a Account) IncreaseCounter(ctx context.Context, msg *counterv1.MsgIncreaseCounter) (*counterv1.MsgIncreaseCounterResponse, error) {
sender := accountstd.Sender(ctx)
owner, err := a.Owner.Get(ctx)
if err != nil {
return nil, err
}
if !bytes.Equal(sender, owner) {
return nil, fmt.Errorf("sender is not the owner of the account")
}
counter, err := a.Counter.Get(ctx)
if err != nil {
return nil, err
}
counter = counter + msg.Amount
err = a.Counter.Set(ctx, counter)
if err != nil {
return nil, fmt.Errorf("failed to increase counter: %w", err)
}
return &counterv1.MsgIncreaseCounterResponse{
NewAmount: counter,
}, nil
}31-35: type Account struct {
// Owner is the address of the account owner.
- Owner collections.Item[[]byte]
- Owners collections.Item[[]byte]
// Counter is the counter value.
- Counter collections.Item[uint64]
- Counters collections.Item[uint64]
}<!-- suggestion_start --> <details> <summary>Committable suggestion (Beta)</summary> ```suggestion package counter import ( "bytes" "context" "fmt" counterv1 "cosmossdk.io/api/cosmos/accounts/testing/counter/v1" "cosmossdk.io/collections" "cosmossdk.io/x/accounts/accountstd" ) var ( OwnerPrefix = collections.NewPrefix(0) CounterPrefix = collections.NewPrefix(1) ) var _ accountstd.Interface = (*Account)(nil) // NewAccount creates a new account. func NewAccount(d accountstd.Dependencies) (Account, error) { return Account{ Owners: collections.NewItem(d.SchemaBuilder, OwnerPrefix, "owner", collections.BytesValue), Counters: collections.NewItem(d.SchemaBuilder, CounterPrefix, "counter", collections.Uint64Value), }, nil } // Account implements the Account interface. It is an account // who can be used to increase a counter. type Account struct { // Owners is the address of the account owner. Owners collections.Item[[]byte] // Counters is the counter value. Counters collections.Item[uint64] } func (a Account) Init(ctx context.Context, msg *counterv1.MsgInit) (*counterv1.MsgInitResponse, error) { err := a.Owners.Set(ctx, accountstd.Sender(ctx)) if err != nil { return nil, fmt.Errorf("failed to set owner: %w", err) } err = a.Counters.Set(ctx, msg.InitialValue) if err != nil { return nil, fmt.Errorf("failed to set counter: %w", err) } return &counterv1.MsgInitResponse{}, nil } func (a Account) IncreaseCounter(ctx context.Context, msg *counterv1.MsgIncreaseCounter) (*counterv1.MsgIncreaseCounterResponse, error) { sender := accountstd </blockquote></details></blockquote></details> </details>
# Conflicts: # simapp/app.go # simapp/go.sum # x/accounts/go.mod # x/accounts/go.sum
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (5)
- simapp/go.mod
- tests/go.mod
- tests/starship/tests/go.mod
- x/accounts/go.mod
- x/accounts/go.sum
Files selected for processing (3)
- x/accounts/keeper.go (3 hunks)
- x/accounts/keeper_test.go (7 hunks)
- x/accounts/testing/counter/counter.go (1 hunks)
Additional comments: 25
x/accounts/keeper.go (5)
8-21: The new imports seem to be used correctly in the code. Ensure that the versions of these packages are compatible with the rest of the project.
32-48: The new interfaces
QueryRouter,MsgRouter, andSignerProviderare well defined. Make sure that these interfaces are implemented wherever necessary.50-108: The
NewKeeperfunction has been significantly modified. The new parameterses,signerProvider,execRouter, andqueryRouterare used correctly. Theaccountsparameter has been removed, ensure that this does not break any existing functionality. TheKeeperstruct has been updated to include the neweventServicefield and the function signatures ofgetSenderFunc,execModuleFunc, andqueryModuleFunchave been updated to useprotoiface.MessageV1instead ofproto.Message. The new functiongetMessageNameis used correctly.110-120: The
Keeperstruct has been updated to include the neweventServicefield and the function signatures ofgetSenderFunc,execModuleFunc, andqueryModuleFunchave been updated to useprotoiface.MessageV1instead ofproto.Message.271-273: The new function
getMessageNameis used correctly.x/accounts/testing/counter/counter.go (11)
1-11: The import statements look good and are well organized.
14-16: The global variables
OwnerPrefixandCounterPrefixare defined correctly.18-18: The assertion that
Accountimplements theaccountstd.Interfaceis correct.20-26: The
NewAccountfunction is well implemented. It creates a newAccountstruct withOwnerandCounterfields.28-35: The
Accountstruct is well defined withOwnerandCounterfields.37-47: The
Initmethod is correctly implemented. It sets theOwnerandCounterfields of theAccountstruct.49-70: The
IncreaseCountermethod is correctly implemented. It increases theCounterfield of theAccountstruct if the sender is the owner of the account.72-80: The
QueryCountermethod is correctly implemented. It returns theCounterfield of theAccountstruct.82-84: The
RegisterInitHandlermethod is correctly implemented. It registers theInitmethod as the init handler.86-88: The
RegisterExecuteHandlersmethod is correctly implemented. It registers theIncreaseCountermethod as the execute handler.90-92: The
RegisterQueryHandlersmethod is correctly implemented. It registers theQueryCountermethod as the query handler.x/accounts/keeper_test.go (9)
6-12: The import of
google.golang.org/protobuf/runtime/protoifaceis new. Ensure that it is used in the code and is necessary.30-43: The
eventServicestruct and its methods are new. Ensure that they are used correctly and that the methods are implemented according to theevent.Serviceinterface.44-50: The
newKeeperfunction now accepts a variadic parameter ofAccountCreatorFunc. Ensure that this change is reflected in all calls tonewKeeper.52-62: The
TestKeeper_Initfunction has been updated to use theaccountstd.AddAccountfunction to add an account. Ensure that this change is reflected in all calls toTestKeeper_Init.53-60: The
m.queryModuleFuncfunction signature has been updated to accept a response message as a parameter and return an error. Ensure that this change is reflected in all calls tom.queryModuleFunc.87-92: The
TestKeeper_Executefunction has been updated to use theaccountstd.AddAccountfunction to add an account. Ensure that this change is reflected in all calls toTestKeeper_Execute.108-115: The
m.execModuleFuncfunction signature has been updated to accept a response message as a parameter and return an error. Ensure that this change is reflected in all calls tom.execModuleFunc.128-133: The
TestKeeper_Queryfunction has been updated to use theaccountstd.AddAccountfunction to add an account. Ensure that this change is reflected in all calls toTestKeeper_Query.150-164: The
m.queryModuleFuncfunction signature has been updated to accept a response message as a parameter and return an error. Also, the function now merges the response message with the copied response. Ensure that this change is reflected in all calls tom.queryModuleFunc.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- simapp/app_test.go (3 hunks)
- x/accounts/internal/implementation/implementation.go (4 hunks)
- x/accounts/module.go (1 hunks)
Additional comments: 11
simapp/app_test.go (3)
8-11: The new import
"cosmossdk.io/x/accounts"has been added. Ensure that the package exists and is accessible.59-63: The
AccountKeeperhas been replaced withAuthKeeper. Ensure that all instances ofAccountKeeperhave been replaced withAuthKeeperthroughout the codebase.197-203: The
accountsmodule has been added to themodule.VersionMapin theTestRunMigrationsfunction. Ensure that theaccountsmodule has aConsensusVersionmethod and that it returns the correct version.x/accounts/module.go (1)
- 1-100: The
AppModulestruct and its methods are well defined and follow the Cosmos SDK's module structure. The methods are implemented according to the interfaces they are supposed to satisfy. TheNewAppModulefunction correctly initializes anAppModuleinstance with aKeeper. TheRegisterInterfaces,RegisterServices,DefaultGenesis,InitGenesis,ExportGenesis,Name,GetTxCmd,GetQueryCmd, andConsensusVersionmethods are all correctly implemented. TheStoreKeyis correctly prefixed to avoid collision with theauthstore key. TheConsensusVersionis set to 1, which is the default value for a new module.x/accounts/internal/implementation/implementation.go (7)
1-9: The new import statements are correctly placed and formatted.
11-15: The
Dependenciesstruct is a good way to encapsulate dependencies for creating a smart account. It includes aSchemaBuilderandAddressCodec.17-18: The
AccountCreatorFunctype is a function type that creates an account. This is a good way to encapsulate the logic for creating an account.20-35: The
AddAccountfunction is a helper function to add a smart account to the list of smart accounts. It returns a function that given anAccountimplementer, returns the name of the account and theImplementationinstance. This is a good way to encapsulate the logic for adding an account.93-102: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [67-102]
The
NewImplementationfunction creates a newImplementationinstance given anAccountimplementer. This is a good way to encapsulate the logic for creating anImplementationinstance.
113-128: The
Implementationstruct is updated to include new fields for schema and handler schemas. This is a good way to encapsulate the schema and handler schemas for anImplementation.157-163: The
HandlerSchemastruct is a good way to encapsulate the schema of a handler. It includes fields for the request schema and the response schema.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (7)
- simapp/go.mod
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- x/accounts/go.mod
- x/accounts/go.sum
Files selected for processing (1)
- simapp/app_test.go (3 hunks)
Additional comments: 3
simapp/app_test.go (3)
16-22: The new import for the "accounts" module is correctly added.
59-65: The change from
app.AccountKeeper.GetModuleAddress(acc)toapp.AuthKeeper.GetModuleAddress(acc)is correctly implemented. Ensure that theAuthKeeperhas theGetModuleAddressmethod and it behaves as expected.197-203: The addition of the "accounts" module to the
module.VersionMapin theRunMigrationsfunction is correctly implemented. Ensure that theaccounts.AppModule{}.ConsensusVersion()returns the correct version number.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/accounts/internal/implementation/implementation.go (4 hunks)
Additional comments: 6
x/accounts/internal/implementation/implementation.go (6)
1-67: The
Dependenciesstruct,AccountCreatorFunctype, andAddAccountfunction are well-defined and follow good practices. TheAddAccountfunction is particularly interesting as it uses a type parameterAwhich is constrained to theAccountinterface. This is a good use of Go's type parameters feature introduced in Go 1.18.20-35: The
AddAccountfunction is a higher-order function that returns a function. This is a good use of functional programming principles in Go. However, it's important to note that this could potentially increase the complexity of the code and make it harder to understand for developers who are not familiar with functional programming.37-64: The
MakeAccountsMapfunction is well-defined and follows good practices. It creates a map of account names to account implementations from a list of account creator functions. This is a good use of Go's map data structure and functional programming principles.92-101: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [66-101]
The
NewImplementationfunction is well-defined and follows good practices. It creates a newImplementationinstance given anAccountimplementer. However, it's important to note that this function has a high cyclomatic complexity due to the multiple error checks. Consider refactoring this function to reduce its complexity.
112-127: The
Implementationstruct is well-defined and follows good practices. It wraps anAccountimplementer in order to provide a concrete and non-generic implementation usable by thex/accountsmodule. However, the TODO comment on line 124 suggests that theDecodeExecuteRequestandEncodeExecuteResponsefields should be removed and replaced with the schemas. This should be addressed in a future commit.137-154: The
MessageSchemaandHandlerSchemastructs are well-defined and follow good practices. They define the schema of a message and a handler, respectively. However, the comments "CONSENSUS SAFE" and "CONSENSUS UNSAFE" are not clear. Consider adding more explanation about what these comments mean.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # tests/starship/tests/go.mod
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- simapp/go.mod
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- x/accounts/go.mod
Files selected for processing (2)
- x/accounts/internal/implementation/implementation.go (4 hunks)
- x/accounts/module.go (1 hunks)
Additional comments: 15
x/accounts/module.go (11)
1-19: The import section looks clean and organized. All the necessary packages are imported for the module.
21-24: The constants are well defined and follow the naming conventions. The comment on line 23 provides a good explanation for the choice of the
StoreKeyvalue.30-36: The
AppModulestruct is correctly implementing the required interfaces. This is a good practice to ensure that the struct meets the necessary contract.38-40: The
NewAppModulefunction is correctly implemented and returns an instance ofAppModule.52-54: The
RegisterInterfacesfunction correctly registers the message service descriptor.60-63: The
RegisterServicesfunction correctly registers the query and message servers.67-69: The
DefaultGenesisfunction correctly returns the default genesis state.71-78: The
ValidateGenesisfunction correctly unmarshals the genesis state and validates it. However, the comment on line 76 indicates that additional validation logic should be added. Ensure that this is implemented as necessary.80-87: The
InitGenesisfunction correctly imports the genesis state. However, it usespanicto handle errors. As discussed in the previous comments, this is acceptable due to the constraints of the genesis API.89-95: The
ExportGenesisfunction correctly exports the genesis state. It also usespanicto handle errors, which is acceptable in this context.97-107: The
Name,GetTxCmd,GetQueryCmd, andConsensusVersionfunctions are correctly implemented.x/accounts/internal/implementation/implementation.go (4)
1-9: The new import statements are appropriate for the added functionality.
11-15: The
Dependenciesstruct is a good way to encapsulate dependencies for creating an account.17-19: The
AccountCreatorFunctype is a clear way to define a function that creates an account.112-127: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [112-134]
The
Implementationstruct is a good way to wrap an Account implementer in order to provide a concrete and non-generic implementation usable by the x/accounts module. However, the TODO comment on line 124 suggests that theDecodeExecuteRequest,EncodeExecuteResponse,DecodeQueryRequest, andEncodeQueryResponsefields may be removed in the future. This could potentially break any code that relies on these fields.
| DecodeInitRequest func([]byte) (any, error) | ||
| // EncodeInitResponse encodes an init response to be sent back from the message server. | ||
| EncodeInitResponse func(any) ([]byte, error) | ||
| // TODO: remove these fields and use the schemas instead |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- x/accounts/go.mod
- x/accounts/go.sum
Files selected for processing (1)
- simapp/app.go (14 hunks)
Additional comments: 7
simapp/app.go (7)
20-28: The import statements for the
accountspackage and its related modules are correct.149-156: The addition of the
AccountsKeeperfield and renaming of theAccountKeeperfield toAuthKeeperare correct. Ensure that all references to these fields throughout the codebase have been updated accordingly.260-263: The addition of the
accounts.StoreKeyto the list of store keys is correct.286-301: The initialization of the
AccountsKeeperwith a new instance ofaccounts.Keeperis done correctly. However, the error handling could be improved. Instead of panicking, consider returning the error to the caller for better error propagation and handling. This was previously discussed and it was decided to keep the panic as the function signature does not allow returning errors.413-438: The addition of the
accounts.NewAppModuleto the list of app modules is correct. Ensure that all necessary dependencies for theaccountsmodule are correctly initialized before this point.521-527: The addition of the
accounts.ModuleNameto thegenesisModuleOrderis correct. Ensure that the order of modules in this list is correct as it can affect the initialization of the application.578-584: The replacement of
app.AccountKeeperwithapp.AuthKeeperin theNewAnteHandlerfunction call is correct. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features:
accountsmodule to manage user accounts, improving user management capabilities.Queryservice for querying account schemas and retrieving account types, enhancing data retrieval options.accountstdpackage that exports types and functions for implementing smart accounts, expanding the functionality for smart accounts.Refactor:
AuthKeeperinstead ofAccountKeeper, enhancing code consistency.proto.Messagewithprotoiface.MessageV1in various places, aligning with updated protobuf standards.Tests:
Chores:
cosmos/store/internal/kv/v1beta1/kv.protofile, preparing for future updates.