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

apptesting: Switch BeginBlocker and EndBlocker calls to be ABCI calls #2740

Closed
3 tasks
Tracked by #4622
ValarDragon opened this issue Sep 14, 2022 · 2 comments
Closed
3 tasks
Tracked by #4622

Comments

@ValarDragon
Copy link
Member

Background

The SDK baseapp has a very undocumented / hard to understand dependence on new Ctx generations, that are needed for App.DeliverTx to work with the same go ctx we use in test code. This is something we reverse engineered to be correct within the simulator, and just painstakingly did for osmosis-rust.

Our go tests currently work off of a ctx that is distinct from what would be used for DeliverTx's if we used both. In #2575 we fixed one of the problems.

It remains that we need to fix the s.App.BeginBlocker(ctx, req) and s.App.EndBlocker(Ctx, req) calls, to instead be the ABCI calls s.App.BeginBlock(req) and s.App.EndBlock(req) . We likely need to ensure that the entire begin block / endblock / (optional commit) flow is happening correctly in tests. (And were exposing the sensible API's from our code)

Suggested Design

  • Change the beginblock + endblock calls to be ABCI method
  • Map the flow were doing in failing tests, understand if we have API issues or more changes that need to happen, etc.

Acceptance Criteria

  • We only call ABCI calls for begin/endblock in apptesting
@ValarDragon ValarDragon added this to the Core Ice Box milestone Sep 14, 2022
@osmo-bot osmo-bot moved this to Needs Review 🔍 in Osmosis Chain Development Sep 14, 2022
@hieuvubk
Copy link
Contributor

hieuvubk commented Oct 4, 2022

I got a problem when changing BeginBlocker to BeginBlock, especially in upgrades_test. The LastBlockHeight from the previous block's commit is required to start a new block.
Looks like we are just changing the context Header without committing

@ValarDragon
Copy link
Member Author

Lets close for now, we have to revisit anyway when we ugprade to SDK's runtime module

@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✅ in Osmosis Chain Development Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants