-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: Add non atomic multimsg #19350
Conversation
WalkthroughThe update introduces a new Changes
Assessment against linked issues
The code changes effectively fulfill the objectives outlined in the linked issue, enhancing the system's flexibility in managing transaction processing. 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 (
|
Do add test cases for impersonation |
I don't tlike the "async" nomenclature as messages are not being executed asynchronously. They're being executed in a non atomic way. |
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: 0
Configuration used: .coderabbit.yml
Files selected for processing (6)
- tests/integration/auth/keeper/msg_server_test.go (1 hunks)
- x/accounts/keeper.go (8 hunks)
- x/auth/keeper/keeper.go (5 hunks)
- x/auth/testutil/expected_keepers_mocks.go (2 hunks)
- x/auth/types/expected_keepers.go (2 hunks)
- x/auth/vesting/testutil/expected_keepers_mocks.go (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- tests/integration/auth/keeper/msg_server_test.go
- x/accounts/keeper.go
- x/auth/keeper/keeper.go
- x/auth/testutil/expected_keepers_mocks.go
- x/auth/types/expected_keepers.go
- x/auth/vesting/testutil/expected_keepers_mocks.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/auth/keeper/keeper.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/auth/keeper/keeper.go
…ita/nonatomic-multimsg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit
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.
beside these below, LGTM!!!
…ita/nonatomic-multimsg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
…ita/nonatomic-multimsg
are we good to merge this @testinginprod ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Actionable comments outside the diff hunks (1)
x/bank/testutil/expected_keepers_mocks.go (1)
[!TIP]
Codebase VerificationThe search results indicate that the method
IsAccountsModuleAccount
is still referenced in multiple files within the codebase, includingkeeper.go
, various mock files intestutil
, andaccount.go
. This suggests that the method is still actively used and expected by other parts of the system.Given this information, the removal of the
IsAccountsModuleAccount
method from theexpected_keepers_mocks.go
in thex/bank
module could potentially lead to issues if the method is expected to be mocked during testing of components that interact with thex/bank
module.Therefore, it is crucial to ensure that the removal of this method does not disrupt any existing functionalities or dependencies that rely on this method being mocked. Further investigation or reconsideration of the removal might be necessary to avoid potential integration issues.
Analysis chain
Line range hint
1-1
: Verify the removal ofIsAccountsModuleAccount
does not affect other modules that might rely on this functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to IsAccountsModuleAccount in the codebase. rg --type go "IsAccountsModuleAccount"Length of output: 2117
…ita/nonatomic-multimsg
can we get this merged @tac0turtle |
Description
Closes: #13911
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
NonAtomicExec
method.AccountsModule
across tests and configurations.Refactor
AccountsKeeper
for smoother interactions.Tests
AccountsModule
for enhanced coverage.NonAtomicExec
method.Chores