-
Notifications
You must be signed in to change notification settings - Fork 580
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
Add implementations for OnChanUpgradeOpen and OnChanUpgradeRestore for controller. #5467
Add implementations for OnChanUpgradeOpen and OnChanUpgradeRestore for controller. #5467
Conversation
…deRestore for controller.
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.
Awesome, thank you @DimitrisJim! 🚀
testCases := []struct { | ||
name string | ||
malleate func() | ||
expError error |
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.
Suppose there will never an expError
because the func can only panic in one case.
It should be possible to set some mock app or middleware as the app which doesn't implement the interface. Maybe could reuse something from #5406 with expPanics
, where instead of using nil
we use a mock that doesn't implement the interface:
if expPanics {
cbs = controller.NewIBCMiddleware(ibcmock.NewBlockUpgradeMiddleware(), suite.chainA.GetSimApp().ICAControllerKeeper)
}
Happy to do it in a follow up if we go with the mock upgrade middleware
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.
Happy to do it in a follow up if we go with the mock upgrade middleware
Sounds perfect to me, I'll open a quick issue for it. (I'll clean up expError for now)
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.
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 thanks @DimitrisJim
…ks-for-controller
Description
note: all panic in callbacks should be unreachable.
closes: #5457
Commit Message / Changelog Entry
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/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.