-
Notifications
You must be signed in to change notification settings - Fork 625
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
chore: disallow localhost client creation in 02-client #3114
Conversation
"success: 06-solomachine client type supported", | ||
solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}), | ||
&solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}, | ||
true, |
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.
I've adapted the tests here slightly. This one was actually setup incorrectly and the case was failing on clientState.Initialise()
due to non-matching consensus state, rather than it being an unsupported client. Solomachine is now in the list of allowed clients so this test case should pass.
Initially I removed the localhost client type from |
@@ -17,7 +17,7 @@ func (k Keeper) CreateClient( | |||
ctx sdk.Context, clientState exported.ClientState, consensusState exported.ConsensusState, | |||
) (string, error) { | |||
params := k.GetParams(ctx) | |||
if !params.IsAllowedClient(clientState.ClientType()) { | |||
if !params.IsAllowedClient(clientState.ClientType()) || clientState.ClientType() == exported.Localhost { | |||
return "", sdkerrors.Wrapf( | |||
types.ErrInvalidClientType, | |||
"client state type %s is not registered in the allowlist", clientState.ClientType(), |
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.
Maybe a nit, but wouldn't this error be a bit misleading for localhost? I mean, the reader of the error would see that localhost is in the allow list...
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.
Yes, I suppose it would!
We can either change the error msg or put the localhost check in its own conditional, what do you think?
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.
Maybe I would prefer then to have it in its own conditional.
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.
Updated!
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.
Thank you, @damiannolan!
Description
09-localhost
clients in02-client
ref: #3034
Commit Message / Changelog Entry
chore: disallow localhost client creation in 02-client
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.