Update x/banking and x/crisis InitChain re slow Gaia startup #7764
Conversation
robert-zaremba
requested review from
aaronc,
alessio and
alexanderbez
as code owners
robert-zaremba
added
backport/0.40.x (Stargate)
C:genesis
|
Forgot to write in the task description: Please don't look at the |
robert-zaremba
changed the title
Codecov Report
@@ Coverage Diff @@
## master #7764 +/- ##
=======================================
Coverage 54.13% 54.14%
=======================================
Files 610 610
Lines 38842 38854 +12
=======================================
+ Hits 21028 21037 +9
- Misses 15667 15670 +3
Partials 2147 2147 |
| @@ -192,6 +197,7 @@ func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts serverty | |||
| cast.ToString(appOpts.Get(flags.FlagHome)), | |||
| cast.ToUint(appOpts.Get(server.FlagInvCheckPeriod)), | |||
| simapp.MakeTestEncodingConfig(), // Ideally, we would reuse the one created by NewRootCmd. | |||
| appOpts, | |||
There was a problem hiding this comment.
Can we remove cast.ToString(appOpts.Get(flags.FlagHome)), cast.ToUint(appOpts.Get(server.FlagInvCheckPeriod)), and use appOpts directly to read the values inside NewSimApp? Let's minimize the number of arguments when possible. May be not relavant for this PR.
There was a problem hiding this comment.
Yeah, I was planning to group all this config options already long time ago.
Should I do it in this PR or in a new one? Thoughts?
There was a problem hiding this comment.
A separate PR works too, can you create an issue to track this?
There was a problem hiding this comment.
Yes, I think we should do it in a separate post, because this will be another breaking feature. Also, not sure if that will land in release, @clevinson, @aaronc , what do you think?
There was a problem hiding this comment.
Here is the old issue I created it. I'm happy that we like to see a reduction of this arguments. It's an easy change.
#7178
There was a problem hiding this comment.
Well we should still keep the variadic options (which is what I was trying to advocate for in #7178 -- I'm against options structs for ALL arguments in this case). However, for the sake of this PR, we can at least avoid the following calls/args:
cast.ToString(appOpts.Get(flags.FlagHome)),
cast.ToUint(appOpts.Get(server.FlagInvCheckPeriod)),
Just pass appOpts and let the app do the above two calls. This is what I meant.
| if err := k.ValidateBalance(ctx, addr); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this safe to remove? May be we still need ValidateBalance after SetBalances below? @alexanderbez any thoughts?
There was a problem hiding this comment.
According to my investigation - yes, because we are calling k.ClearBalances(ctx, addr) in k.SetBalances.
There was a problem hiding this comment.
Please also have a look here: #7682 (comment)
| func NewSimApp( | ||
| logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, skipUpgradeHeights map[int64]bool, | ||
| homePath string, invCheckPeriod uint, encodingConfig simappparams.EncodingConfig, baseAppOptions ...func(*baseapp.BaseApp), | ||
| homePath string, invCheckPeriod uint, encodingConfig simappparams.EncodingConfig, | ||
| appOpts servertypes.AppOptions, baseAppOptions ...func(*baseapp.BaseApp), |
There was a problem hiding this comment.
Are there any explicit argument we can remove from this constructor that are found in appOptons?
There was a problem hiding this comment.
Not sure. I can handle it in a separate PR.
There was a problem hiding this comment.
Yes, we can omit homePath string, invCheckPeriod uint. Feel free to do it in this PR or another :)
There was a problem hiding this comment.
I would prefer in another and have this one merged. This one is about InitChain options.
There was a problem hiding this comment.
I added TODOs to #7178
…7764) * add more logs during the initialization process * initializtion: move profiling to the top of the startProcess function * x/bank InitGenesis: remove k.ValidateBalance * debug: add logs and telemetry to x/bank and x/crisis * make x/crisis AssertInvariants optional during InitGenesis * Add module init flags mechanism * update changelog * remove debug fmt.Print * fix testutil/network/ * fix log message * update test NewApp calls * review changes Co-authored-by: Aaron Craelius <aaron@regen.network>
Description
closes: #7682
Please don't look at the
fmt.Println. I will remove it once I will get ACK that this changeset is OK.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes