implement 09-localhost per specification #5769

gregdhill · 2020-03-09T21:07:59Z

Closes: #5543

Description

Mostly a stripped down copy of 07-tendermint, implementing 09-localhost.

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)

jackzampolin · 2020-03-17T14:37:17Z

A great start @gregdhill! Can't wait to review when this is r4r!

cwgoes · 2020-04-06T09:41:14Z

@gregdhill Let me know when this is ready, glad to review it.

gregdhill · 2020-04-10T10:33:11Z

@jackzampolin @cwgoes I have rebased this work which should be ready to review.

x/ibc/09-localhost/localhost_test.go

codecov · 2020-04-10T10:37:11Z

Codecov Report

Merging #5769 into master will increase coverage by 0.12%.
The diff coverage is 37.95%.

@@            Coverage Diff             @@
##           master    #5769      +/-   ##
==========================================
+ Coverage   55.54%   55.67%   +0.12%     
==========================================
  Files         420      426       +6     
  Lines       25293    25962     +669     
==========================================
+ Hits        14049    14454     +405     
- Misses      10275    10515     +240     
- Partials      969      993      +24

fedekunze · 2020-04-10T17:12:16Z

@gregdhill ibc-alpha was merged to master 2 days ago. I'm sorry this has taken so long to review. Mind fixing the conflicts on last time? 🙏

fedekunze

Thanks @gregdhill for taking care of this. Some minor comments

CHANGELOG.md

fedekunze · 2020-04-14T14:29:33Z

x/ibc/09-localhost/client_state.go

