imp: remove `timestamp` from `consensusState` and query contract to retrieve it #4034

crodriguezvega · 2023-07-07T09:40:22Z

Description

Still work in progress, since we need new versions of the contracts supporting the query to get the timestamp at a certain height. Tests are expected to fail.

closes: #3957

Commit Message / Changelog Entry

type: commit message

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.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

…om-consensusstate-and-query-contract-to-retrieve-it

crodriguezvega · 2023-07-09T12:58:17Z

modules/light-clients/08-wasm/types/vm.go

+type metadataQueryResponse struct {
+	contractResult
+	GenesisMetadata []clienttypes.GenesisMetadata `json:"genesis_metadata"`
+}
+
+type timestampAtHeightQueryResponse struct {
+	contractResult
+	Timestamp uint64 `json:"timestamp"`
+}
+
+type CheckForMisbehaviourExecuteResult struct {
+	contractResult
+	FoundMisbehaviour bool `json:"found_misbehaviour"`
+}


I added these types for the different query results that we obtain.

crodriguezvega · 2023-07-09T13:03:10Z

modules/light-clients/08-wasm/types/client_state.go

+}
+
+// wasmQuery queries the contract with the given payload and writes the result to output.
+func wasmQuery[T ContractResult](ctx sdk.Context, clientStore sdk.KVStore, cs *ClientState, payload any) (T, error) {


I added this function (which is very similar to call above) to wrap the query call to the contract. Since the query (still to be added) to return the timestamp at a certain height (with payload of type timestampAtHeightQueryResponse) might return an error if there's no consensus at that given height, I thought that we can also check the result and wrap the error if there's any.

I had to named this function like this and not query, because query conflicted with another function defined in a package in the SDK that is also in scope. I will probably rename call above to wasmExecute, for consistency.

…om-consensusstate-and-query-contract-to-retrieve-it

modules/light-clients/08-wasm/types/client_state.go

vmarkushin · 2023-07-21T01:36:44Z

modules/light-clients/08-wasm/types/consensus_state.go

@@ -24,15 +22,11 @@ func (cs ConsensusState) ClientType() string {

 // GetTimestamp returns block time in nanoseconds of the header that created consensus state.
 func (cs ConsensusState) GetTimestamp() uint64 {
-	return cs.Timestamp
+	return 0


Shouldn't we query the contract in this case?

No, the idea is that we will mark this function as deprecated, since it is only used in the light clients, so it doesn't need to be part of the ConsensusState interface. In the meantime, until we remove it, we will just return 0.

* Use hasConsensusState instead of GetConsensusState.

* Use wasmQuery for the rest of the query functions. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

…om-consensusstate-and-query-contract-to-retrieve-it

modules/light-clients/08-wasm/types/client_state.go

modules/light-clients/08-wasm/types/vm.go

damiannolan

Leaving my approval, left some suggestions 👍

damiannolan · 2023-08-01T09:26:19Z

modules/light-clients/08-wasm/types/client_state.go

+type (
+	timestampAtHeightInnerPayload struct {
+		Height exported.Height `json:"height"`
+	}
+	timestampAtHeightPayload struct {
+		TimestampAtHeight timestampAtHeightInnerPayload `json:"timestamp_at_height"`
+	}
+)


this is also superseded by #4133, right?

I'd like to not use type blocks, e.g.:

type ( someType struct {} someOtherType struct {} )

instead:

type someType struct {} type someOtherType struct {}

Just for my own understanding: what's wrong with type blocks?

@chatton doesn't like them! 😆

I think I have similar feelings!

Well, I don't want to get @chatton angry, so...

modules/light-clients/08-wasm/types/client_state.go

damiannolan · 2023-08-01T09:30:52Z

modules/light-clients/08-wasm/types/client_state.go

+
+// wasmQuery queries the contract with the given payload and writes the result to output.
+func wasmQuery[T ContractResult](ctx sdk.Context, clientStore sdk.KVStore, cs *ClientState, payload any) (T, error) {
+	var output T


is this output, result, or response?

I see these three different terms being used interchangeably in a lot of places, we be good to standardise on one.
I'm happy enough with result / ContractResult, response or ContractResponse is also fine imo! :)

Did some renaming.

damiannolan · 2023-08-01T09:32:11Z

modules/light-clients/08-wasm/types/vm.go

+	Timestamp uint64 `json:"timestamp"`
+}
+
+type checkForMisbehaviourExecuteResult struct {


this should be checkForMisbehaviourReponse or? how come the different naming?

Renamed as well.

damiannolan · 2023-08-01T09:33:16Z

modules/light-clients/08-wasm/types/vm.go

+type statusQueryResponse struct {
+	contractResult
+	Status exported.Status `json:"status"`
+}
+
+type metadataQueryResponse struct {
+	contractResult
+	GenesisMetadata []clienttypes.GenesisMetadata `json:"genesis_metadata"`
+}
+
+type timestampAtHeightQueryResponse struct {
+	contractResult
+	Timestamp uint64 `json:"timestamp"`
+}


In #4133 all the contract payload types are in msgs.go right? Should these be included alongside them?

Even tho they are unexported, would be nice to have basic godocs.

I will add them in #4133.

modules/light-clients/08-wasm/types/store.go

* Add linting for 08-wasm. * Go mod tidy. * Please the linter.

* add 08-wasm global store key * Update client_state.go * Update modules/light-clients/08-wasm/types/vm.go Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>

…om-consensusstate-and-query-contract-to-retrieve-it

crodriguezvega added 3 commits July 7, 2023 11:38

imp: remove timestamp from consensus state

3e8e0d5

more linting

062a2c8

Merge branch 'feat/wasm-clients' into carlos/3957-remove-timestamp-fr…

252e07a

…om-consensusstate-and-query-contract-to-retrieve-it

crodriguezvega added the 08-wasm label Jul 7, 2023

make types unexported

c3bc9e5

crodriguezvega commented Jul 9, 2023

View reviewed changes

crodriguezvega mentioned this pull request Jul 19, 2023

imp: add query and sudo message types to encapsulate all variants of calls #4133

Merged

9 tasks

crodriguezvega added 2 commits July 20, 2023 21:30

Merge branch 'feat/wasm-clients' into carlos/3957-remove-timestamp-fr…

b545d95

…om-consensusstate-and-query-contract-to-retrieve-it

make proto-all

3e6a8df

vmarkushin reviewed Jul 21, 2023

View reviewed changes

crodriguezvega and others added 4 commits July 21, 2023 22:31

code hash for code id

ec4273d

Use hasConsensusState instead of GetConsensusState (#4171)

518291d

* Use hasConsensusState instead of GetConsensusState.

Use wasmQuery for the rest of the query functions. (#4176)

3d2eb10

* Use wasmQuery for the rest of the query functions. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

Merge branch 'feat/wasm-clients' into carlos/3957-remove-timestamp-fr…

5d73b93

…om-consensusstate-and-query-contract-to-retrieve-it

crodriguezvega marked this pull request as ready for review August 1, 2023 08:00

crodriguezvega requested review from colin-axner, AdityaSripal, damiannolan, charleenfei, chatton and DimitrisJim as code owners August 1, 2023 08:00

DimitrisJim reviewed Aug 1, 2023

View reviewed changes

modules/light-clients/08-wasm/types/client_state.go Outdated Show resolved Hide resolved

modules/light-clients/08-wasm/types/vm.go Outdated Show resolved Hide resolved

review comments

afdec57

damiannolan approved these changes Aug 1, 2023

View reviewed changes

DimitrisJim and others added 6 commits August 1, 2023 13:25

Remove check for consensus state.

7718cd7

Add linting for 08-wasm module (#4206)

2afde1c

* Add linting for 08-wasm. * Go mod tidy. * Please the linter.

add 08-wasm global store key (#4185)

bf39013

* add 08-wasm global store key * Update client_state.go * Update modules/light-clients/08-wasm/types/vm.go Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com> --------- Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>

solve conflicts

f179a7b

Merge branch 'feat/wasm-clients' into carlos/3957-remove-timestamp-fr…

8f10683

…om-consensusstate-and-query-contract-to-retrieve-it

review comments/fix linter warnings

c6ce87d

crodriguezvega added 3 commits August 1, 2023 14:06

address comments

f608d7c

add missing check

1037138

remove check that was wrong

95385ba

crodriguezvega merged commit 644367f into feat/wasm-clients Aug 1, 2023
20 of 21 checks passed

crodriguezvega deleted the carlos/3957-remove-timestamp-from-consensusstate-and-query-contract-to-retrieve-it branch August 1, 2023 12:28

crodriguezvega mentioned this pull request Aug 1, 2023

Remove timestamp from ConsensusState and query contract to retrieve it #3957

Closed

3 tasks

faddat mentioned this pull request Sep 10, 2023

deps: update wasm-clients to sdk50 #4616

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp: remove `timestamp` from `consensusState` and query contract to retrieve it #4034

imp: remove `timestamp` from `consensusState` and query contract to retrieve it #4034

crodriguezvega commented Jul 7, 2023 •

edited

Loading

crodriguezvega Jul 9, 2023

crodriguezvega Jul 9, 2023 •

edited

Loading

crodriguezvega Jul 9, 2023

vmarkushin Jul 21, 2023

crodriguezvega Jul 21, 2023

damiannolan left a comment

damiannolan Aug 1, 2023

crodriguezvega Aug 1, 2023

damiannolan Aug 1, 2023

crodriguezvega Aug 1, 2023

damiannolan Aug 1, 2023

crodriguezvega Aug 1, 2023 •

edited

Loading

damiannolan Aug 1, 2023

crodriguezvega Aug 1, 2023

damiannolan Aug 1, 2023

crodriguezvega Aug 1, 2023

imp: remove timestamp from consensusState and query contract to retrieve it #4034

imp: remove timestamp from consensusState and query contract to retrieve it #4034

Conversation

crodriguezvega commented Jul 7, 2023 • edited Loading

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

crodriguezvega Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imp: remove `timestamp` from `consensusState` and query contract to retrieve it #4034

imp: remove `timestamp` from `consensusState` and query contract to retrieve it #4034

crodriguezvega commented Jul 7, 2023 •

edited

Loading

crodriguezvega Jul 9, 2023 •

edited

Loading

crodriguezvega Aug 1, 2023 •

edited

Loading