Implement new begin blocker interface #4563
Conversation
…https://github.com/cosmos/ibc-go into cian/add-client-state-height-check-for-localhost-e2e
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4563 +/- ##
==========================================
+ Coverage 79.20% 79.25% +0.04%
==========================================
Files 188 188
Lines 13063 13064 +1
==========================================
+ Hits 10347 10354 +7
+ Misses 2282 2276 -6
Partials 434 434
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| func (am AppModule) BeginBlock(ctx sdk.Context) { | ||
| ibcclient.BeginBlocker(ctx, am.keeper.ClientKeeper) | ||
| func (am AppModule) BeginBlock(ctx context.Context) error { | ||
| ibcclient.BeginBlocker(sdk.UnwrapSDKContext(ctx), am.keeper.ClientKeeper) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
chatton
requested review from
colin-axner,
AdityaSripal,
damiannolan,
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
|
The original issue mentioned a test for stopping/starting a chain in a test, I'm not sure I understand the additional benefit that we get from having that. I think this addition to the localhost test should ensure our begin blocker is correctly wired. Let me know if I'm missing something @colin-axner ! |
It would be to detect the missing begin blocker for the capability module. If you stop and start all nodes, you reset the mem store, if the begin block is missing from capability, the mem store will not be reinitialized and any IBC action afterward, such as sending a packet will fail. In the short term, I think it makes sense to either add a test or add the stop start to the genesis test or IBC transfer. In the long term, we could consider adding randomness into our testing (allowing random tests to be stop/started) |
|
I added a chain restart in the transfer test, and am bumping callbacks to the new rc5 cc @colin-axner |
| t.Run("ensure capability module BeginBlock is executed", func(t *testing.T) { | ||
| // by restarting the chain we ensure that the capability module's BeginBlocker is executed. | ||
| s.Require().NoError(chainA.StopAllNodes(ctx)) | ||
| s.Require().NoError(chainA.StartAllNodes(ctx)) | ||
| s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA), "failed to wait for blocks") | ||
| s.InitGRPCClients(chainA) | ||
| }) |
There was a problem hiding this comment.
I don't think this is the best place to do this. This is happening in every transfer test now?
Seems like it would make more sense to have a dedicated test in genesis_test which covered tearing down nodes and restarting with some existing channel being reused post restart to ensure capabilities are populated
There was a problem hiding this comment.
think the naming is a bit misleading with base_test.go, this is actually just happening in the TestMsgTransfer_Succeeds_Nonincentivized. Happy to move it to its own separate test though.
I might do in a follow up though so we can get closer to tagging v8
chatton
deleted the
cian/add-client-state-height-check-for-localhost-e2e
branch
Description
closes: #4559
Commit Message / Changelog Entry
N/A part of SDK upgrade
see the guidelines for commit messages. (view raw markdown for examples)
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.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.