imp: check order in validate basic of MsgRegisterInterchainAccount
#5671
Conversation
crodriguezvega
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
| @@ -68,7 +70,7 @@ func (ch Channel) ValidateBasic() error { | |||
| if ch.State == UNINITIALIZED { | |||
| return ErrInvalidChannelState | |||
| } | |||
| if !(ch.Ordering == ORDERED || ch.Ordering == UNORDERED) { | |||
| if !slices.Contains([]Order{ORDERED, UNORDERED}, ch.Ordering) { | |||
There was a problem hiding this comment.
I took the liberty to make this small improvement.
| { | ||
| "order is not valid", | ||
| func() { | ||
| msg.Order = channeltypes.NONE |
There was a problem hiding this comment.
Small nit I just realised: in the protos we called this field order for MsgRegisterInterchainAccount, while in MsgChannelOpenInit is called ordering. Should we rename order to ordering for consistency?
There was a problem hiding this comment.
yea, think we should! can push directly here if you want me to.
There was a problem hiding this comment.
Yeah, go for it!
There was a problem hiding this comment.
ah, would be nice to backport that change and just saw about BP this PR. can open as separate instead to have it backport cleanly
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5671 +/- ##
=======================================
Coverage 81.16% 81.17%
=======================================
Files 199 199
Lines 15278 15281 +3
=======================================
+ Hits 12401 12404 +3
Misses 2408 2408
Partials 469 469
|
| @@ -41,6 +42,10 @@ func (msg MsgRegisterInterchainAccount) ValidateBasic() error { | |||
| return errorsmod.Wrap(err, "invalid connection ID") | |||
| } | |||
|
|
|||
| if !slices.Contains([]channeltypes.Order{channeltypes.ORDERED, channeltypes.UNORDERED}, msg.Order) { | |||
There was a problem hiding this comment.
I asked about this in security audit and though it does indeed get handled later on I do think having explicit ordering check here also makes sense. Only worry is something like #5668 occurring again, maybe we can add these checks in spec too?
There was a problem hiding this comment.
Sounds good to include it in the spec as well. I have this PR with ICS 27 updates, so I can include it there.
crodriguezvega
deleted the
carlos/check-order-msgregisterinterchainaccount-validatebasic
branch
Description
Also found while testing with hermes: a valid
MsgRegisterInterchainAccountmessages can be constructed and submitted withNONEorder. The channel will not be created because an error is returned inValidateBasicofMsgChannelOpenInit, but I thought it would be nicer to do the same check in theValidateBasicofMsgRegisterInterchainAccount.However, I was also thinking that maybe we actually shouldn't do this check at all in
ValidateBasicand just set the order toORDEREDif the value passed inregisterInterchainAccountisNONE. That way I think we preserve backwards compatibility better, because upstream users don't need to change the existing code they may have creating aMsgRegisterInterchainAccount, otherwise, they will have to change their code and set a valid ordering.We could leave the
ValidateBasiccheck of this PR and merge it to main, but not backport; and then inrelease/v8.1.xwe could implement the other solution.Looking forward to reading people's thoughts.
closes: #XXXX
Commit Message / Changelog Entry
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.