chore: disallow multiple localhost connections #3169
Conversation
damiannolan
requested review from
colin-axner and
AdityaSripal
as code owners
|
NOTE: this is only added to INIT and TRY as the client ID isn't available directly on ACK and CONFIRM msgs. It is probably not required on the TRY msg checks but I think its no harm to add it. |
| if msg.ClientId == exported.Localhost { | ||
| return sdkerrors.Wrap(clienttypes.ErrInvalidClientType, "localhost connetion handshakes are disallowed") | ||
| } |
There was a problem hiding this comment.
I think this isn't strictly necessary as there should not exist a connection in INIT which would have a valid consensus state stored for the localhost. But, I think it makes sense to be clear/defensive
There was a problem hiding this comment.
Yeah, exactly! This was my thinking too :)
|
ACK and CONFIRM do not need the checks because they would require an existing connection with the localhost client ID. There will be only one existing connection in OPEN state, thus negating any usage with ACK and CONFIRM which require the connection state to be in INIT or TRY on the counterparty side |
damiannolan
deleted the
damian/disallow-multiple-localhost-connections
branch
Description
Adds basic checks to clientID on
MsgConnectionOpenInitandMsgConnectionOpenTrybasic validation.closes: #3167
Commit Message / Changelog Entry
NA
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/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.