-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(accounts): Allow funds to be sent to accounts on init and execute #19360
Conversation
WalkthroughThe update enhances the system's fund management capabilities across various components. It introduces the handling of funds in messages and operations, improving account transactions and tests. Key changes include modifying message structures, adding functions for coin transfers, and updating tests. These changes empower the system to effectively manage and track funds within account operations, enhancing functionality and versatility. Changes
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 (
|
@testinginprod your pull request is missing a changelog! |
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 Status
Actionable comments generated: 30
Configuration used: .coderabbit.yml
Files selected for processing (18)
- api/cosmos/accounts/testing/counter/v1/counter.pulsar.go (17 hunks)
- api/cosmos/accounts/v1/tx.pulsar.go (31 hunks)
- proto/cosmos/accounts/testing/counter/v1/counter.proto (2 hunks)
- proto/cosmos/accounts/v1/tx.proto (3 hunks)
- simapp/app.go (3 hunks)
- tests/e2e/accounts/account_abstraction_test.go (1 hunks)
- tests/e2e/accounts/wiring_test.go (3 hunks)
- x/accounts/accountstd/exports.go (2 hunks)
- x/accounts/genesis_test.go (1 hunks)
- x/accounts/internal/implementation/context.go (9 hunks)
- x/accounts/internal/implementation/context_test.go (4 hunks)
- x/accounts/keeper.go (14 hunks)
- x/accounts/keeper_account_abstraction.go (3 hunks)
- x/accounts/keeper_test.go (5 hunks)
- x/accounts/msg_server.go (2 hunks)
- x/accounts/testing/counter/counter.go (2 hunks)
- x/accounts/testing/counter/v1/counter.pb.go (7 hunks)
- x/accounts/v1/tx.pb.go (12 hunks)
Files not summarized due to errors (2)
- api/cosmos/accounts/testing/counter/v1/counter.pulsar.go: Error: Message exceeds token limit
- api/cosmos/accounts/v1/tx.pulsar.go: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 34
proto/cosmos/accounts/testing/counter/v1/counter.proto (2)
- 7-8: Imports for
cosmos/base/v1beta1/coin.proto
andgogoproto/gogo.proto
are correctly added to support the newfunds
field.- 45-47: The addition of the
funds
field inMsgTestDependenciesResponse
is correctly defined with appropriate options for casting and nullability.tests/e2e/accounts/wiring_test.go (4)
- 9-11: Correctly added imports for
storetypes
andtestutil
to support new test scenarios involving gas meter and funds setup.- 26-26: Correctly added a context setup with a gas meter.
- 50-50: Casting to
int
for gas comparison is correctly implemented.- 60-65: Funds verification tests for creator and counter accounts are correctly added and implemented.
x/accounts/msg_server.go (2)
- 34-34: Correctly passing
request.Funds
tom.k.Init
to handle funds during account initialization.- 83-83: Correctly passing
execute.Funds
tom.k.Execute
to handle funds during account execution.proto/cosmos/accounts/v1/tx.proto (3)
- 10-11: Imports for
cosmos/base/v1beta1/coin.proto
andgogoproto/gogo.proto
are correctly added to support the newfunds
field.- 38-41: The addition of the
funds
field inMsgInit
is correctly defined with appropriate options for casting and nullability.- 61-64: The addition of the
funds
field inMsgExecute
is correctly defined with appropriate options for casting and nullability.x/accounts/testing/counter/counter.go (2)
- 63-63: Comment for checking funds is correctly added, but ensure the implementation actually checks the funds as expected.
- 118-127: Refactored gas meter handling and funds check are correctly implemented. Ensure the
expected funds
error is handled appropriately in the calling context.x/accounts/accountstd/exports.go (2)
- 10-10: Correctly imported
sdk "github.com/cosmos/cosmos-sdk/types"
for theFunds
function.- 81-83: The addition of the
Funds
function is correctly implemented to return funds sent during execute or init requests.x/accounts/internal/implementation/context.go (4)
- 29-29: Addition of
funds
field incontextValue
struct is correctly implemented to handle funds in the account context.- 36-42: Functions
addCtx
andgetCtx
are correctly implemented for manipulating context values.- 57-57: Correctly passing
funds
as a parameter inMakeAccountContext
function.- 138-138: The
Funds
function is correctly implemented to return funds from the execution context.x/accounts/keeper.go (4)
- 43-46:
CoinTransferMsgFunc
type definition is clear and well-documented.- 119-126:
Keeper
struct correctly includesmakeSendCoinsMsg
and adheres to the single responsibility principle by abstracting coin transfer logic.- 285-291: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [279-303]
makeAccountContext
method correctly handles the creation of context with funds for non-query operations. Ensurefunds
parameter is validated if necessary in future use cases.
- 392-410:
maybeSendFunds
method correctly checks ifamt
is non-zero before proceeding with fund transfer, adhering to early return pattern.simapp/app.go (1)
- 818-833: The implementation of
makeSendCoinsMsg
correctly constructs abanktypes.MsgSend
message. Ensure that error handling foraddressCodec.BytesToString
is adequately tested for bothfrom
andto
address conversions to prevent runtime errors.x/accounts/testing/counter/v1/counter.pb.go (5)
- 251-252: The addition of
Funds
field inMsgTestDependenciesResponse
struct aligns with the PR's objective to allow sending funds. Ensure corresponding protobuf changes are made.- 316-321: The
GetFunds
method correctly returns theFunds
field fromMsgTestDependenciesResponse
. This method is necessary for accessing the funds in a safe manner.- 604-617: The marshaling logic for the
Funds
field inMsgTestDependenciesResponse
appears correct, iterating over theFunds
slice in reverse to prepend eachCoin
to the buffer. This is a standard approach in protobuf-generated Go code.- 781-786: The
Size
method forMsgTestDependenciesResponse
correctly calculates the size of theFunds
field, iterating over eachCoin
to add its size. This is necessary for efficient buffer allocation during marshaling.- 1255-1288: The unmarshaling logic for the
Funds
field inMsgTestDependenciesResponse
correctly appends each unmarshaledCoin
to theFunds
slice. This ensures that the funds are accurately reconstructed from the protobuf data.x/accounts/v1/tx.pb.go (4)
- 43-45: The
Funds
field inMsgInit
struct is correctly added according to protobuf conventions, using therepeated
keyword for a slice ofCoin
and specifying the protobuf field number as4
.- 102-107: The
GetFunds
method forMsgInit
is implemented correctly, following Go best practices for getter methods.- 172-174: The
Funds
field inMsgExecute
struct is correctly added, following the same pattern as inMsgInit
.- 231-236: The
GetFunds
method forMsgExecute
follows Go conventions and correctly retrieves theFunds
field.api/cosmos/accounts/testing/counter/v1/counter.pulsar.go (1)
- 1941-1990: The implementation of
_MsgTestDependenciesResponse_5_list
directly manipulates the underlying slice in methods likeAppend
,Set
, andTruncate
. While this is generally acceptable, it's important to ensure that any modifications maintain the integrity of the data, especially when dealing with shared or concurrent access.Ensure that the modifications to the underlying slice in
_MsgTestDependenciesResponse_5_list
methods do not introduce data races or inconsistencies, particularly in concurrent scenarios.
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 Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (5)
- tests/e2e/accounts/wiring_test.go (3 hunks)
- x/accounts/accountstd/exports.go (2 hunks)
- x/accounts/internal/implementation/context.go (9 hunks)
- x/accounts/keeper.go (14 hunks)
- x/accounts/utils_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- tests/e2e/accounts/wiring_test.go
- x/accounts/accountstd/exports.go
- x/accounts/internal/implementation/context.go
Additional comments: 8
x/accounts/utils_test.go (2)
- 17-17: Importing
sdk "github.com/cosmos/cosmos-sdk/types"
is appropriate for the usage ofsdk.Coins
in the newly added functionnotImplementedMakeCoins
.- 52-52: The modification to
newKeeper
to usenotImplementedMakeCoins
aligns with the intention to handle coin capabilities in testing without introducing bank dependencies.x/accounts/keeper.go (6)
- 25-25: Importing
sdk "github.com/cosmos/cosmos-sdk/types"
is necessary for the usage ofsdk.Coins
in the newly introducedCoinTransferMsgFunc
and other methods handling funds.- 43-46: The introduction of
CoinTransferMsgFunc
is a good design choice, keepingx/accounts
independent of thebank
module by using a function type for coin transfers.- 86-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [79-101]
Adding
makeSendCoinsMsg
to theNewKeeper
constructor and theKeeper
struct is correct. Ensure thatmakeSendCoinsMsg
is validated for non-nil to prevent potential nil dereference issues, as previously noted.
- 168-180: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [152-175]
The modifications to the
Init
method to include funds processing are correct. Error handling formaybeSendFunds
is addressed, but consider logging the error for better traceability, as previously noted.
- 220-232: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [200-226]
The updates to the
Execute
method to handle funds transfer are correct. Similar to theInit
method, consider adding logging for errors inmaybeSendFunds
, as previously noted.
- 392-410: The implementation of
maybeSendFunds
correctly checks if the amount is non-zero before proceeding with the fund transfer. This method encapsulates the logic for sending funds efficiently and securely by ensuring that the "from" address cannot impersonate another account.
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.
Just curious if we could use cosmossdk.io/api/cosmos/bank instead and avoid the bank import that way in x/accounts but still have a good default.
simapp/app.go
Outdated
@@ -811,3 +814,21 @@ func BlockedAddresses() map[string]bool { | |||
|
|||
return modAccAddrs | |||
} | |||
|
|||
func makeSendCoinsMsg(addressCodec coreaddress.Codec) func([]byte, []byte, sdk.Coins) (protoiface.MessageV1, 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.
imho, this makes the wiring of this module quite weird.
can we add docs here on why its needed?
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.
Yes I do agree this makes things weird.
I think we can have a default which uses api/bank, but I was a little reluctant on relying on API considering it's future is not certain. |
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 Status
Actionable comments generated: 6
Configuration used: .coderabbit.yml
Files selected for processing (3)
- core/appmodule/module.go (2 hunks)
- x/accounts/coin_transfer.go (1 hunks)
- x/accounts/keeper.go (13 hunks)
Additional comments: 3
x/accounts/keeper.go (3)
- 43-46: The definition of
coinsTransferMsgFunc
is clear and well-documented. Ensure that the function type is used consistently throughout the module.- 167-179: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [151-174]
The
Init
method correctly handles fund transfers usingmaybeSendFunds
. Ensure error handling includes logging for better traceability, as previously noted.
- 219-231: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [199-225]
The
Execute
method correctly handles fund transfers usingmaybeSendFunds
. Similar toInit
, ensure error handling includes logging for errors.
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1" | ||
v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1" | ||
"cosmossdk.io/core/address" | ||
"cosmossdk.io/x/accounts/internal/implementation" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/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.
Consider grouping SDK-related imports together for better readability.
import (
"google.golang.org/protobuf/proto"
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1"
v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1"
"cosmossdk.io/core/address"
"cosmossdk.io/x/accounts/internal/implementation"
+
sdk "github.com/cosmos/cosmos-sdk/types"
)
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.
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1" | |
v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1" | |
"cosmossdk.io/core/address" | |
"cosmossdk.io/x/accounts/internal/implementation" | |
sdk "github.com/cosmos/cosmos-sdk/types" | |
import ( | |
"google.golang.org/protobuf/proto" | |
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1" | |
v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1" | |
"cosmossdk.io/core/address" | |
"cosmossdk.io/x/accounts/internal/implementation" | |
sdk "github.com/cosmos/cosmos-sdk/types" | |
) |
// protoV2GogoWrapper is a wrapper of a protov2 message into a gogo message. | ||
// this is exceptionally allowed to enable accounts to be decoupled from | ||
// the SDK, since x/accounts can support only protov1 in its APIs. | ||
// But in order to keep it decoupled from the SDK we need to use the API module. | ||
// This is a hack to make an API module type work in x/accounts. Once the SDK | ||
// has protov2 support, we can migrate internal/implementation/encoding.go to | ||
// work with protov2. |
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 comment explains a workaround for protobuf version compatibility. Ensure this is tracked for future removal once the SDK supports protov2 natively.
Would you like me to open a GitHub issue to track the removal of this workaround once protov2 support is available in the SDK?
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.
yes, please do!
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.
@testinginprod: I'm creating a GitHub issue to track the removal of the protobuf version compatibility workaround once protov2 support is available in the SDK. Here's the command I used to create the issue:
gh issue create --title "Track Removal of Protobuf Version Compatibility Workaround" --body "This issue tracks the removal of the protobuf version compatibility workaround implemented in `x/accounts/coin_transfer.go`. The workaround is necessary until the Cosmos SDK natively supports protov2.
The relevant discussion can be found in PR https://github.com/cosmos/cosmos-sdk/pull/19360 and the specific comment at https://github.com/cosmos/cosmos-sdk/pull/19360#discussion_r1481734092.
The workaround involves wrapping a protov2 message into a gogo message to decouple `x/accounts` from the SDK while maintaining compatibility. This should be revisited and potentially removed once the SDK has protov2 support." --repo cosmos/cosmos-sdk
This issue will ensure that the necessary follow-up is tracked and addressed in due course. Thank you for highlighting the importance of this task! 🐇
func defaultCoinsTransferMsgFunc(addrCdc address.Codec) coinsTransferMsgFunc { | ||
return func(from, to []byte, coins sdk.Coins) (implementation.ProtoMsg, implementation.ProtoMsg, error) { | ||
fromAddr, err := addrCdc.BytesToString(from) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
toAddr, err := addrCdc.BytesToString(to) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
v2Coins := make([]*v1beta1.Coin, len(coins)) | ||
for i, coin := range coins { | ||
v2Coins[i] = &v1beta1.Coin{ | ||
Denom: coin.Denom, | ||
Amount: coin.Amount.String(), | ||
} | ||
} | ||
return protoV2GogoWrapper{&bankv1beta1.MsgSend{ | ||
FromAddress: fromAddr, | ||
ToAddress: toAddr, | ||
Amount: v2Coins, | ||
}}, new(bankv1beta1.MsgSendResponse), 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 defaultCoinsTransferMsgFunc
function correctly translates SDK Coins
to protobuf Coin
messages and wraps the MsgSend
in a protoV2GogoWrapper
. However, ensure error messages are descriptive and consistent.
if err != nil {
- return nil, nil, err
+ return nil, nil, fmt.Errorf("error converting from address bytes to string: %w", err)
}
...
if err != nil {
- return nil, nil, err
+ return nil, nil, fmt.Errorf("error converting to address bytes to string: %w", err)
}
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
func defaultCoinsTransferMsgFunc(addrCdc address.Codec) coinsTransferMsgFunc { | |
return func(from, to []byte, coins sdk.Coins) (implementation.ProtoMsg, implementation.ProtoMsg, error) { | |
fromAddr, err := addrCdc.BytesToString(from) | |
if err != nil { | |
return nil, nil, err | |
} | |
toAddr, err := addrCdc.BytesToString(to) | |
if err != nil { | |
return nil, nil, err | |
} | |
v2Coins := make([]*v1beta1.Coin, len(coins)) | |
for i, coin := range coins { | |
v2Coins[i] = &v1beta1.Coin{ | |
Denom: coin.Denom, | |
Amount: coin.Amount.String(), | |
} | |
} | |
return protoV2GogoWrapper{&bankv1beta1.MsgSend{ | |
FromAddress: fromAddr, | |
ToAddress: toAddr, | |
Amount: v2Coins, | |
}}, new(bankv1beta1.MsgSendResponse), nil | |
} | |
func defaultCoinsTransferMsgFunc(addrCdc address.Codec) coinsTransferMsgFunc { | |
return func(from, to []byte, coins sdk.Coins) (implementation.ProtoMsg, implementation.ProtoMsg, error) { | |
fromAddr, err := addrCdc.BytesToString(from) | |
if err != nil { | |
return nil, nil, fmt.Errorf("error converting from address bytes to string: %w", err) | |
} | |
toAddr, err := addrCdc.BytesToString(to) | |
if err != nil { | |
return nil, nil, fmt.Errorf("error converting to address bytes to string: %w", err) | |
} | |
v2Coins := make([]*v1beta1.Coin, len(coins)) | |
for i, coin := range coins { | |
v2Coins[i] = &v1beta1.Coin{ | |
Denom: coin.Denom, | |
Amount: coin.Amount.String(), | |
} | |
} | |
return protoV2GogoWrapper{&bankv1beta1.MsgSend{ | |
FromAddress: fromAddr, | |
ToAddress: toAddr, | |
Amount: v2Coins, | |
}}, new(bankv1beta1.MsgSendResponse), nil | |
} | |
} |
// MsgHandlerRouter is implemented by the runtime provider. | ||
type MsgHandlerRouter interface { | ||
// RegisterHandler is called by modules to register msg handler functions. | ||
RegisterHandler(name string, handler func(ctx context.Context, msg protoiface.MessageV1) (msgResp protoiface.MessageV1, err error)) | ||
} | ||
|
||
// HasMsgHandler is implemented by modules that instead of exposing msg server expose | ||
// a set of handlers. | ||
type HasMsgHandler interface { | ||
// RegisterMsgHandlers is implemented by the module that will register msg handlers. | ||
RegisterMsgHandlers(router MsgHandlerRouter) | ||
} |
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 addition of MsgHandlerRouter
and HasMsgHandler
interfaces introduces a new pattern for registering message handlers. Ensure the documentation for these interfaces is clear and provides enough context for developers to understand their usage.
// MsgHandlerRouter is implemented by the runtime provider.
type MsgHandlerRouter interface {
// RegisterHandler is called by modules to register msg handler functions.
// The name parameter is the message type, and the handler function takes a context and a message,
// returning a message response and an error.
RegisterHandler(name string, handler func(ctx context.Context, msg protoiface.MessageV1) (msgResp protoiface.MessageV1, err error))
}
// HasMsgHandler is implemented by modules that, instead of exposing msg server, expose
// a set of handlers directly.
type HasMsgHandler interface {
// RegisterMsgHandlers is called during the module initialization phase to register the module's msg handlers.
// The router parameter is used to register each handler with its corresponding message type.
RegisterMsgHandlers(router MsgHandlerRouter)
}
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.
// MsgHandlerRouter is implemented by the runtime provider. | |
type MsgHandlerRouter interface { | |
// RegisterHandler is called by modules to register msg handler functions. | |
RegisterHandler(name string, handler func(ctx context.Context, msg protoiface.MessageV1) (msgResp protoiface.MessageV1, err error)) | |
} | |
// HasMsgHandler is implemented by modules that instead of exposing msg server expose | |
// a set of handlers. | |
type HasMsgHandler interface { | |
// RegisterMsgHandlers is implemented by the module that will register msg handlers. | |
RegisterMsgHandlers(router MsgHandlerRouter) | |
} | |
// MsgHandlerRouter is implemented by the runtime provider. | |
type MsgHandlerRouter interface { | |
// RegisterHandler is called by modules to register msg handler functions. | |
// The name parameter is the message type, and the handler function takes a context and a message, | |
// returning a message response and an error. | |
RegisterHandler(name string, handler func(ctx context.Context, msg protoiface.MessageV1) (msgResp protoiface.MessageV1, err error)) | |
} | |
// HasMsgHandler is implemented by modules that, instead of exposing msg server, expose | |
// a set of handlers directly. | |
type HasMsgHandler interface { | |
// RegisterMsgHandlers is called during the module initialization phase to register the module's msg handlers. | |
// The router parameter is used to register each handler with its corresponding message type. | |
RegisterMsgHandlers(router MsgHandlerRouter) | |
} |
makeSendCoinsMsg: defaultCoinsTransferMsgFunc(addressCodec), | ||
Schema: collections.Schema{}, | ||
AccountNumber: collections.NewSequence(sb, AccountNumberKey, "account_number"), | ||
AccountsByType: collections.NewMap(sb, AccountTypeKeyPrefix, "accounts_by_type", collections.BytesKey, collections.StringValue), | ||
AccountByNumber: collections.NewMap(sb, AccountByNumber, "account_by_number", collections.BytesKey, collections.Uint64Value), | ||
AccountsState: collections.NewMap(sb, implementation.AccountStatePrefix, "accounts_state", collections.PairKeyCodec(collections.Uint64Key, collections.BytesKey), collections.BytesValue), |
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 addition of makeSendCoinsMsg
and Schema
to the Keeper
struct is appropriate for handling fund transfers. However, ensure that Schema
is properly documented to clarify its purpose.
type Keeper struct {
...
+ // Schema represents the schema for the module, used for managing collections of data.
Schema collections.Schema
...
}
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.
makeSendCoinsMsg: defaultCoinsTransferMsgFunc(addressCodec), | |
Schema: collections.Schema{}, | |
AccountNumber: collections.NewSequence(sb, AccountNumberKey, "account_number"), | |
AccountsByType: collections.NewMap(sb, AccountTypeKeyPrefix, "accounts_by_type", collections.BytesKey, collections.StringValue), | |
AccountByNumber: collections.NewMap(sb, AccountByNumber, "account_by_number", collections.BytesKey, collections.Uint64Value), | |
AccountsState: collections.NewMap(sb, implementation.AccountStatePrefix, "accounts_state", collections.PairKeyCodec(collections.Uint64Key, collections.BytesKey), collections.BytesValue), | |
makeSendCoinsMsg: defaultCoinsTransferMsgFunc(addressCodec), | |
// Schema represents the schema for the module, used for managing collections of data. | |
Schema: collections.Schema{}, | |
AccountNumber: collections.NewSequence(sb, AccountNumberKey, "account_number"), | |
AccountsByType: collections.NewMap(sb, AccountTypeKeyPrefix, "accounts_by_type", collections.BytesKey, collections.StringValue), | |
AccountByNumber: collections.NewMap(sb, AccountByNumber, "account_by_number", collections.BytesKey, collections.Uint64Value), | |
AccountsState: collections.NewMap(sb, implementation.AccountStatePrefix, "accounts_state", collections.PairKeyCodec(collections.Uint64Key, collections.BytesKey), collections.BytesValue), |
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.
LGTM (nit: personally I'm not a fan of maybeSendFunds
but it's not exported so it's fine)
Description
When an account is created, or when an account is sent a msg, the creator or the sender are allowed to also provide funds to the account.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Refactor