imp: remove height from header and merge header with misbehaviour
#4049
Conversation
| type checkForMisbehaviourExecuteResult struct { | ||
| contractResult | ||
| FoundMisbehaviour bool `json:"found_misbehaviour"` | ||
| } | ||
|
|
||
| type updateStateExecuteResult struct { | ||
| contractResult | ||
| Heights []exported.Height `json:"heights"` | ||
| } |
There was a problem hiding this comment.
Added these types for the different results.
There was a problem hiding this comment.
same comment from other PR about response vs result naming :)
| clientMessage struct { | ||
| Header *Header `json:"header,omitempty"` | ||
| Misbehaviour *Misbehaviour `json:"misbehaviour,omitempty"` | ||
| ClientMessage *ClientMessage `json:"client_message"` |
There was a problem hiding this comment.
This is how the payload to the contract call looks after merging Header and Misbehaviour.
…header-and-merge-header-with-misbehaviour # Conflicts: # modules/light-clients/08-wasm/types/header_test.go # modules/light-clients/08-wasm/types/misbehaviour.go # modules/light-clients/08-wasm/types/misbehaviour_test.go # modules/light-clients/08-wasm/types/upgrade.go # modules/light-clients/08-wasm/types/wasm.pb.go
…header-and-merge-header-with-misbehaviour
crodriguezvega
requested review from
colin-axner,
AdityaSripal,
damiannolan,
charleenfei,
chatton and
DimitrisJim
as code owners
|
|
||
| for _, tc := range testCases { | ||
| suite.Run(tc.name, func() { | ||
| clientMessage := tc.clientMessage |
There was a problem hiding this comment.
nit: redundant var, but feel free to ignore, its probably fine
| if !ok { | ||
| panic(fmt.Errorf("expected type %T, got %T", &Header{}, clientMsg)) | ||
| panic(fmt.Errorf("expected type %T, got %T", &ClientMessage{}, clientMsg)) |
There was a problem hiding this comment.
if we're going to panic here, then we should also panic in CheckForMisbehaviour and UpdateStateOnMisbehaviour if clientMsg is not *ClientMessage, right?
There was a problem hiding this comment.
I think this mostly followed what is done for tendermint. For CheckForMisbehaviour, returning false would call UpdateState and result in that panic being hit. I guess panicking could be done here but it wouldn't save us much right?
| type checkForMisbehaviourExecuteResult struct { | ||
| contractResult | ||
| FoundMisbehaviour bool `json:"found_misbehaviour"` | ||
| } | ||
|
|
||
| type updateStateExecuteResult struct { | ||
| contractResult | ||
| Heights []exported.Height `json:"heights"` | ||
| } |
There was a problem hiding this comment.
same comment from other PR about response vs result naming :)
…header-and-merge-header-with-misbehaviour # Conflicts: # modules/light-clients/08-wasm/types/misbehaviour_handle.go # modules/light-clients/08-wasm/types/vm.go # modules/light-clients/08-wasm/types/wasm.pb.go
crodriguezvega
deleted the
carlos/3958-remove-height-from-header-and-merge-header-with-misbehaviour
branch
Description
Work in progress. Still needs new versions of contracts for tests to pass.
closes: #3958
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.