refactor(core,x/**): simplify core service api and embed environment in keepers#20071
Conversation
|
Important Auto Review SkippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 58 files out of 115 files are above the max files limit of 50. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 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 (
|
github-actions
Bot
added
C:x/staking
C:x/gov
C:x/slashing
C:x/distribution
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| if err := k.proposalTable.Update(k.environment.KVStoreService.OpenKVStore(ctx), proposal.Id, &proposal); err != nil { | ||
| if err := k.proposalTable.Update(k.KVStoreService.OpenKVStore(ctx), proposal.Id, &proposal); err != nil { |
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop.
| //nolint:gosec // "implicit memory aliasing in the for loop (because of the pointer on &v)" | ||
| for _, v := range votes { | ||
| err = k.voteTable.Delete(k.environment.KVStoreService.OpenKVStore(ctx), &v) | ||
| err = k.voteTable.Delete(k.KVStoreService.OpenKVStore(ctx), &v) |
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop.
| proposalInfo.Status = group.PROPOSAL_STATUS_ABORTED | ||
|
|
||
| if err := k.proposalTable.Update(k.environment.KVStoreService.OpenKVStore(ctx), proposalInfo.Id, &proposalInfo); err != nil { | ||
| if err := k.proposalTable.Update(k.KVStoreService.OpenKVStore(ctx), proposalInfo.Id, &proposalInfo); err != nil { |
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop.
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
left a few questions. The biggest change is losing module information in the logs. But it doesnt seem like this was consistent
| } | ||
|
|
||
| func (*mockAccount) Environment() appmodule.Environment { | ||
| func (*mockAccount) GetEnvironment() appmodule.Environment { |
There was a problem hiding this comment.
how come get here, but removed elsewhere?
There was a problem hiding this comment.
This is a method on the keeper, not a service. As we use an interface instead of the concrete type for ante handler, we need to obtain environment. It was just called Environement() before but now it clashes with the embedded environment on the auth struct.
Yes, so the module name is gotten by depinject: https://github.com/cosmos/cosmos-sdk/pull/20071/files#diff-e58d72d17b7bd432b5a25c6277d603886f901a753261930bba47f5f47d706beeR263 |
| if errors.Is(err, collections.ErrEncoding) { | ||
| proposal.Id = prop.Key.K2() | ||
| if err := failUnsupportedProposal(logger, ctx, k, proposal, err.Error(), false); err != nil { | ||
| if err := failUnsupportedProposal(ctx, k, proposal, err.Error(), false); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| if errors.Is(err, collections.ErrEncoding) { | ||
| proposal.Id = prop.Key.K2() | ||
| if err := failUnsupportedProposal(logger, ctx, k, proposal, err.Error(), true); err != nil { | ||
| if err := failUnsupportedProposal(ctx, k, proposal, err.Error(), true); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| // prunes expired proposals. | ||
| func (k Keeper) EndBlocker(ctx context.Context) error { | ||
| if err := k.TallyProposalsAtVPEnd(ctx, k.environment); err != nil { | ||
| if err := k.TallyProposalsAtVPEnd(ctx); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| } | ||
|
|
||
| return k.PruneProposals(ctx, k.environment) | ||
| return k.PruneProposals(ctx) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
|
I think embedding |
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
will you modify app.go as part of this pr as well?
sure can do 👍🏾 |
4 participants
Description
I started by simplifying core services naming (as discussed on Slack) and as I had to touch all usage of environment in keeper, I applied @kocubinski suggestion of embedding environment in keepers directly here.
Get)Environement()getter renamingAuthor 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...