chore: remove OnChanUpgradeRestore callbacks and discard state changes on app upgrade callbacks
#5696
Conversation
damiannolan
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
chatton,
DimitrisJim and
srdtrk
as code owners
|
Linter will likely cry oceans about defining consts for re-used strings. I can fix it later! |
modules/core/keeper/msg_server.go
Outdated
| @@ -769,8 +769,9 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC | |||
| return nil, errorsmod.Wrap(err, "channel upgrade init failed") | |||
| } | |||
|
|
|||
| cacheCtx, _ := ctx.CacheContext() // NOTE: the writeFn is discarded and application state changes are not committed. | |||
There was a problem hiding this comment.
nit: would be nice to have the docstring above
There was a problem hiding this comment.
Also a note that events will also not be emitted
There was a problem hiding this comment.
[nit] I wonder is it worth adding a helper function to like disposeContext() to make it super clear we're not using a cacheCtx to maybe call it later, we're ensuring it is thrown away from the get go.
There was a problem hiding this comment.
It also might be nice to include the why for this docstring, I can see myself coming back to this later thinking "why did we do this again?" 😅
There was a problem hiding this comment.
I added additional context in the in-line docstrings!
[nit] I wonder is it worth adding a helper function to like disposeContext() to make it super clear we're not using a cacheCtx to maybe call it later, we're ensuring it is thrown away from the get go.
I like the idea, but I feel like most cosmos devs are generally comfortable with the concept of the "cached context" and that terminology. It might be just introducing more cognitive load with a function which essentially just drops the writeFn. I think its clear what this is doing and I appreciate one less "go to definition" if I'm trying to verify code! Happy to discuss on an issue if there's appetite for changing it
modules/core/keeper/msg_server.go
Outdated
| @@ -769,8 +769,9 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC | |||
| return nil, errorsmod.Wrap(err, "channel upgrade init failed") | |||
| } | |||
|
|
|||
| cacheCtx, _ := ctx.CacheContext() // NOTE: the writeFn is discarded and application state changes are not committed. | |||
There was a problem hiding this comment.
[nit] I wonder is it worth adding a helper function to like disposeContext() to make it super clear we're not using a cacheCtx to maybe call it later, we're ensuring it is thrown away from the get go.
modules/core/keeper/msg_server.go
Outdated
| @@ -769,8 +769,9 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC | |||
| return nil, errorsmod.Wrap(err, "channel upgrade init failed") | |||
| } | |||
|
|
|||
| cacheCtx, _ := ctx.CacheContext() // NOTE: the writeFn is discarded and application state changes are not committed. | |||
There was a problem hiding this comment.
It also might be nice to include the why for this docstring, I can see myself coming back to this later thinking "why did we do this again?" 😅
|
I like |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5696 +/- ##
==========================================
+ Coverage 81.18% 81.27% +0.09%
==========================================
Files 199 199
Lines 15289 15231 -58
==========================================
- Hits 12412 12379 -33
+ Misses 2408 2387 -21
+ Partials 469 465 -4
|
damiannolan
deleted the
damian/5692-remove-restore-cb-discard-app-state
branch
…ges on app upgrade callbacks (#5696) * docs: remove refs to OnChanUpgradeRestore in docs * testing: rm OnChanUpgradeRestore callback from mock app test helpers * chore: rm OnChanUpgradeRestore application callback from ibc core and apps * chore: discard app state changes by using a cacheCtx and discarding writeFn * docs: add godoc notes to app callbacks discarding state changes * test: adding unit tests for discarding app state changes * chore: clean imports * chore: review suggestions for in-line docstring comments * chore: fix linting tests, add mock types for testing kv and events * doc: add more context to in-line docstrings (cherry picked from commit 9faaff5) # Conflicts: # docs/docs/01-ibc/06-channel-upgrades.md # modules/core/keeper/msg_server.go # modules/core/keeper/msg_server_test.go
…ges on app upgrade callbacks (backport #5696) (#5701) * chore: remove `OnChanUpgradeRestore` callbacks and discard state changes on app upgrade callbacks (#5696) * docs: remove refs to OnChanUpgradeRestore in docs * testing: rm OnChanUpgradeRestore callback from mock app test helpers * chore: rm OnChanUpgradeRestore application callback from ibc core and apps * chore: discard app state changes by using a cacheCtx and discarding writeFn * docs: add godoc notes to app callbacks discarding state changes * test: adding unit tests for discarding app state changes * chore: clean imports * chore: review suggestions for in-line docstring comments * chore: fix linting tests, add mock types for testing kv and events * doc: add more context to in-line docstrings (cherry picked from commit 9faaff5) # Conflicts: # docs/docs/01-ibc/06-channel-upgrades.md # modules/core/keeper/msg_server.go # modules/core/keeper/msg_server_test.go * resolve conflicts --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Description
OnChanUpgradeRestorecallbackcloses: #5692
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.