+
+// GetLatestHeight returns zero.
+func (cs ClientState) GetLatestHeight() uint64 {
+	return 0


correct me if I'm wrong @cwgoes but I think this should return ctx.BlockHeight()

Yes, that's right.

Suggested change

return 0

return cs.ctx.BlockHeight()

x/ibc/09-localhost/client_state.go

x/ibc/09-localhost/evidence.go

x/ibc/09-localhost/misbehaviour.go

x/ibc/09-localhost/update.go

gregdhill · 2020-04-16T09:29:05Z

Thanks for reviewing @fedekunze! Please see my latest changes.

fedekunze

utACK. Minor suggestions. Thanks @gregdhill!

x/ibc/09-localhost/client_state.go

CHANGELOG.md

cwgoes

Mostly LGTM, one change req'd, one question

cwgoes · 2020-04-16T17:52:48Z

x/ibc/09-localhost/client_state.go

+
+// GetLatestHeight returns zero.
+func (cs ClientState) GetLatestHeight() uint64 {
+	return 0


Yes, that's right.

cwgoes · 2020-04-16T17:54:18Z

x/ibc/09-localhost/client_state.go

+
+// GetChainID returns an empty string
+func (cs ClientState) GetChainID() string {
+	return ""


Hmm, I don't know about this, maybe this should be ctx.ChainID()? Maybe non-essential though

@cwgoes I think we need to add a ctx sdk.Context to the ClientState field.

// ClientState requires (read-only) access to keys outside the client prefix. type ClientState struct { ctx sdk.Context store sdk.KVStore }

@gregdhill can you add it and add this following function too?:

// WithContext updates the client state context to provide the chain ID and latest height func (cs *ClientState) WithContext(ctx sdk.Context) { cs.ctx = ctx }

Suggested change

return ""

return cs.ctx.ChainID()

fedekunze

Final comments @gregdhill and we're good to go. Thanks again!

x/ibc/09-localhost/client_state.go

fedekunze · 2020-04-17T13:51:45Z

x/ibc/09-localhost/client_state.go

+
+// GetChainID returns an empty string
+func (cs ClientState) GetChainID() string {
+	return ""


@cwgoes I think we need to add a ctx sdk.Context to the ClientState field.

// ClientState requires (read-only) access to keys outside the client prefix. type ClientState struct { ctx sdk.Context store sdk.KVStore }

@gregdhill can you add it and add this following function too?:

// WithContext updates the client state context to provide the chain ID and latest height func (cs *ClientState) WithContext(ctx sdk.Context) { cs.ctx = ctx }

fedekunze · 2020-04-17T13:52:12Z

x/ibc/09-localhost/client_state.go

+
+// GetChainID returns an empty string
+func (cs ClientState) GetChainID() string {
+	return ""


Suggested change

return ""

return cs.ctx.ChainID()

fedekunze · 2020-04-17T13:52:38Z

x/ibc/09-localhost/client_state.go

+
+// GetLatestHeight returns zero.
+func (cs ClientState) GetLatestHeight() uint64 {
+	return 0


Suggested change

return 0

return cs.ctx.BlockHeight()

x/ibc/09-localhost/client_state.go

x/ibc/09-localhost/evidence.go

x/ibc/09-localhost/header.go

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>

addressed

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

x/ibc/09-localhost/consensus_state.go

x/ibc/02-client/handler.go

gregdhill requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners March 9, 2020 21:07

tac0turtle requested a review from cwgoes March 9, 2020 23:47

tac0turtle added the x/ibc label Mar 9, 2020

gregdhill force-pushed the 09-localhost branch from 43c4a23 to 7a0fa4f Compare April 10, 2020 10:32

golangcibot reviewed Apr 10, 2020

View reviewed changes

x/ibc/09-localhost/localhost_test.go Outdated Show resolved Hide resolved

fedekunze changed the base branch from ibc-alpha to master April 10, 2020 17:10

gregdhill force-pushed the 09-localhost branch 2 times, most recently from 712124e to f03e020 Compare April 12, 2020 17:01

fedekunze suggested changes Apr 14, 2020

View reviewed changes

gregdhill force-pushed the 09-localhost branch 2 times, most recently from 03b99fb to 5b43c26 Compare April 16, 2020 09:26

fedekunze approved these changes Apr 16, 2020

View reviewed changes

x/ibc/09-localhost/client_state.go Outdated Show resolved Hide resolved

x/ibc/09-localhost/client_state.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

fedekunze added the R4R label Apr 16, 2020

gregdhill force-pushed the 09-localhost branch from 5b43c26 to 04db363 Compare April 16, 2020 15:12

fedekunze requested a review from AdityaSripal April 16, 2020 15:18

cwgoes previously requested changes Apr 16, 2020

View reviewed changes

fedekunze reviewed Apr 17, 2020

View reviewed changes

implement 09-localhost per specification

e2e9cc6

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>

gregdhill force-pushed the 09-localhost branch from dbae791 to e2e9cc6 Compare April 17, 2020 14:07

fedekunze merged commit de00a7f into cosmos:master Apr 17, 2020

gregdhill deleted the 09-localhost branch April 17, 2020 15:22

AdityaSripal reviewed Apr 17, 2020

View reviewed changes

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

x/ibc/09-localhost/consensus_state.go Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement 09-localhost per specification #5769

implement 09-localhost per specification #5769

gregdhill commented Mar 9, 2020 •

edited

Loading

jackzampolin commented Mar 17, 2020

cwgoes commented Apr 6, 2020

gregdhill commented Apr 10, 2020

codecov bot commented Apr 10, 2020 •

edited

Loading

fedekunze commented Apr 10, 2020

fedekunze left a comment

fedekunze Apr 14, 2020

cwgoes Apr 16, 2020

fedekunze Apr 17, 2020

gregdhill commented Apr 16, 2020

fedekunze left a comment

cwgoes left a comment

cwgoes Apr 16, 2020

cwgoes Apr 16, 2020

fedekunze Apr 17, 2020

fedekunze Apr 17, 2020

fedekunze left a comment

fedekunze Apr 17, 2020

fedekunze Apr 17, 2020

fedekunze Apr 17, 2020

implement 09-localhost per specification #5769

implement 09-localhost per specification #5769

Conversation

gregdhill commented Mar 9, 2020 • edited Loading

Description

jackzampolin commented Mar 17, 2020

cwgoes commented Apr 6, 2020

gregdhill commented Apr 10, 2020

codecov bot commented Apr 10, 2020 • edited Loading

Codecov Report

fedekunze commented Apr 10, 2020

fedekunze 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

gregdhill commented Apr 16, 2020

fedekunze left a comment

Choose a reason for hiding this comment

cwgoes 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

fedekunze 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

gregdhill commented Mar 9, 2020 •

edited

Loading

codecov bot commented Apr 10, 2020 •

edited

Loading