-
Notifications
You must be signed in to change notification settings - Fork 592
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
imp: add query and sudo message types to encapsulate all variants of calls #4133
imp: add query and sudo message types to encapsulate all variants of calls #4133
Conversation
Path exported.Path `json:"path"` | ||
} | ||
checkForMisbehaviourMsg struct { | ||
ClientMessage exported.ClientMessage `json:"client_message"` |
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.
Instead of having two fields (Header
and Misbehaviour
) we pass directly ClientMessage
, as we discussed during our own internal walkthrough. I think I mentioned this already to Composable, but I will double check.
…capsulate-calls # Conflicts: # modules/light-clients/08-wasm/types/errors.go # modules/light-clients/08-wasm/types/wasm.pb.go
@@ -43,3 +43,10 @@ message Misbehaviour { | |||
|
|||
bytes data = 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.
I brought this over from #3958.
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, much cleaner looking. left some small comments/questions
ClientMessage: clientMsgConcrete, | ||
clientMessage, ok := clientMsg.(*ClientMessage) | ||
if !ok { | ||
return errorsmod.Wrapf(ErrInvalidClientMessage, "expected type %T, got %T", &ClientMessage{}, clientMsg) |
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.
return errorsmod.Wrapf(ErrInvalidClientMessage, "expected type %T, got %T", &ClientMessage{}, clientMsg) | |
return errorsmod.Wrapf(ErrInvalidClientMessage, "expected type %T, got %T", ClientMessage{}, clientMsg) |
I don't think we expect a pointer type here?
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 think this is ok because of the casting clientMessage, ok := clientMsg.(*ClientMessage)
above?
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.
Nice, I really like the QueryMsg
and SudoMsg
types! Much cleaner than having all of these inner/outer things spread around!
I left a couple of comments/suggestions, again nothing major!
…capsulate-calls # Conflicts: # modules/light-clients/08-wasm/go.mod # modules/light-clients/08-wasm/types/client_message.go # modules/light-clients/08-wasm/types/client_state.go # modules/light-clients/08-wasm/types/misbehaviour_handle.go # modules/light-clients/08-wasm/types/update.go # modules/light-clients/08-wasm/types/upgrade.go # modules/light-clients/08-wasm/types/wasm.pb.go
type statusMsg struct{} | ||
type exportMetadataMsg struct{} | ||
type timestampAtHeightMsg struct { | ||
Height exported.Height `json:"height"` | ||
} |
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.
gofumpt disagrees with you guys, @damiannolan and @chatton, and wants to put here a type block. I respect your opinion a lot, guys, you know that, but my religion doesn't allow me to break the 11th commandment: "You will never fight against your beloved linter". :)
I addressed the comments and, even though I don't have the required blessings, I will go ahead and merge it. We can fix anything else in follow-up PRs. |
Description
See item 6.7 of report (TODO: add link when report is available in ibc-go).
Using the code provided by Ethan as reference.
There are parts of this PR that are incomplete because changes from #4033 and #4034 are needed.
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/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.