feat(client): migrate client params #3640
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| store := ctx.KVStore(k.storeKey) | ||
| bz := store.Get([]byte(types.ParamsKey)) | ||
| if bz == nil { | ||
| panic("ibc client params are not set in store") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
There was a problem hiding this comment.
This is called in BeginBlocker when updating the localhost client. Personally I'd rather avoid panics entirely in these situations.
There was a problem hiding this comment.
Gotcha, will switch to empty params mirroring what the sdk does.
There was a problem hiding this comment.
Is empty params what the sdk does? I know @colin-axner mentioned briefly that he had a minor preference for panicking, but it would be nice if we could avoid it imo.
For context, panicking in the BeginBlocker abci call is not caught by sdk's baseapp. Panicking in msg server handlers is "fine" because its caught by the grpc recovery interceptor, but doing so in BeginBlocker would crash nodes and ultimately halt a chain
There was a problem hiding this comment.
Ah, good point, @damiannolan! I believe the SDK does not panic.
There was a problem hiding this comment.
That seems to be the case for x/bank, x/mint, x/staking and x/slashing. I'm sure others that migrated should follow that same pattern.
I'll hunt down Colin's comment real quick since I might of missed it, it's probably something along the lines of it doesn't make sense to return empty params if none are there.
There was a problem hiding this comment.
My view is that unset params are a sign of a misconfigured chain which should be halted. It seems unsafe to process any transactions if certain parameters on chain haven't been decided on. My recommendation would be defense in depth, as we may choose to add params in the future which might not have a similar sane default value as allowed clients
DimitrisJim
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
| store := ctx.KVStore(k.storeKey) | ||
| bz := store.Get([]byte(types.ParamsKey)) | ||
| if bz == nil { | ||
| panic("ibc client params are not set in store") |
There was a problem hiding this comment.
This is called in BeginBlocker when updating the localhost client. Personally I'd rather avoid panics entirely in these situations.
|
re: And a docs entry is needed in docs/migrations/ just a reference to this issue mentioning how app.go needs to be modified. I saw that mentioned by Colin and was wondering if just adding a single note for all modules that params have been migrated and an example of what needs to change would suffice. All keepers defined in ibc go now need to accept an additional authority argument, etc. Seems like having a diff for all 5(?) modules would be a lot of duplication. Thoughts? |
DimitrisJim
force-pushed
the
jim/3500-migrate-02-client-params-to-be-self-managed
branch
from
6e931d1 to
64e0a1e
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3640 +/- ##
==========================================
- Coverage 78.78% 78.74% -0.04%
==========================================
Files 186 186
Lines 12769 12816 +47
==========================================
+ Hits 10060 10092 +32
- Misses 2278 2291 +13
- Partials 431 433 +2
|
modules/core/keeper/msg_server.go
Outdated
| // UpdateClientParams defines a rpc handler method for MsgUpdateClientParams. | ||
| func (k Keeper) UpdateClientParams(goCtx context.Context, msg *clienttypes.MsgUpdateClientParams) (*clienttypes.MsgUpdateClientParamsResponse, error) { | ||
| ck := k.ClientKeeper | ||
| if ck.GetAuthority() != msg.Authority { |
There was a problem hiding this comment.
since the entrypoint(s) to the msg server for ibc core submodules is in the core keeper, it might make sense for GetAuthority to just exist at the level of the core ibc keeper rather than passing the authority along to each nested keeper (client, connection).
I don't see a world right now in which client would have authority x and connection would have authority y.
Thoughts? cc. @colin-axner
There was a problem hiding this comment.
that's seems like a good point to me. I'll wait for Colin's thoughts on this before making changes.
There was a problem hiding this comment.
I've made this change. Code definitely looks better. Will revert if required.
DimitrisJim
force-pushed
the
jim/3500-migrate-02-client-params-to-be-self-managed
branch
2 times, most recently
from
b8e432f to
5fc3686
Compare
DimitrisJim
force-pushed
the
jim/3500-migrate-02-client-params-to-be-self-managed
branch
from
5fc3686 to
e483b3b
Compare
There was a problem hiding this comment.
Awesome work, thanks @DimitrisJim 🚀
I think we should go ahead and add the migration doc entry in this PR.
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Damian Nolan <[email protected]>
DimitrisJim
deleted the
jim/3500-migrate-02-client-params-to-be-self-managed
branch
Description
closes: #3500
Commit Message / Changelog Entry
feat(client)!: migrate client parameters to be self managedsee 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.