refactor(x/auth): auth module can recognize x/accounts account#20002
Conversation
github-actions
Bot
added
C:CLI
C:x/distribution
WalkthroughWalkthroughThe changes involve enhancing the account management system by integrating an Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // IsAccountsModuleAccount check if an address belong to a smart account. | ||
| func (k Keeper) IsAccountsModuleAccount( | ||
| ctx context.Context, | ||
| accountAddr []byte, | ||
| ) bool { | ||
| _, err := k.AccountByNumber.Get(ctx, accountAddr) | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
The method IsAccountsModuleAccount checks if an account number exists rather than verifying if the account is a "smart account". Please clarify the intended functionality or adjust the implementation to match the description.
| @@ -439,6 +439,10 @@ func (k BaseKeeper) setSupply(ctx context.Context, coin sdk.Coin) { | |||
| func (k BaseKeeper) trackDelegation(ctx context.Context, addr sdk.AccAddress, balance, amt sdk.Coins) error { | |||
| acc := k.ak.GetAccount(ctx, addr) | |||
There was a problem hiding this comment.
I'm wondering if we should provide a way for GetAccount to work for x/accounts.
I think the way i'd handle this is:
- GetAccount
- try on x/auth state
- If not exist:
- check on x/accounts
- fetch account type from x/accounts
- if account type == "base_account" (the one living in default/base)
- then convert it into BaseAccount struct
- else: return it as a private implementation.
- if account type == "base_account" (the one living in default/base)
There was a problem hiding this comment.
yeah i was thinking of adding an query handler just for auth getAccount to return some type of usable account info struct
There was a problem hiding this comment.
but for the sake of pushing this to unblock e2e test for auditing, we can move this to another PR
testinginprod
left a comment
testinginprod
left a comment
There was a problem hiding this comment.
LGTM, but I'd take time to better handle the GetAccount case.
…osmos/cosmos-sdk into son/auth_recognizes_x/accounts
| return nil, nil, err | ||
| } | ||
| accountKeeper := authkeeper.NewAccountKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(storeKey.(*storetypes.KVStoreKey)), log.NewNopLogger()), cdc, authtypes.ProtoBaseAccount, nil, addressCodec, sdk.Bech32MainPrefix, authorityStrAddr) | ||
| ctrl := gomock.NewController(&testing.T{}) |
There was a problem hiding this comment.
Consider passing a *testing.T instance to gomock.NewController instead of using &testing.T{} directly.
- ctrl := gomock.NewController(&testing.T{})
+ ctrl := gomock.NewController(t)This change ensures that the mock controller is associated with the actual test case instance, allowing for proper handling of test failures and assertions.
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.
| ctrl := gomock.NewController(&testing.T{}) | |
| ctrl := gomock.NewController(t) |
* main: (25 commits) fix: Implement gogoproto customtype to secp256r1 keys (cosmos#20027) fix(x/epochs): avoid invalid epoch duration in simulation (cosmos#20030) fix(x/bank): align query with multi denoms for send-enabled (cosmos#20028) refactor(x/slashing)!: remove Accounts String (cosmos#20026) refactor(x/evidence)!: remove Address.String() (cosmos#20016) chore: make telemetry consistent (cosmos#20025) chore: prepare x/tx changelog (cosmos#20015) build(deps): Bump actions/add-to-project from 1.0.0 to 1.0.1 (cosmos#20018) feat(x/bank): support depinject for send restrictions (cosmos#20014) feat: Conditionally emit metrics based on enablement (cosmos#19903) fix(store): fix the typo (cosmos#20011) docs(x/feegrant): fix allowance typo (cosmos#20000) chore(confix): update latest config value (cosmos#20012) refactor(x/auth): auth module can recognize x/accounts account (cosmos#20002) fix(mempool): use no-op mempool as default (cosmos#19970) fix(simapp): add epoch store to upgrade (cosmos#20007) test(kv): add unit tests for the helpers functions kv.AssertKeyAtLeas… (cosmos#19965) feat(x/tx): Rename custom Amino JSON encoder to "inline_json" (cosmos#19919) refactor(x/auth): use transaction service (cosmos#19967) fix(client/v2): add encoder for `cosmos.base.v1beta1.DecCoin` (cosmos#19976) ...
4 participants
Description
This is to help lockup account execute delegate and undelegate. Bank module now can skip the
TrackDelegationandTrackUndelegationprocess for legacy vesting account if the address belong to anaccountsmodule accountthanks @likhita-809 ( i have to borrow some of your code 🙏 ) for easing my process of adding x/accounts depinject 🙏
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.
I have...
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes