Make clients use proto Height #7184

AdityaSripal · 2020-08-27T23:14:34Z

Description

This is part 1 of addressing #6531

It accomplishes the following:

create a Height interface in 02-client/exported
create concrete implementation in client/proto
Use concrete implementation in internal state of 07-tendermint client and 09-localhost

What this PR does not do:

Change all core IBC interfaces (02/03/04/24) to use the abstract Height implementation
Retrieve epoch number from chain ID

These will be addressed in subsequent PRs. So effectively the clients that need epoch numbers now use them under the hood, but they still only return uint64 (namely the EpochHeight) in all IBC interfaces

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.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-08-27T23:19:05Z

Codecov Report

Merging #7184 into master will increase coverage by 0.00%.
The diff coverage is 77.00%.

@@           Coverage Diff           @@
##           master    #7184   +/-   ##
=======================================
  Coverage   54.77%   54.77%           
=======================================
  Files         563      564    +1     
  Lines       38612    38661   +49     
=======================================
+ Hits        21148    21177   +29     
- Misses      15733    15752   +19     
- Partials     1731     1732    +1

fedekunze

LGTM. How we are going to check the epoch from the chain-id? I noticed that there are a few places where the epoch is hardcoded to 0 due to that TODO.

fedekunze · 2020-08-28T07:38:48Z

x/ibc/02-client/genesis.go

@@ -43,7 +43,11 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, gs types.GenesisState) {
 	}

 	// client id is always "localhost"
-	clientState := localhosttypes.NewClientState(ctx.ChainID(), ctx.BlockHeight())
+	// Hardcode 0 as epoch number for now
+	// TODO: Retrieve epoch from chain-id


track issue?

fedekunze · 2020-08-28T07:42:35Z

x/ibc/07-tendermint/types/header.go

