ICS07 follow ups #5631

AdityaSripal · 2020-02-10T20:21:28Z

Addresses comments from #5485 (review)
Implements changes from cosmos/ibc#367

Description

Fixes to ICS7 to allow for safe updating/submitting misbehavior while ensuring the ICS2 package remains independent of tendermint specific client logic

For contributor use:

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

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

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

x/ibc/02-client/alias.go

…on to 07-tendermint

x/ibc/04-channel/keeper/keeper_test.go

x/ibc/03-connection/keeper/keeper_test.go

cwgoes

Thanks! Mostly looks good - a few remaining questions on abstraction boundaries, see comments.

x/ibc/02-client/handler.go

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

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

…usState

AdityaSripal · 2020-02-14T18:21:21Z

Blocked on figuring out how to do cosmos/ibc#378 while maintaing current IBC abstractions. Plan to find a solution for this for IBC week

Will address this in separate PR as its not required for demo

fedekunze

Looks good overall. I'm not sure what is the reasoning to move the messages to each specific client instead of ICS02.

Thanks @AdityaSripal

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

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

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

x/ibc/02-client/handler.go

fedekunze · 2020-02-17T09:56:14Z

x/ibc/02-client/handler.go

+func HandleMsgCreateClient(ctx sdk.Context, k Keeper, msg exported.MsgCreateClient) (*sdk.Result, error) {
+	clientType := exported.ClientTypeFromString(msg.GetClientType())
+	switch clientType {
+	case 0:


no need for this case

need it in case the string in msg does not match any of the expected values

it will fall under the default case

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

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

x/ibc/04-channel/keeper/keeper_test.go

x/ibc/07-tendermint/misbehaviour_test.go

x/ibc/07-tendermint/misbehaviour.go

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

cwgoes · 2020-02-17T14:37:56Z

End to end test cases of the imported tendermint.Verify
See if we can initially import the sequential light client only

…osmos-sdk into aditya/ics07-follow-ups

…itya/ics07-follow-ups

…s/cosmos-sdk into aditya/ics07-follow-ups

cwgoes

utACK

fedekunze

Changes LGTM. Additions from #5586 are still pending

cwgoes · 2020-02-17T23:19:51Z

Changes LGTM. Additions from #5586 are still pending

@AdityaSripal Did you want to add those here or in a separate PR?

cwgoes · 2020-02-17T23:41:31Z

Also, we should make sure that this PR fixes cosmos/relayer#27.

fedekunze

ACK pending minor comments

fedekunze · 2020-02-18T11:38:57Z

x/ibc/02-client/handler.go

+		if !ok {
+			return nil, sdkerrors.Wrap(ErrInvalidClientType, "Msg is not a Tendermint CreateClient msg")
+		}
+		clientState, err := ibctmtypes.InitializeFromMsg(tmMsg)


move out of the switch statement

also, you can just use Initialize here

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

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

x/ibc/20-transfer/handler_test.go

x/ibc/20-transfer/keeper/relay_test.go

x/ibc/ante/ante_test.go

codecov · 2020-02-18T12:53:51Z

Codecov Report

Merging #5631 into fedekunze/ics07-follow-ups will increase coverage by <.01%.
The diff coverage is 63.63%.

@@                      Coverage Diff                       @@
##           fedekunze/ics07-follow-ups    #5631      +/-   ##
==============================================================
+ Coverage                       42.54%   42.55%   +<.01%     
==============================================================
  Files                             375      375              
  Lines                           29855    29861       +6     
==============================================================
+ Hits                            12702    12706       +4     
- Misses                          16040    16042       +2     
  Partials                         1113     1113

Impacted Files	Coverage Δ
x/ibc/07-tendermint/update.go	`52.27% <100%> (ø)`	⬆️
x/ibc/07-tendermint/types/msgs.go	`52.72% <100%> (+2.72%)`	⬆️
x/ibc/02-client/keeper/client.go	`95.55% <100%> (ø)`	⬆️
x/ibc/07-tendermint/types/client_state.go	`68.33% <20%> (-0.9%)`	⬇️

* ADR07 follow up changes * add assertion checks * update ICS02 tests * update ICS07 tests * add trusting and ubd period to msg * tests * more test updates * ICS07 follow ups (#5631) * refactor tendermint package to move msgs here * fix rest of package to accomadate 07 refactor * added GetHeight to ConsensusState and moved clientstate struct creation to 07-tendermint * start work on making misbehavior retrieve consensusState LTE misbehavior * allow misbehavior submission at height not equal to persisted consensusState * optimize submitMisbehavior by erroring earlier * cleanup misbehavior and propose lazy fix * fix bug * Update x/ibc/02-client/keeper/client.go Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * address fede review * add chain-id into clientstate * address necessary fede review Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Aditya <adityasripal@gmail.com>

AdityaSripal added 2 commits February 8, 2020 18:19

refactor tendermint package to move msgs here

8969e80

fix rest of package to accomadate 07 refactor

80b1432

AdityaSripal requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners February 10, 2020 20:21

AdityaSripal changed the title ~~Aditya/ics07 follow ups~~ ICS07 follow ups Feb 10, 2020

AdityaSripal added x/ibc labels Feb 10, 2020

golangcibot reviewed Feb 10, 2020

View reviewed changes

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

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

jackzampolin reviewed Feb 10, 2020

View reviewed changes

x/ibc/02-client/alias.go Show resolved Hide resolved

added GetHeight to ConsensusState and moved clientstate struct creati…

16ff5fc

…on to 07-tendermint

golangcibot reviewed Feb 12, 2020

View reviewed changes

x/ibc/04-channel/keeper/keeper_test.go Outdated Show resolved Hide resolved

x/ibc/03-connection/keeper/keeper_test.go Outdated Show resolved Hide resolved

x/ibc/03-connection/keeper/keeper_test.go Outdated Show resolved Hide resolved

start work on making misbehavior retrieve consensusState LTE misbehavior

56398af

AdityaSripal requested a review from cwgoes February 13, 2020 02:11

cwgoes suggested changes Feb 13, 2020

View reviewed changes

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

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

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

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

cwgoes mentioned this pull request Feb 13, 2020

The one true master IBC implementation tracking issue cosmos/gaia#201

Closed

34 tasks

AdityaSripal added 2 commits February 13, 2020 10:13

allow misbehavior submission at height not equal to persisted consens…

87da72a

…usState

optimize submitMisbehavior by erroring earlier

a67eabf

AdityaSripal added 2 commits February 17, 2020 00:45

cleanup misbehavior and propose lazy fix

bbaa538

fix bug

d3ac2b8

fedekunze suggested changes Feb 17, 2020

View reviewed changes

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

f9fbfeb

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

cwgoes mentioned this pull request Feb 17, 2020

Tendermint light client verification #5586

Closed

13 tasks

AdityaSripal added 3 commits February 17, 2020 08:31

address fede review

ab0bb5a

Merge branch 'aditya/ics07-follow-ups' of https://github.com/cosmos/c…

da267b8

…osmos-sdk into aditya/ics07-follow-ups

fix conflicts

4a0e309

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into ad…

f08ffae

…itya/ics07-follow-ups

AdityaSripal added R4R and removed WIP labels Feb 17, 2020

AdityaSripal requested review from fedekunze and cwgoes February 17, 2020 17:31

Merge branch 'fedekunze/ics07-follow-ups' of https://github.com/cosmo…

e687eb8

…s/cosmos-sdk into aditya/ics07-follow-ups

cwgoes approved these changes Feb 17, 2020

View reviewed changes

fedekunze suggested changes Feb 17, 2020

View reviewed changes

cwgoes mentioned this pull request Feb 17, 2020

Issue with update client cosmos/relayer-archive#27

Closed

add chain-id into clientstate

b23bc14

fedekunze approved these changes Feb 18, 2020

View reviewed changes

address necessary fede review

de8e815

fedekunze merged commit 535a493 into fedekunze/ics07-follow-ups Feb 18, 2020

fedekunze deleted the aditya/ics07-follow-ups branch February 18, 2020 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS07 follow ups #5631

ICS07 follow ups #5631

AdityaSripal commented Feb 10, 2020

cwgoes left a comment

AdityaSripal commented Feb 14, 2020 •

edited

Loading

fedekunze left a comment

fedekunze Feb 17, 2020

AdityaSripal Feb 17, 2020

fedekunze Feb 17, 2020

cwgoes commented Feb 17, 2020

cwgoes left a comment

fedekunze left a comment

cwgoes commented Feb 17, 2020

cwgoes commented Feb 17, 2020

fedekunze left a comment

fedekunze Feb 18, 2020

fedekunze Feb 18, 2020

codecov bot commented Feb 18, 2020

ICS07 follow ups #5631

ICS07 follow ups #5631

Conversation

AdityaSripal commented Feb 10, 2020

Description

cwgoes left a comment

Choose a reason for hiding this comment

AdityaSripal commented Feb 14, 2020 • edited Loading

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Feb 17, 2020

Choose a reason for hiding this comment

AdityaSripal Feb 17, 2020

Choose a reason for hiding this comment

fedekunze Feb 17, 2020

Choose a reason for hiding this comment

cwgoes commented Feb 17, 2020

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

cwgoes commented Feb 17, 2020

cwgoes commented Feb 17, 2020

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Feb 18, 2020

Choose a reason for hiding this comment

fedekunze Feb 18, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2020

Codecov Report

AdityaSripal commented Feb 14, 2020 •

edited

Loading