Modular IBC Client #7028

AdityaSripal · 2020-08-12T21:39:42Z

Description

closes: #6064

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-12T21:43:41Z

Codecov Report

Merging #7028 into master will increase coverage by 0.00%.
The diff coverage is 38.67%.

@@           Coverage Diff           @@
##           master    #7028   +/-   ##
=======================================
  Coverage   61.97%   61.98%           
=======================================
  Files         520      520           
  Lines       32041    32015   -26     
=======================================
- Hits        19856    19843   -13     
+ Misses      10552    10542   -10     
+ Partials     1633     1630    -3

x/ibc/02-client/handler.go

fedekunze · 2020-08-13T08:18:25Z

x/ibc/02-client/handler.go

 		consensusHeight = uint64(ctx.BlockHeight())
 	default:
-		return nil, sdkerrors.Wrapf(types.ErrInvalidClientType, "unsupported client type (%s)", msg.GetClientType())
+		consState := msg.GetConsensusState()
+		if consState == nil {


I'm not sure that returning an error is good? I for eg the localhost client would return nil here. I'd just set the consensusHeight = 0 in this case

this is a good point. @cwgoes thoughts? Is it likely that clients outside of localhost would not have consensus states?

Went with fede's suggestion

fedekunze

Looking good. Left a few comments 👍

x/ibc/02-client/handler.go

x/ibc/02-client/keeper/encoding.go

fedekunze · 2020-08-13T08:22:48Z

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

+
+	// Retrieve trusted consensus states for each Header in misbehaviour
+	// and unmarshal from clientStore
+	consBytes1 := clientStore.Get(host.KeyConsensusState(tmEvidence.Header1.TrustedHeight))


nit, can you add a few more spacing between lines? it's too packed lol

also use GetConsensusState helper func

I can't because I'm using the prefix.Store which only implements Get and Set, I think this is preferable to giving the specific clients access to the client.Keeper

see my comment on update.go, but the helper function I was referring to is in 07-tendermint/types/store.go not the client keeper

x/ibc/09-localhost/types/msgs.go

colin-axner

first pass LGTM, this will greatly reduce the complexity in 02-client 💯

x/ibc/02-client/exported/exported.go

x/ibc/02-client/handler.go

colin-axner · 2020-08-13T09:50:25Z

x/ibc/02-client/handler.go

 		consensusHeight = uint64(ctx.BlockHeight())
 	default:
-		return nil, sdkerrors.Wrapf(types.ErrInvalidClientType, "unsupported client type (%s)", msg.GetClientType())
+		consState := msg.GetConsensusState()
+		if consState == nil {


this is a good point. @cwgoes thoughts? Is it likely that clients outside of localhost would not have consensus states?

x/ibc/02-client/handler.go

colin-axner · 2020-08-13T09:52:00Z

x/ibc/02-client/keeper/client.go

@@ -69,34 +66,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		err             error
 	)

-	switch clientType {
-	case exported.Tendermint:


x/ibc/02-client/keeper/encoding.go

colin-axner · 2020-08-13T09:58:23Z

x/ibc/09-localhost/types/client_state.go

@@ -72,6 +72,26 @@ func (cs ClientState) GetProofSpecs() []*ics23.ProofSpec {
 	return nil
 }

+// CheckValidityAndUpdateState updates the localhost client
+// It only needs access to the context


nit: only do line breaks when the line goes to far to the right, not per idea. This is pretty standard across the SDK and I find it easier to read/look at because it is standard

colin-axner · 2020-08-13T10:01:22Z

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

-	tmConsState, ok := consState.(*types.ConsensusState)
-	if !ok {
+	// Get trusted consensus state and unmarshal from clientStore
+	consBytes := clientStore.Get(host.KeyConsensusState(tmHeader.TrustedHeight))


use helper function GetConsensusState

I can't because I'm using the prefix.Store which only handles Get and Set, I think this is preferable to giving the specific clients access to the client.Keeper

I'm confused? I was referring to the helper in x/ibc/07-tendermint/types/store.go

// GetConsensusState retrieves the consensus state from the client prefixed // store. An error is returned if the consensus state does not exist. func GetConsensusState(store sdk.KVStore, cdc codec.BinaryMarshaler, height uint64) (*ConsensusState, error) {

This isn't giving access to the keeper

😮 I did not realize that function existed!! That's a lot cleaner! Thanks @colin-axner !!

colin-axner · 2020-08-13T10:02:52Z

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

+
+	// Retrieve trusted consensus states for each Header in misbehaviour
+	// and unmarshal from clientStore
+	consBytes1 := clientStore.Get(host.KeyConsensusState(tmEvidence.Header1.TrustedHeight))


also use GetConsensusState helper func

…into aditya-modular-client

AdityaSripal · 2020-08-13T23:00:56Z

simapp/test_helpers.go

@@ -35,7 +35,7 @@ var DefaultConsensusParams = &abci.ConsensusParams{
 	},
 	Evidence: &abci.EvidenceParams{
 		MaxAgeNumBlocks: 302400,
-		MaxAgeDuration:  1814400,


THis used to be nanoseconds

AdityaSripal · 2020-08-13T23:08:29Z

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

-	ageDuration := currentTimestamp.Sub(infractionTime)
-	ageBlocks := uint64(infractionHeight) - height
+	ageDuration := ctx.BlockTime().Sub(infractionTime)
+	ageBlocks := int64(cs.LatestHeight) - infractionHeight


ageBlocks was being incorrectly calculated, fixed now.

AdityaSripal · 2020-08-13T23:09:09Z

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

 			suite.now,
 			false,
 		},
 		{
-			"rejected misbehaviour due to expired age",
+			"rejected misbehaviour due to expired age duration",


Tested incorrectly, fixed now

AdityaSripal · 2020-08-13T23:16:41Z

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

+		(ageDuration > consensusParams.Evidence.MaxAgeDuration ||
+			ageBlocks > consensusParams.Evidence.MaxAgeNumBlocks) {


This must be OR not AND

fedekunze

looks clean! thanks @AdityaSripal

colin-axner

This pr is a huge improvement 🎉 . I left some nits, but my main request is that we use the GetConsensusState helper function in 07-tendermint/types/store.go. I think there was some confusion on what I meant with my initial review comment, but there exists redundant code and it will be much easier for others to read

colin-axner · 2020-08-18T09:17:55Z

x/ibc/02-client/handler_test.go

+	localhosttypes "github.com/cosmos/cosmos-sdk/x/ibc/09-localhost/types"
+)
+
+func (suite *ClientTestSuite) TestHandleCreateClientLocalHost() {


it would have been nice to note that since suite.SetupTest() is not being ran, test case state changes are persisted through each test meaning test case ordering matters. This could be confusing for later developers who modify this test or copy its structure.

colin-axner · 2020-08-18T09:19:05Z

x/ibc/02-client/handler_test.go

+
+			clientState, ok := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), tc.clientID)
+			suite.Require().True(ok, "could not retrieve clientState")
+			suite.Require().NotNil(clientState, "clientstate is nil")


checking the clientState.(type) would have been nice here as well to ensure the correct client state was created for the clientID

colin-axner · 2020-08-18T09:24:34Z

x/ibc/testing/chain.go

-func (chain *TestChain) CreateTMClient(counterparty *TestChain, clientID string) error {
-	// construct MsgCreateClient using counterparty
-	msg := ibctmtypes.NewMsgCreateClient(
+func (chain *TestChain) ConstructMsgCreateClient(counterparty *TestChain, clientID string) clientexported.MsgCreateClient {


some godoc here would have been nice or a different name since this creates specifically a tendermint client

colin-axner · 2020-08-18T09:27:17Z

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

+
+	// Retrieve trusted consensus states for each Header in misbehaviour
+	// and unmarshal from clientStore
+	consBytes1 := clientStore.Get(host.KeyConsensusState(tmEvidence.Header1.TrustedHeight))


see my comment on update.go, but the helper function I was referring to is in 07-tendermint/types/store.go not the client keeper

colin-axner · 2020-08-18T09:29:51Z

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

-		ageDuration > consensusParams.Evidence.MaxAgeDuration &&
-		ageBlocks > uint64(consensusParams.Evidence.MaxAgeNumBlocks) {
+		(ageDuration > consensusParams.Evidence.MaxAgeDuration ||
+			ageBlocks > consensusParams.Evidence.MaxAgeNumBlocks) {
 		return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidEvidence,
 			"age duration (%s) and age blocks (%d) are greater than max consensus params for duration (%s) and block (%d)",


this error message should be updated to reflect the logic change

colin-axner · 2020-08-18T09:30:53Z

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


 import (
 	"bytes"
+	fmt "fmt"


colin-axner · 2020-08-18T09:33:47Z

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

-		}
+			if tc.expPass {
+				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
+				suite.Require().NotNil(clientState, "valid test case %d failed: %s", i, tc.name)


nit: when using suite.Run since the case name is passed in and printed out upon an error, I like to leave these error messages blank unless providing useful notes on why it might have failed

colin-axner · 2020-08-18T09:40:05Z

x/ibc/02-client/keeper/client.go

@@ -69,34 +66,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
 		err             error
 	)



It's a little strange the the client keeper has UpdateClient and CheckMisbehaviourAndUpdateState. We should update these to be consistent? These funcs also do checks on the client state

* use cleaner helper function * fix error strings * lint * typos Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* start modular client work * fix panic * reuse keeper marshal methods * readd TODO * add nil checks for misbehaviour * address reviews * address rest of reviews and fix builds * fixed tests * address rest of reviews * fix expired blocks bug * fix expired bug

start modular client work

2c6f3d0

AdityaSripal requested review from colin-axner and fedekunze August 12, 2020 21:39

AdityaSripal added the x/ibc label Aug 12, 2020

fedekunze reviewed Aug 12, 2020

View reviewed changes

x/ibc/02-client/handler.go Outdated Show resolved Hide resolved

AdityaSripal added 2 commits August 12, 2020 18:28

fix panic

4763e87

reuse keeper marshal methods

2595376

AdityaSripal mentioned this pull request Aug 12, 2020

Modular 02-client #6515

Closed

9 tasks

AdityaSripal changed the title ~~Modular Client~~ Modular IBC Client Aug 12, 2020

AdityaSripal added 2 commits August 12, 2020 18:51

readd TODO

e25c3db

add nil checks for misbehaviour

4021db7

fedekunze reviewed Aug 13, 2020

View reviewed changes

colin-axner reviewed Aug 13, 2020

View reviewed changes

AdityaSripal added 5 commits August 13, 2020 15:31

address reviews

b8180c1

address rest of reviews and fix builds

4d9ce8f

fixed tests

2f1c78c

address rest of reviews

41dd95d

Merge branch 'master' into aditya-modular-client

8120dae

AdityaSripal marked this pull request as ready for review August 13, 2020 23:00

AdityaSripal requested review from aaronc, alessio, alexanderbez, clevinson and jgimeno as code owners August 13, 2020 23:00

AdityaSripal added 3 commits August 13, 2020 19:07

fix expired blocks bug

5c2bf66

Merge branch 'aditya-modular-client' of github.com:cosmos/cosmos-sdk …

24451d3

…into aditya-modular-client

fix expired bug

168d6aa

AdityaSripal commented Aug 13, 2020

View reviewed changes

fedekunze approved these changes Aug 14, 2020

View reviewed changes

fedekunze merged commit 3735b18 into master Aug 14, 2020

fedekunze deleted the aditya-modular-client branch August 14, 2020 08:47

colin-axner reviewed Aug 18, 2020

View reviewed changes

AdityaSripal mentioned this pull request Aug 18, 2020

#7028 followup #7104

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modular IBC Client #7028

Modular IBC Client #7028

AdityaSripal commented Aug 12, 2020

codecov bot commented Aug 12, 2020 •

edited

Loading

fedekunze Aug 13, 2020

colin-axner Aug 13, 2020

AdityaSripal Aug 13, 2020

fedekunze left a comment •

edited

Loading

fedekunze Aug 13, 2020

colin-axner Aug 13, 2020

AdityaSripal Aug 13, 2020 •

edited

Loading

colin-axner Aug 18, 2020

colin-axner left a comment

colin-axner Aug 13, 2020

colin-axner Aug 13, 2020

colin-axner Aug 13, 2020

colin-axner Aug 13, 2020

fedekunze Aug 13, 2020

AdityaSripal Aug 13, 2020

colin-axner Aug 18, 2020

AdityaSripal Aug 18, 2020

colin-axner Aug 13, 2020

AdityaSripal Aug 13, 2020

AdityaSripal Aug 13, 2020

AdityaSripal Aug 13, 2020

AdityaSripal Aug 13, 2020

fedekunze left a comment

colin-axner left a comment

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

colin-axner Aug 18, 2020

		(ageDuration > consensusParams.Evidence.MaxAgeDuration \|\|
		ageBlocks > consensusParams.Evidence.MaxAgeNumBlocks) {

		@@ -69,34 +66,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
		err error
		)

Modular IBC Client #7028

Modular IBC Client #7028

Conversation

AdityaSripal commented Aug 12, 2020

Description

codecov bot commented Aug 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

colin-axner 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 12, 2020 •

edited

Loading

fedekunze left a comment •

edited

Loading

AdityaSripal Aug 13, 2020 •

edited

Loading