-
Notifications
You must be signed in to change notification settings - Fork 855
Flatten BeginBlock and remove nested logics #2522
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
base: main
Are you sure you want to change the base?
Conversation
8a736a9 to
e3332ea
Compare
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (30.76%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2522 +/- ##
==========================================
- Coverage 43.22% 43.17% -0.05%
==========================================
Files 1583 1586 +3
Lines 138846 138790 -56
==========================================
- Hits 60011 59925 -86
- Misses 73389 73425 +36
+ Partials 5446 5440 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
b18902f to
5e3e8bf
Compare
5e3e8bf to
6bc167c
Compare
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, just one question regarding capability BeginBlocker
| func (am AppModule) BeginBlock(ctx sdk.Context, _ abci.RequestBeginBlock) { | ||
| defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) | ||
|
|
||
| am.keeper.InitMemStore(ctx) |
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.
why was this not previously calling capability's BeginBlocker function which also just calls the InitMemStore?
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.
i'm not quite sure - could just be a refactor oversight from cosmos-sdk
Describe your changes and provide context
Previously, BeginBlock follows a very involved call path:
This PR simplifies BeginBlock to be
SeiApp->keepersTesting performed to validate your change
no functional change