feat(accounts): re-introduce bundler #21562
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Cosmos SDK's account management system, including the addition of a new message type Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
testinginprod
added
the
backport/v0.52.x
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| x.ExecutionGasLimit = value.Uint() | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.interfaces.account_abstraction.v1.TxExtension")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.interfaces.account_abstraction.v1.TxExtension")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.interfaces.account_abstraction.v1.TxExtension does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| "github.com/cosmos/cosmos-sdk/types/tx/signing" | ||
| ) | ||
|
|
||
| func protoV2TxToProtoV1(t *txv1beta1.Tx) *tx.Tx { |
There was a problem hiding this comment.
Why don't we just marshal and unmarshal? This is probably marginally faster but less safe.
testinginprod
requested review from
a team,
facundomedica,
sontrinh16 and
julienrbrt
as code owners
|
@testinginprod your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (22)
x/accounts/msg_server_test.go (1)
47-58: LGTM! Consider adding a descriptive comment.The new test function
TestMsgServer_BundlingDisabledis well-structured and effectively tests the behavior when bundling is disabled. It follows Go testing best practices and provides good coverage for this scenario.To improve clarity, consider adding a brief comment explaining the purpose of this test at the beginning of the function.
func TestMsgServer_BundlingDisabled(t *testing.T) { + // Test that attempting to execute a bundle when bundling is disabled returns ErrBundlingDisabled k, ctx := newKeeper(t, accountstd.AddAccount("test", NewTestAccount)) k.DisableTxBundling()x/accounts/README.md (2)
602-617: Minor style improvements suggestedConsider the following style improvements:
- Use consistent list item prefixes. Change the numbered list to use "1." for all items instead of "1.", "2.", "3.", "4.".
- For the nested list under
bundler_payment_messages, consider using asterisks (*) instead of dashes (-) for consistency with other unordered lists in the document.🧰 Tools
🪛 LanguageTool
[style] ~605-~605: To form a complete sentence, be sure to include a subject.
Context: ...uding NFTs, IBC tokens, or delegations. Can be empty if payment arrangements are ma...(MISSING_IT_THERE)
🪛 Markdownlint
602-602: Expected: 1; Actual: 2; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
608-608: Expected: 1; Actual: 3; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
613-613: Expected: 1; Actual: 4; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
605-605: Minor grammatical improvement suggestedTo improve clarity, consider rephrasing the sentence:
- Can be empty if payment arrangements are made off-chain or if the bundler doesn't require payment. + It can be empty if payment arrangements are made off-chain or if the bundler doesn't require payment.This change forms a complete sentence and improves readability.
🧰 Tools
🪛 LanguageTool
[style] ~605-~605: To form a complete sentence, be sure to include a subject.
Context: ...uding NFTs, IBC tokens, or delegations. Can be empty if payment arrangements are ma...(MISSING_IT_THERE)
x/accounts/protoutils.go (2)
40-46: Consider inlining simple functionsThe function
protoV2TimestampToV1is simple and used only once. Consider inlining it to reduce redundancy.You can replace its usage directly in
protoV2TxToProtoV1:- timeoutTimestamp := protoV2TimestampToV1(t.Body.TimeoutTimestamp) + var timeoutTimestamp *time.Time + if t.Body.TimeoutTimestamp != nil { + ts := t.Body.TimeoutTimestamp.AsTime() + timeoutTimestamp = &ts + }And remove the
protoV2TimestampToV1function if it's no longer needed.
6-8: Group standard library imports separatelyAccording to the Uber Go Style Guide, standard library imports should be grouped separately from third-party imports.
Reorganize the imports:
import ( "time" + "errors" + "google.golang.org/protobuf/types/known/anypb" "google.golang.org/protobuf/types/known/timestamppb" // ... )x/accounts/proto/cosmos/accounts/v1/tx.proto (1)
84-104: Enhance comments and formatting forBundledTxResponsemessageThe comments for the
BundledTxResponsemessage are misaligned and could be improved for clarity. According to Protobuf style guidelines, comments should be placed directly above the message or field they describe, without extra indentation. Additionally, providing a comprehensive description enhances readability.Consider updating the comments as follows:
-// BundledTxResponse defines the response of a bundled tx. - // If the operation fails the error field will be populated, the used gas fields will also be - // populated depending on when the execution stopped. Bundler payment responses will be populated - // if the execution fails. message BundledTxResponse { +// BundledTxResponse defines the response of a bundled transaction. +// If the operation fails, the `error` field will be populated. +// The gas usage fields will reflect the gas consumed up to the point of failure. +// `bundler_payment_responses` will be populated if the execution fails.x/accounts/keeper_account_abstraction_test.go (3)
18-19: Use fixed timestamps for deterministic testsUsing
time.Now()in tests can lead to non-deterministic behavior and flaky tests. Consider using a fixedcurrentTimeto ensure that tests are repeatable and independent of the system clock.Apply this diff to set a fixed timestamp:
-currentTime := time.Now() +currentTime := time.Unix(1696200000, 0) // Fixed timestamp for test determinism
217-228: Userequirefor critical assertions to halt on failureIn the test loop, using
requireinstead ofassertfor error checks ensures that the test stops immediately upon failure, preventing subsequent assertions from running with invalid states.Apply this diff to switch to
require:- assert.Error(t, err) - assert.Contains(t, err.Error(), tt.wantErr) + require.Error(t, err) + require.Contains(t, err.Error(), tt.wantErr)
21-26: Include comments to clarify test setup variablesAdding comments to explain the purpose and structure of
validXtenhances readability and helps other developers understand the test setup more easily.x/accounts/keeper_account_abstraction.go (3)
Line range hint
24-31: Enhance readability by adjusting error variable declarationsThe error variable declarations include inline comments, which may affect readability. Consider placing comments above each error variable to enhance clarity, following Go conventions.
Apply this diff:
var ( - ErrAASemantics = errors.New("invalid account abstraction tx semantics") - // ErrAuthentication is returned when the authentication fails. - ErrAuthentication = errors.New("authentication failed") - // ErrBundlerPayment is returned when the bundler payment fails. - ErrBundlerPayment = errors.New("bundler payment failed") - // ErrExecution is returned when the execution fails. - ErrExecution = errors.New("execution failed") + // ErrAASemantics is returned when the account abstraction transaction semantics are invalid. + ErrAASemantics = errors.New("invalid account abstraction tx semantics") + + // ErrAuthentication is returned when the authentication fails. + ErrAuthentication = errors.New("authentication failed") + + // ErrBundlerPayment is returned when the bundler payment fails. + ErrBundlerPayment = errors.New("bundler payment failed") + + // ErrExecution is returned when the execution fails. + ErrExecution = errors.New("execution failed") )
164-164: Address the TODO comment regarding timeout checksThe TODO comment indicates concerns about code repetition in the timeout verification logic. Refactoring the timeout checks into a separate helper function can reduce duplication and enhance maintainability.
Would you like assistance in refactoring this code segment as suggested above?
203-206: Implement validation logic inverifyAaXtfunctionThe
verifyAaXtfunction currently does not perform any checks and simply returnsnil. Consider implementing the necessary validation logic forTxExtensionto ensure the integrity of the transaction extension.Would you like assistance in implementing this validation logic?
tests/integration/accounts/fixture_test.go (1)
57-57: Add comment to explain emptyRegisterQueryHandlersimplementationThe
RegisterQueryHandlersmethod inmockAccountis empty, which might cause confusion for future maintainers. Adding a comment clarifies that no query handlers are needed for this mock account.Apply this diff to add an explanatory comment:
func (m mockAccount) RegisterQueryHandlers(_ *accountstd.QueryBuilder) { + // No query handlers to register for mockAccount. }x/accounts/keeper.go (1)
Line range hint
85-107: Inconsistent Method Receivers inKeeperStructThe
Keeperstruct methods predominantly use value receivers (e.g.,func (k Keeper)), but the newly addedDisableTxBundlingmethod uses a pointer receiver(k *Keeper).For consistency and to avoid potential confusion, it's recommended to use the same receiver type for all methods of a struct. Since
DisableTxBundlingmodifies the receiver's state, using pointer receivers for all methods is advisable. This also ensures that any modifications to theKeeperinstance persist across method calls.Consider refactoring the
Keeperstruct methods to use pointer receivers:-// Existing method with value receiver -func (k Keeper) SomeMethod() { +// Refactored method with pointer receiver +func (k *Keeper) SomeMethod() { // method implementation }simapp/app.go (1)
220-226: Consider enhancing the panic message for better debugging.While panicking on initialization errors is acceptable, adding more context to the panic message can aid in debugging.
You could modify the panic call to include additional context:
if err != nil { - panic(err) + panic(fmt.Errorf("failed to create txDecoder: %w", err)) }api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (2)
2649-2649: Correct pluralization in the comment forTxExtensionIn the comment for the
TxExtensionstruct, "AA's" is used to denote a plural, but the apostrophe indicates possession. It should be "AAs" without an apostrophe to represent the plural form of "Account Abstractions".Apply this diff to correct the comment:
-// TxExtension is the extension option that AA's add to txs when they're bundled. +// TxExtension is the extension option that AAs add to txs when they're bundled.
2655-2676: Enclose field names in backticks within comments for clarityFor improved readability and to adhere to the Go documentation conventions, consider enclosing field names in backticks within the comments. This makes it clear when you are referring to code identifiers.
Apply these changes to the field comments:
-// authentication_gas_limit expresses the gas limit to be used for the authentication part of the +// `AuthenticationGasLimit` expresses the gas limit to be used for the authentication part of the // bundled tx. -// bundler_payment_messages expresses a list of messages that the account +// `BundlerPaymentMessages` expresses a list of messages that the account // executes to pay the bundler for submitting the bundled tx. // It can be empty if the bundler does not need any form of payment, // the handshake for submitting the UserOperation might have happened off-chain. // Bundlers and accounts are free to use any form of payment, in fact the payment can // either be empty or be expressed as: // - NFT payment // - IBC Token payment. // - Payment through delegations. -// bundler_payment_gas_limit defines the gas limit to be used for the bundler payment. +// `BundlerPaymentGasLimit` defines the gas limit to be used for the bundler payment. // This ensures that, since the bundler executes a list of bundled tx and there needs to // be minimal trust between bundler and the tx sender, the sender cannot consume // the whole bundle gas. -// execution_gas_limit defines the gas limit to be used for the execution of the UserOperation's +// `ExecutionGasLimit` defines the gas limit to be used for the execution of the UserOperation's // execution messages.api/cosmos/accounts/v1/tx.pulsar.go (1)
2604-2604: Simplify slice initialization by removing unnecessary nil checkIn Go, appending to a nil slice initializes it automatically. The check
if x.Txs == nil { x.Txs = [][]byte{} }is unnecessary and can be omitted. This simplifies the code and aligns with Go idioms.x/accounts/proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (4)
44-45: Consider replacing "expresses" with "specifies" for clarityIn the comment for
authentication_gas_limit, using "specifies" would improve clarity and align with common terminology.Suggested change:
-// authentication_gas_limit expresses the gas limit to be used for the authentication part of the +// authentication_gas_limit specifies the gas limit to be used for the authentication part of the
47-47: Consider replacing "expresses" with "specifies" for claritySimilarly, in the comment for
bundler_payment_messages, replacing "expresses" with "specifies" may enhance understanding.Suggested change:
-// bundler_payment_messages expresses a list of messages that the account +// bundler_payment_messages specifies a list of messages that the account
53-55: Ensure consistent punctuation in list itemsThe list items in the comment have inconsistent punctuation. To improve readability, add periods to all items or remove them uniformly.
Suggested change:
// - NFT payment +// - NFT payment. // - IBC Token payment. // - Payment through delegations.Alternatively, remove periods from the other items:
-// - IBC Token payment. +// - IBC Token payment -// - Payment through delegations. +// - Payment through delegations
58-60: Simplify the comment for better readabilityThe sentence in the comment for
bundler_payment_gas_limitis complex. Consider rephrasing it to enhance clarity.Suggested change:
-// This ensures that, since the bundler executes a list of bundled tx and there needs to -// be minimal trust between bundler and the tx sender, the sender cannot consume -// the whole bundle gas. +// This ensures that the sender cannot consume the entire gas allocated for the bundle, +// as there is minimal trust between the bundler and the sender.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/accounts/interfaces/account_abstraction/v1/interface.pb.gois excluded by!**/*.pb.gox/accounts/v1/tx.pb.gois excluded by!**/*.pb.gox/tx/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
- api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (6 hunks)
- api/cosmos/accounts/v1/tx.pulsar.go (27 hunks)
- simapp/app.go (3 hunks)
- tests/e2e/accounts/account_abstraction_test.go (0 hunks)
- tests/integration/accounts/bundler_test.go (1 hunks)
- tests/integration/accounts/fixture_test.go (1 hunks)
- tests/integration/auth/keeper/fixture_test.go (1 hunks)
- x/accounts/README.md (1 hunks)
- x/accounts/depinject.go (2 hunks)
- x/accounts/keeper.go (6 hunks)
- x/accounts/keeper_account_abstraction.go (3 hunks)
- x/accounts/keeper_account_abstraction_test.go (1 hunks)
- x/accounts/msg_server.go (3 hunks)
- x/accounts/msg_server_test.go (1 hunks)
- x/accounts/proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (2 hunks)
- x/accounts/proto/cosmos/accounts/v1/tx.proto (1 hunks)
- x/accounts/protoutils.go (1 hunks)
- x/accounts/utils_test.go (1 hunks)
- x/auth/tx/config.go (1 hunks)
- x/tx/go.mod (1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/accounts/account_abstraction_test.go
🧰 Additional context used
📓 Path-based instructions (16)
api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/accounts/v1/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/accounts/bundler_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/accounts/fixture_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/auth/keeper/fixture_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/accounts/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/keeper_account_abstraction.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/keeper_account_abstraction_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/msg_server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/protoutils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/tx/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
x/accounts/msg_server.go (2)
Learnt from: testinginprod PR: cosmos/cosmos-sdk#18499 File: x/accounts/msg_server.go:123-125 Timestamp: 2023-11-20T18:30:34.484Z Learning: The `ExecuteBundle` method in `x/accounts/msg_server.go` is intentionally left unimplemented and is expected to be addressed in a follow-up PR.Learnt from: testinginprod PR: cosmos/cosmos-sdk#18499 File: x/accounts/msg_server.go:123-125 Timestamp: 2024-10-08T15:31:05.486Z Learning: The `ExecuteBundle` method in `x/accounts/msg_server.go` is intentionally left unimplemented and is expected to be addressed in a follow-up PR.
🪛 LanguageTool
x/accounts/README.md
[style] ~605-~605: To form a complete sentence, be sure to include a subject.
Context: ...uding NFTs, IBC tokens, or delegations. Can be empty if payment arrangements are ma...(MISSING_IT_THERE)
🪛 Markdownlint
x/accounts/README.md
624-624: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
625-625: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
602-602: Expected: 1; Actual: 2; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
608-608: Expected: 1; Actual: 3; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
613-613: Expected: 1; Actual: 4; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
🔇 Additional comments (27)
x/tx/go.mod (1)
27-27: Verify the necessity of the new indirect dependency.A new indirect dependency has been added:
github.com/rogpeppe/go-internal v1.11.0. While indirect dependencies are typically added automatically by the Go toolchain, it's important to understand why this dependency was introduced.To ensure this dependency is necessary and doesn't introduce any conflicts or security issues, please run the following commands:
Please review the output of these commands and confirm:
- If the dependency is directly used in the codebase.
- If removing the dependency causes any issues.
- If there are any known high or critical severity vulnerabilities for this package.
If the dependency is not directly used and can be removed without issues, consider removing it. If it's necessary, ensure that it doesn't introduce any security risks.
x/accounts/utils_test.go (3)
Line range hint
1-105: Overall assessment: Minimal impact on test utilityThe change in the
newKeeperfunction is isolated and doesn't affect the overall structure or purpose of the test utility. The modification is minimal and appears to be a necessary adjustment to align with changes in the main implementation.
Line range hint
1-105: Test coverage assessmentWhile this file is a test utility and doesn't contain direct unit tests, it plays a crucial role in facilitating testing for the accounts module. The change to the
newKeeperfunction might impact how other tests are set up.To ensure sufficient code coverage:
- Verify that existing tests using this utility still pass with the updated
newKeeperfunction.- Consider adding or updating tests in other files that specifically exercise the new parameter added to the
NewKeeperfunction call.To check if existing tests are affected, run:
Would you like assistance in identifying tests that might need updates or in creating new tests to cover this change?
78-78: Approve change with suggestions for improvementThe addition of the
nilparameter to theNewKeeperfunction call appears to be correct, likely reflecting changes in the main implementation. However, to ensure clarity and maintainability:
- Please verify that this change is consistent with the updated
NewKeeperfunction signature in the main implementation.- Consider adding a comment explaining the purpose of this new parameter, especially since it's set to
nilin the test setup.To verify the consistency of this change, please run the following command:
Consider adding a comment above the
NewKeepercall to explain the purpose of the newnilparameter:// The nil parameter represents [explain its purpose here] m, err := NewKeeper(codec.NewProtoCodec(ir), env, addressCodec, ir, nil, accounts...)tests/integration/auth/keeper/fixture_test.go (1)
Line range hint
1-153: Summary: Approve changes, recommend thorough testingThe changes to this integration test file appear to be minimal and focused on updating the
accounts.NewKeeperfunction call. While this change seems intentional and localized, it's crucial to ensure it doesn't introduce any unintended side effects in the test execution.Recommendations:
- Run the entire test suite to verify that this change doesn't negatively impact any existing tests.
- Consider adding or updating tests specifically for the functionality related to this new parameter in the
accounts.NewKeeperfunction.To ensure the changes haven't introduced any regressions, please run the following command:
This will help verify that all tests in the auth keeper package still pass after this change.
x/auth/tx/config.go (2)
Line range hint
89-91: Improved error handling for SigningOptionsThe addition of an early check for nil
SigningOptionsenhances the function's robustness and aligns with the Uber Golang style guide's recommendation for early error handling. The error message is clear and descriptive.
Line range hint
196-201: Improved error handling and decoder initializationThe changes in this segment enhance the function's robustness and correctness:
- The syntax error (extra comma) has been fixed.
- Error handling for the decoder initialization is now more comprehensive.
- The use of
txdecode.NewDecoderensures proper setting of bothtxConfig.decoderandtxConfig.txDecoder.These improvements align well with the Uber Golang style guide's recommendations for error handling and code clarity.
x/accounts/README.md (4)
542-561: LGTM: Clear introduction to transaction bundlingThe introduction to transaction bundling is well-written and informative. The advantages are clearly explained, and the mermaid diagram effectively illustrates the bundling process. This section provides a solid foundation for understanding the concept.
563-617: LGTM: Comprehensive explanation of TxExtensionThe Tx Extension section provides a thorough explanation of the TxExtension message, including its protobuf definition and detailed descriptions of each field. This information is crucial for developers working with bundled transactions.
🧰 Tools
🪛 LanguageTool
[style] ~605-~605: To form a complete sentence, be sure to include a subject.
Context: ...uding NFTs, IBC tokens, or delegations. Can be empty if payment arrangements are ma...(MISSING_IT_THERE)
🪛 Markdownlint
602-602: Expected: 1; Actual: 2; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
608-608: Expected: 1; Actual: 3; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
613-613: Expected: 1; Actual: 4; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
618-638: LGTM: Important compatibility considerationsThe Compatibility section effectively highlights the potential issues that may arise when implementing bundling, particularly regarding ante handler checks. The instructions for disabling bundling are clear and concise, providing a straightforward solution for chains where bundling may not be suitable.
🧰 Tools
🪛 Markdownlint
624-624: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
625-625: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
541-638: Overall assessment: Excellent addition to the documentationThe new content on transaction bundling is a valuable addition to the x/accounts module documentation. It provides clear explanations of the bundling concept, its benefits, and important technical details such as the TxExtension message. The compatibility considerations are particularly useful for developers implementing this feature.
The minor style and grammatical issues identified can be easily addressed, and once resolved, will further enhance the already high-quality documentation.
🧰 Tools
🪛 LanguageTool
[style] ~605-~605: To form a complete sentence, be sure to include a subject.
Context: ...uding NFTs, IBC tokens, or delegations. Can be empty if payment arrangements are ma...(MISSING_IT_THERE)
🪛 Markdownlint
624-624: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
625-625: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
602-602: Expected: 1; Actual: 2; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
608-608: Expected: 1; Actual: 3; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
613-613: Expected: 1; Actual: 4; Style: 1/1/1
Ordered list item prefix(MD029, ol-prefix)
x/accounts/depinject.go (2)
10-10: LGTM!The import statement correctly adds the
txdecodepackage required for transaction decoding.
52-58: Proper initialization of the transaction decoderThe
txDecvariable is correctly initialized usingtxdecode.NewDecoderwith the appropriate options. This ensures that transaction decoding is properly configured with the necessary signing context and codec.x/accounts/msg_server.go (6)
5-5: Importing the 'errors' package is appropriate.The addition of the
"errors"import is necessary for error handling introduced in the code changes.
15-16: Definition of 'ErrBundlingDisabled' follows Go conventions.Defining
ErrBundlingDisabledas a package-level variable usingerrors.New()aligns with Go best practices for error handling.
91-93: Properly handling the case when bundling is disabled.The conditional check for
m.k.bundlingDisabledcorrectly prevents the execution of bundled transactions when bundling is disabled, returning the appropriate errorErrBundlingDisabled.
95-98: Validating and decoding the 'Bundler' address correctly.The conversion of
req.Bundlerusingm.k.addressCodec.StringToBytesand handling potential errors ensures that the bundler's address is valid before proceeding.
99-103: Efficiently processing bundled transactions.The loop correctly iterates over
req.Txs, executing each bundled transaction withm.k.ExecuteBundledTxand collecting the responses.
91-104: Verify the proper utilization of 'req.Bundler' in 'ExecuteBundledTx'.Ensure that
req.Bundleris appropriately used withinm.k.ExecuteBundledTxfor authorization and validation purposes. This is crucial for maintaining the security and correctness of bundled transaction execution.Run the following script to inspect the usage of
ExecuteBundledTxand confirm thatreq.Bundleris handled correctly:x/accounts/proto/cosmos/accounts/v1/tx.proto (2)
109-109: Updated comment improves clarity forresponsesfieldThe revised description for the
responsesfield inMsgExecuteBundleResponseclarifies that it contains the list of responses from the bundled transactions, enhancing the understanding of its purpose.
80-80: Verify all usages ofMsgExecuteBundleand itstxsfield after type changeThe type of the
txsfield inMsgExecuteBundlehas been changed fromrepeated cosmos.tx.v1beta1.TxRawtorepeated bytes. This alteration may impact serialization/deserialization processes and any code that interacts with this field. Please ensure that all parts of the codebase that useMsgExecuteBundleand itstxsfield are updated to handle the new type appropriately.You can run the following shell script to find all references to
MsgExecuteBundleand itstxsfield:✅ Verification successful
All usages of
MsgExecuteBundleand itstxsfield have been correctly updated to handle the newrepeated bytestype. Serialization and deserialization processes are appropriately managed, and no issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'MsgExecuteBundle' and its 'txs' field. # Search for 'MsgExecuteBundle' occurrences in proto files rg --type proto 'MsgExecuteBundle' # Search for usages of 'MsgExecuteBundle' in Go code rg --type go 'MsgExecuteBundle' # Search for code accessing the 'txs' field of 'MsgExecuteBundle' rg --type go 'MsgExecuteBundle.*\.txs'Length of output: 29494
Script:
#!/bin/bash # Description: Find all usages of 'MsgExecuteBundle' in proto files and code interacting with the 'txs' field. # Search for 'MsgExecuteBundle' occurrences in proto files rg 'MsgExecuteBundle' --glob '*.proto' # Search for code accessing the 'txs' field of 'MsgExecuteBundle' rg 'MsgExecuteBundle.*\.txs' --type goLength of output: 1153
Script:
#!/bin/bash # Description: Extract and inspect usages of 'txs' field in MsgExecuteBundle within tx.pulsar.go # Extract lines where 'MsgExecuteBundle.txs' is accessed rg 'MsgExecuteBundle\.txs' api/cosmos/accounts/v1/tx.pulsar.goLength of output: 358
Script:
#!/bin/bash # Description: Extract surrounding context for each 'case "cosmos.accounts.v1.MsgExecuteBundle.txs":' in tx.pulsar.go # Use grep with -C to get context lines around each match rg 'case "cosmos\.accounts\.v1\.MsgExecuteBundle\.txs":' -A 5 -B 5 api/cosmos/accounts/v1/tx.pulsar.goLength of output: 3019
x/accounts/keeper.go (1)
369-398: Efficient Handling insendManyMessagesFunctionsThe newly added
sendManyMessagesReturnAnysandsendManyMessagesfunctions effectively process multiple messages, handle errors appropriately, and collect responses efficiently.simapp/app.go (2)
81-81: Import statement fortxdecodeis properly added.The new import
txdecode "cosmossdk.io/x/tx/decode"is correctly grouped with other module imports.
319-319: PassingtxDecodertoaccounts.NewKeeperis appropriate.The inclusion of
txDecoderas a parameter enhances the accounts keeper's ability to process transactions effectively.api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (1)
2892-2903: Ensure generated code adheres to style guidelinesThe
Exporterfunction forTxExtensionis part of the generated code. While modifications to generated code can be overwritten, if possible, consider ensuring that the code generation templates produce code that conforms to the Uber Go Style Guide.To check if the code generator can be configured to adhere to the style guide, review the code generation settings or templates.
api/cosmos/accounts/v1/tx.pulsar.go (1)
7-7: Verify the necessity of the underscore importThe underscore import of
"cosmossdk.io/api/cosmos/tx/v1beta1"indicates that the package is imported for its side effects. Please verify if these side effects are required. If not, consider removing the import to simplify dependencies.x/accounts/proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (1)
42-64: LGTM!The
TxExtensionmessage is well-structured, and the fields are appropriately defined. The additions align with the objectives to enhance transaction processing and account abstraction capabilities within the SDK.
| _, err := s.ExecuteBundle(ctx, &v1.MsgExecuteBundle{ | ||
| Bundler: "someone", | ||
| Txs: nil, | ||
| }) | ||
| require.ErrorIs(t, err, ErrBundlingDisabled) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error checking for more robust testing.
While the current error check is correct, it could be more specific to ensure the exact error is returned. Consider using require.EqualError to verify both the error type and its message.
- require.ErrorIs(t, err, ErrBundlingDisabled)
+ require.EqualError(t, err, ErrBundlingDisabled.Error(), "Expected ErrBundlingDisabled error")This change will make the test more robust by checking both the error type and its message.
📝 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.
| _, err := s.ExecuteBundle(ctx, &v1.MsgExecuteBundle{ | |
| Bundler: "someone", | |
| Txs: nil, | |
| }) | |
| require.ErrorIs(t, err, ErrBundlingDisabled) | |
| _, err := s.ExecuteBundle(ctx, &v1.MsgExecuteBundle{ | |
| Bundler: "someone", | |
| Txs: nil, | |
| }) | |
| require.EqualError(t, err, ErrBundlingDisabled.Error(), "Expected ErrBundlingDisabled error") |
| @@ -79,6 +79,7 @@ func initFixture(t *testing.T, extraAccs map[string]accountstd.Interface) *fixtu | |||
| runtime.NewEnvironment(runtime.NewKVStoreService(keys[accounts.StoreKey]), log.NewNopLogger(), runtime.EnvWithQueryRouterService(queryRouter), runtime.EnvWithMsgRouterService(router)), | |||
| addresscodec.NewBech32Codec("cosmos"), | |||
| cdc.InterfaceRegistry(), | |||
| nil, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Parameter Count for accounts.NewKeeper
The accounts.NewKeeper function is called with only one parameter in both of the following test files:
tests/integration/accounts/fixture_test.go: 1 parametertests/integration/auth/keeper/fixture_test.go: 1 parameter
However, the review comment indicated that a nil parameter was added to the accounts.NewKeeper call in tests/integration/auth/keeper/fixture_test.go. This suggests that the nil parameter was not actually added, leading to an inconsistency.
Please ensure that the nil parameter is correctly added to all relevant instances of accounts.NewKeeper to maintain consistency and prevent potential issues.
🔗 Analysis chain
Approve change, but clarification needed on new parameter
The addition of the nil parameter to accounts.NewKeeper appears to be intentional and part of a larger update to the function signature. However, a few points need attention:
-
Could you please clarify the purpose of this new parameter? Understanding its role will help ensure it's being used correctly in the test setup.
-
If this parameter represents a new feature or configuration option, consider updating the related documentation to reflect this change.
-
It would be beneficial to verify if this change has been consistently applied across all relevant test files to maintain consistency in the test suite.
To ensure consistency across test files, please run the following command:
This will help identify if the change has been applied consistently across all test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of accounts.NewKeeper in test files
rg --type go -g '*_test.go' 'accounts\.NewKeeper\(' tests/
Length of output: 212
Script:
#!/bin/bash
# Search for all occurrences of accounts.NewKeeper in test files
rg --type go -g '*_test.go' 'accounts\.NewKeeper\(' tests/
Length of output: 237
Script:
#!/bin/bash
# Count the number of parameters passed to accounts.NewKeeper in test files
rg --type go -g '*_test.go' 'accounts\.NewKeeper\(' tests/ | while read -r line; do
file=$(echo "$line" | cut -d: -f1)
params=$(echo "$line" | awk -F'(' '{print $2}' | tr -d ')' | tr ',' '\n' | wc -l)
echo "$file: $params parameters"
done
Length of output: 1028
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling the error without panicking
Currently, if txdecode.NewDecoder returns an error, the code panics. It's generally preferable to handle errors gracefully or return them to the caller, allowing for better error management and avoiding abrupt termination.
Apply this diff to return the error instead of panicking:
func ProvideModule(in ModuleInputs) ModuleOutputs {
accCreators := make([]accountstd.AccountCreatorFunc, len(in.Accounts))
for i, acc := range in.Accounts {
accCreators[i] = acc.MakeAccount
}
txDec, err := txdecode.NewDecoder(txdecode.Options{
SigningContext: in.Registry.SigningContext(),
ProtoCodec: in.Cdc,
})
if err != nil {
- panic(err)
+ panic(fmt.Errorf("failed to create transaction decoder: %w", err))
}
accountsKeeper, err := NewKeeper(
in.Cdc, in.Environment, in.AddressCodec, in.Registry, txDec,
accCreators...,
)
if err != nil {
panic(err)
}
m := NewAppModule(in.Cdc, accountsKeeper)
return ModuleOutputs{AccountsKeeper: accountsKeeper, Module: m}
}Alternatively, if modifying the function signature is feasible, consider returning an error:
-func ProvideModule(in ModuleInputs) ModuleOutputs {
+func ProvideModule(in ModuleInputs) (ModuleOutputs, error) {
accCreators := make([]accountstd.AccountCreatorFunc, len(in.Accounts))
for i, acc := range in.Accounts {
accCreators[i] = acc.MakeAccount
}
txDec, err := txdecode.NewDecoder(txdecode.Options{
SigningContext: in.Registry.SigningContext(),
ProtoCodec: in.Cdc,
})
if err != nil {
- panic(err)
+ return ModuleOutputs{}, err
}
accountsKeeper, err := NewKeeper(
in.Cdc, in.Environment, in.AddressCodec, in.Registry, txDec,
accCreators...,
)
if err != nil {
- panic(err)
+ return ModuleOutputs{}, err
}
m := NewAppModule(in.Cdc, accountsKeeper)
- return ModuleOutputs{AccountsKeeper: accountsKeeper, Module: m}
+ return ModuleOutputs{AccountsKeeper: accountsKeeper, Module: m}, nil
}This change would allow the caller to handle the error appropriately.
Committable suggestion was skipped due to low confidence.
| accountsKeeper, err := NewKeeper( | ||
| in.Cdc, in.Environment, in.AddressCodec, in.Registry, | ||
| in.Cdc, in.Environment, in.AddressCodec, in.Registry, txDec, |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Many NewKeeper calls across the codebase are missing the txDec parameter.
Please update all instances of NewKeeper to include the txDec parameter to ensure consistency and prevent potential issues.
🔗 Analysis chain
Verify that all calls to NewKeeper include the txDec parameter
To ensure consistency, please verify that all invocations of NewKeeper across the codebase have been updated to include the new txDec parameter.
Run the following script to check:
This script searches for all occurrences of NewKeeper and counts the number of arguments to ensure txDec is included.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to `NewKeeper` include the `txDec` parameter.
# Test: Search for all invocations of `NewKeeper`. Expect: All calls should have five or more arguments before variadic parameters.
rg --type go 'NewKeeper\(' -A 5 | gawk '
BEGIN { FS="[\n()]" }
/NewKeeper\(/ { call=$0; getline; args=1; while ($0 !~ /\)/) { args++; getline; } if (args < 5) { print FILENAME ":" FNR ": " call " - missing txDec parameter"; } }
'
Length of output: 13040
| if m.k.bundlingDisabled { | ||
| return nil, ErrBundlingDisabled | ||
| } | ||
|
|
||
| _, err := m.k.addressCodec.StringToBytes(req.Bundler) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| responses := make([]*v1.BundledTxResponse, len(req.Txs)) | ||
| for i, bundledTx := range req.Txs { | ||
| bundleRes := m.k.ExecuteBundledTx(ctx, req.Bundler, bundledTx) | ||
| responses[i] = bundleRes | ||
| } | ||
| return &v1.MsgExecuteBundleResponse{Responses: responses}, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding concurrency for executing bundled transactions.
To enhance performance, especially when dealing with a large number of transactions, consider processing the bundled transactions concurrently.
You might use goroutines and synchronization mechanisms to execute m.k.ExecuteBundledTx concurrently. Ensure that the underlying implementations are thread-safe and handle any potential concurrency issues, such as race conditions or shared resource conflicts.
| func (k *Keeper) DisableTxBundling() { | ||
| k.bundlingDisabled = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Issue with DisableTxBundling Method
The DisableTxBundling method sets the bundlingDisabled flag to true:
func (k *Keeper) DisableTxBundling() {
k.bundlingDisabled = true
}However, if instances of Keeper are used with value receivers elsewhere, the state change may not persist as intended.
To ensure the bundlingDisabled state is correctly maintained:
- Use pointer receivers consistently for all methods that modify the
Keeperstruct. - Ensure that
Keeperinstances are passed and stored as pointers where mutations are expected.
Consider updating the method and struct usage as follows:
-// Method with pointer receiver
func (k *Keeper) DisableTxBundling() {
k.bundlingDisabled = true
}
-// Example of value receiver method
-func (k Keeper) SomeMethod() {
+// Update to pointer receiver
+func (k *Keeper) SomeMethod() {
// method implementation
}And ensure that Keeper is used as a pointer throughout the codebase:
-keeper := Keeper{
+keeper := &Keeper{
// struct initialization
}| type _TxExtension_2_list struct { | ||
| list *[]*anypb.Any | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary use of pointer to slice in _TxExtension_2_list
In the _TxExtension_2_list struct, the list field is a pointer to a slice (*[]*anypb.Any). Since slices are already reference types in Go, using a pointer to a slice is generally unnecessary and can lead to complexity. Consider changing the field to a slice ([]*anypb.Any) instead.
Apply this diff to simplify the struct:
type _TxExtension_2_list struct {
- list *[]*anypb.Any
+ list []*anypb.Any
}Update the methods accordingly to accommodate this change. For example, modify the Len method:
func (x *_TxExtension_2_list) Len() int {
- if x.list == nil {
+ if x.list == nil {
return 0
}
- return len(*x.list)
+ return len(x.list)
}📝 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.
| type _TxExtension_2_list struct { | |
| list *[]*anypb.Any | |
| } | |
| type _TxExtension_2_list struct { | |
| list []*anypb.Any | |
| } |
| list *[][]byte | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type name _MsgExecuteBundle_2_list does not follow Go naming conventions
According to the Uber Go style guide, type names should be in PascalCase without underscores. Consider renaming _MsgExecuteBundle_2_list to MsgExecuteBundle2List to adhere to Go naming conventions and improve code readability.
| v := new(v1beta11.TxRaw) | ||
| *x.list = append(*x.list, v) | ||
| return protoreflect.ValueOfMessage(v.ProtoReflect()) | ||
| panic(fmt.Errorf("AppendMutable can not be called on message MsgExecuteBundle at list field Txs as it is not of Message kind")) |
There was a problem hiding this comment.
Avoid using panic in AppendMutable method
Per the Uber Go style guide, usage of panic is discouraged except for unrecoverable errors. Since AppendMutable should not be called for this type, consider returning an error or implementing a no-op to handle this case gracefully without panicking.
| @@ -216,6 +217,13 @@ func NewSimApp( | |||
| appCodec := codec.NewProtoCodec(interfaceRegistry) | |||
| legacyAmino := codec.NewLegacyAmino() | |||
| signingCtx := interfaceRegistry.SigningContext() | |||
| txDecoder, err := txdecode.NewDecoder(txdecode.Options{ | |||
There was a problem hiding this comment.
can we add this to the upgrading.md file?
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
TxExtensionmessage to manage gas limits and payment messages for bundled transactions.MsgExecuteBundleandBundledTxResponsestructures to capture detailed execution metrics.Bug Fixes
Documentation
Tests