-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
ibc: client params allowlist #7855
Conversation
…osmos-sdk into fedekunze/7825-client-allowlist
Codecov Report
@@ Coverage Diff @@
## master #7855 +/- ##
==========================================
- Coverage 54.21% 54.18% -0.04%
==========================================
Files 610 612 +2
Lines 38868 38960 +92
==========================================
+ Hits 21071 21109 +38
- Misses 15639 15692 +53
- Partials 2158 2159 +1 |
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.
Just a nit on client type names, otherwise this logic looks fine.
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.
overall looks good, agree with @cwgoes comments
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.
Approve, minor fixes that can be added along with other requested changes
x/ibc/core/02-client/types/params.go
Outdated
|
||
var ( | ||
// DefaultAllowedClients are "Solo Machine" and "Tendermint" | ||
DefaultAllowedClients = []string{"Solo Machine", "Tendermint"} |
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.
Should LocalHost
be added as well?
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.
we could consider adding it and remove the CreateLocalhost boolean from GenesisState. Although I'd prefer to do that on another PR
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.
ACK
* ibc: client params allowlist * genesis and gRPC * client * lint * spec * fixes * validate localhost client * move client types back to exported * update genesis * sort clients by id
closes #7825