@@ -71,9 +72,10 @@ func (h Header) ValidateBasic(chainID string) error {

 	// TrustedHeight is less than Header for updates
 	// and less than or equal to Header for misbehaviour
-	if h.TrustedHeight > h.GetHeight() {
+	height := clienttypes.NewHeight(0, h.GetHeight())


should we check the epoch from the chain-id here too?

fedekunze · 2020-08-28T07:44:12Z

x/ibc/07-tendermint/types/misbehaviour_handle.go

@@ -96,7 +96,7 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 		return nil, sdkerrors.Wrap(err, "verifying Header2 in Misbehaviour failed")
 	}

-	cs.FrozenHeight = uint64(tmEvidence.GetHeight())
+	cs.FrozenHeight = clienttypes.NewHeight(0, uint64(tmEvidence.GetHeight()))


ditto epoch from header chain id?

colin-axner

LGTM, only requesting changes since I think there was one semantic change in tendermint.

I'd also prefer if we added LTE and GTE for readability.

proto/ibc/localhost/localhost.proto

colin-axner · 2020-08-28T09:12:31Z

proto/ibc/localhost/localhost.proto

-  uint64 height = 3;
+  ibc.client.Height height = 3 [
+    (gogoproto.moretags) = "yaml:\"height\"",
+    (gogoproto.nullable) = false


does this need to be a pointer for proto?

Would this be a problem for proto encoding/decoding? Given the choice, i prefer it to not be a pointer since there's no reason to have a nil Height for ClientState/ConsensusState and not having nil checks everywhere is simpler

proto/ibc/tendermint/tendermint.proto

colin-axner · 2020-08-28T09:14:50Z

proto/ibc/client/client.proto

+// In these cases, the epoch number is incremented so that height continues to be monitonically increasing
+// even as the EpochHeight gets reset
+message Height {
+  option (gogoproto.goproto_stringer) = false;


why this option?

I want to be able to define my own String() method on Height, especially since it will eventually go into host.ConsensusStatePath

x/ibc/02-client/types/height.go

x/ibc/07-tendermint/types/misbehaviour_handle_test.go

x/ibc/07-tendermint/types/misbehaviour_handle.go

x/ibc/07-tendermint/types/misbehaviour.go

x/ibc/02-client/types/height.go

colin-axner · 2020-08-28T09:48:08Z

How we are going to check the epoch from the chain-id?

chain-id's will be forced to follow a specific format like appending -<epochnumber> to their chain-id

fedekunze · 2020-08-28T11:06:02Z

chain-id's will be forced to follow a specific format like appending - to their chain-id

note that won't work for Ethermint as the EVM requires an integer for chain-id

colin-axner · 2020-08-28T11:07:31Z

note that won't work for Ethermint as the EVM requires an integer for chain-id

cc/ @cwgoes

cwgoes · 2020-08-28T14:08:57Z

note that won't work for Ethermint as the EVM requires an integer for chain-id

cc/ @cwgoes

Hmm, we can just parse the integer instead, I suppose. This is a bit of a hack but since Tendermint doesn't natively support epochs we have to put the epoch number in something which will be signed-over.

Co-authored-by: colin axnér <[email protected]>

AdityaSripal · 2020-08-28T19:28:33Z

As mentioned above, currently hardcoding epoch-number as 0 for now. Will retrieve from chain-id in future PR

cwgoes

ACK modulo follow-ups

Addressed

…to aditya/proto-height

AdityaSripal added 5 commits August 27, 2020 16:59

start using Height in TM client

3ab74c0

fix client tests

2618b21

fix client tests

5c229ed

fix connection tests

cc88998

fix rest of tests

249c00e

AdityaSripal added the x/ibc label Aug 27, 2020

AdityaSripal requested review from cwgoes, colin-axner and fedekunze August 27, 2020 23:14

AdityaSripal requested review from aaronc, alessio and alexanderbez as code owners August 27, 2020 23:14

fedekunze approved these changes Aug 28, 2020

View reviewed changes

Merge branch 'master' into aditya/proto-height

d6a7bec

colin-axner previously requested changes Aug 28, 2020

View reviewed changes

AdityaSripal and others added 5 commits August 28, 2020 14:03

Apply suggestions from code review

e0e0f98

Co-authored-by: colin axnér <[email protected]>

fix simple issues

6010b97

add and use LTE and GTE methods

99457df

remove TM-specific height semantics from 02-client

226e655

Merge branch 'master' into aditya/proto-height

c502700

AdityaSripal mentioned this pull request Aug 28, 2020

Retrieve epoch numbers from chain-id #7194

Closed

fedekunze requested a review from colin-axner August 28, 2020 20:28

cwgoes approved these changes Aug 30, 2020

View reviewed changes

AdityaSripal added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 31, 2020

AdityaSripal added 3 commits August 31, 2020 13:34

fix merge

1d43fbe

Merge branch 'aditya/proto-height' of github.com:cosmos/cosmos-sdk in…

fed4491

…to aditya/proto-height

fix lint and build

5eefea7

mergify bot merged commit d208d2b into master Aug 31, 2020

mergify bot deleted the aditya/proto-height branch August 31, 2020 17:57

AdityaSripal mentioned this pull request Sep 15, 2020

Abstract Height in IBC Clients #6686

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make clients use proto Height #7184

Make clients use proto Height #7184

AdityaSripal commented Aug 27, 2020

codecov bot commented Aug 27, 2020 •

edited

Loading

fedekunze left a comment

fedekunze Aug 28, 2020

fedekunze Aug 28, 2020

fedekunze Aug 28, 2020

colin-axner left a comment •

edited

Loading

colin-axner Aug 28, 2020

AdityaSripal Aug 28, 2020

colin-axner Aug 28, 2020

AdityaSripal Aug 28, 2020

colin-axner commented Aug 28, 2020

fedekunze commented Aug 28, 2020

colin-axner commented Aug 28, 2020

cwgoes commented Aug 28, 2020

AdityaSripal commented Aug 28, 2020

cwgoes left a comment

Make clients use proto Height #7184

Make clients use proto Height #7184

Conversation

AdityaSripal commented Aug 27, 2020

Description

codecov bot commented Aug 27, 2020 • edited Loading

Codecov Report

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Aug 28, 2020

Choose a reason for hiding this comment

fedekunze Aug 28, 2020

Choose a reason for hiding this comment

fedekunze Aug 28, 2020

Choose a reason for hiding this comment

colin-axner left a comment • edited Loading

Choose a reason for hiding this comment

colin-axner Aug 28, 2020

Choose a reason for hiding this comment

AdityaSripal Aug 28, 2020

Choose a reason for hiding this comment

colin-axner Aug 28, 2020

Choose a reason for hiding this comment

AdityaSripal Aug 28, 2020

Choose a reason for hiding this comment

colin-axner commented Aug 28, 2020

fedekunze commented Aug 28, 2020

colin-axner commented Aug 28, 2020

cwgoes commented Aug 28, 2020

AdityaSripal commented Aug 28, 2020

cwgoes left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 27, 2020 •

edited

Loading

colin-axner left a comment •

edited

Loading