Skip to content
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

Call end block function in each cosmos modules #1683

Merged
merged 2 commits into from
Feb 28, 2020
Merged

Call end block function in each cosmos modules #1683

merged 2 commits into from
Feb 28, 2020

Conversation

antho1404
Copy link
Member

@antho1404 antho1404 commented Feb 26, 2020

The EndBlocker of the module was not called by the module and also these EndBlock for each module were not called because of a wrong initialization of the app.

@antho1404 antho1404 added the bug Something isn't working label Feb 26, 2020
@antho1404 antho1404 self-assigned this Feb 26, 2020
app/app.go Outdated Show resolved Hide resolved
@@ -134,6 +134,7 @@ func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {

// EndBlock returns the end blocker for the execution module. It returns no validator
// updates.
func (AppModule) EndBlock(_ sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
EndBlocker(ctx, am.keeper)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum end blocker is empty in each module. What is the point of calling it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making sure that it's called when we need it, I was experimenting with the pagination and found this issue

app/app.go Outdated Show resolved Hide resolved
@krhubert
Copy link
Contributor

The EndBlocker of the module was not called by the module and also these EndBlock for each module were not called because of a wrong initialization of the app.

Hum, I'm not sure why this code works and how, but EndBlock is empty and dose nothing in each module.

Also you have change BeginBlockers order which on which I'm not sure if they are good.
If you take a look on cosmos/sdk-tutorial , the line is missing is in fact:

  app.mm.SetOrderBeginBlockers(distr.ModuleName, slashing.ModuleName)
  >>> app.mm.SetOrderEndBlockers(staking.ModuleName)

So I would rather add only this one and check if its' working.

@antho1404
Copy link
Member Author

I just reverted the modification on the begin/end order. Now the PR just call the EndBlocker. Not something critical as it is not called anymore but at least it's ready when we want to call it.

Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this PR is necessary but it doesn't cause any new issue, so i'm good with merging it

@krhubert
Copy link
Contributor

What about calling Order explicit like we disscuss?

I'm ok to merge this one too, although as @NicolasMahe point it's not necessary. Also if you want to be consistent you should call BeginBlock as well :P

@antho1404
Copy link
Member Author

I just wanted to revert it and maybe put it in another PR as it creates too much discussion and I want to try to have more details about that.
BeginBlock calls BeginBlocker correctly in all the modules, only the EndBlocker was not called.
I agree it's not a "necessary" PR for now but I was playing with the end of blocks and got stuck for a long time because of this bug so if it helps for next time not being stuck for nothing it's good isn't it? This is still a bug as a function of our module is never called and will never be called without this fix.

@antho1404 antho1404 merged commit 4d4b806 into dev Feb 28, 2020
@antho1404 antho1404 deleted the fix/end-block branch February 28, 2020 09:05
@NicolasMahe NicolasMahe added this to the next milestone Feb 28, 2020
@NicolasMahe NicolasMahe added the release:fix Pull requests that fix something label Feb 28, 2020
@NicolasMahe NicolasMahe changed the title Make sure to call end block for each modules Call end block function in each cosmos modules Mar 2, 2020
@NicolasMahe NicolasMahe mentioned this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release:fix Pull requests that fix something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants