Remove timestamp from ConsensusState and query contract to retrieve it

[crodriguezvega]

Surfaced from Interchain IBC team's review of 08-wasm at hash https://github.com/strangelove-ventures/ibc-go/commit/96e9930ab87a171a3ae6ac9717ee25bfda77212b

Summary

Remove timestamp from ConsensusState.

Problem Definition

The 08-wasm module is not completely a proxy client, since in the implementation of GetTimestampAtHeight function of the ClientState interface 08-wasm fetches the timestamp from the stored consensus state, instead of delegating to the light client contract the responsibility to provide that information.

The timestamp should also already be encoded in the data field of ConsensusState (and also in the data of Header when updating the client). Having a separate timestamp field in ConsensusState opens the possibility that a relayer constructs a MsgCreateClient with a consensus state where the timestamp does not match the timestamp encoded in the data field.

Proposal

We would like light contracts to implement the GetTimestampAtHeight function of the ClientState interface, so that 08-wasm's implementation simply queries the contract to retrieve the timestamp, instead of retrieving it from the stored consensus state. The implementation of GetTimestamp function of the ConsensusState interface should return 0 (the function will be marked deprecated).

This change impacts existing implementation of light contracts, since they will need to implement an extra query to return the timestamp for the consensus state at a particular height.

Tagging @misko9 @blasrodri to hear their thoughts, since this change impacts the contracts.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[misko9]

The ConsensusState type does not know its code id for calling into the contract. Is the proposal to remove timestamp and add a code id field? Or was there another idea?

[crodriguezvega]

Thank you for your comment, @misko9! Since GetTimestapAtHeight is part of the ClientState implementation and the client state knows the code ID, then it could call the right contract, right? If this is correct, then the proposal would be to just remove timestamp.

[misko9]

Yes, that is correct, but we still have the GetTimestamp() implementation in the ConsensusState interface. Do we just panic if it is called? Other than client state's GetTimestampAtHeight, I only see it called in modules/apps/transfer/client/cli/tx.go.

[crodriguezvega]

Sorry, now I know what you mean. Yes, that was actually another change we wanted to make. It turns out that GetTimestamp function for the ConsensusState interface is not used outside of the clients (except for the transfer CLI file that you link). We have an issue to remove that usage from the transfer CLI, so therefore we would like to make the interface function deprecated (I still need to write an issue for that) and in the meantime return 0 in the implementation of 08-wasm's ConsensusState. I will add that to the issue description. Thanks for noticing it!

[faddat]

Hey @vuong177 I think you might want to have a look at this one as well.

@womensrights thank you very much for pulling my eye over here.

[crodriguezvega]

Closed by #4034