02-client refactor: Adding VerifyClientMessage helper fn #1119

seantking · 2022-03-15T11:06:49Z

Description

This PR focuses on refactoring the CheckHeaderAndUpdateState tests into a separate VerifyHeader & VerifyMisbehavior test suite. The bulk of the work has involved rewiring the test cases to take advantage of the latest ibctesting library capabilities.

The test cases for verifying headers with different Revision numbers have been left in the TestCheckHeaderAndUpdateState fn as the refactoring is blocked by this PR.

Renaming checkValidity to VerifyClientMessage and exposing on ClientState
VerifyClientMessage now validates a Header or a Misbehaviour
Using VerifyClientMessage in both CheckMisbehaviourAndUpdateState & CheckHeaderAndUpdateState
Make private verifyHeader fn

partially closes: #879

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

…elper-fn

modules/light-clients/07-tendermint/types/update.go

colin-axner

Great work!

modules/light-clients/07-tendermint/types/update.go

modules/light-clients/07-tendermint/types/update_test.go

…r set

modules/light-clients/07-tendermint/types/update_test.go

modules/light-clients/07-tendermint/types/update.go

modules/light-clients/07-tendermint/types/update_test.go

…elper-fn

damiannolan

Awesome work, in general looks great, left a couple of small comments. Appreciate the amount of the work that went into the tests here 💯

damiannolan · 2022-03-28T14:20:30Z

modules/light-clients/07-tendermint/types/update.go

+// VerifyClientMessage checks if the clientMessage is of type Header or Misbehaviour and verifies the message
+func (cs *ClientState) VerifyClientMessage(
+	ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec,
+	header exported.ClientMessage,


Would suggest this to be named clientMsg

damiannolan · 2022-03-28T14:22:33Z

modules/light-clients/07-tendermint/types/update.go

+// - header timestamp is less than or equal to the consensus state timestamp
+func (cs *ClientState) verifyHeader(
+	ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec,
+	tmHeader exported.ClientMessage, currentTimestamp time.Time,


I think you could change exported.ClientMessage to *Header

damiannolan · 2022-03-28T14:22:51Z

modules/light-clients/07-tendermint/types/update.go

+	ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec,
+	header exported.ClientMessage,
+) error {
+	switch header.(type) {


And here do msg := clientMsg.(type)

damiannolan · 2022-03-28T14:24:12Z

modules/light-clients/07-tendermint/types/update_test.go

 	types "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 	ibctestingmock "github.com/cosmos/ibc-go/v3/testing/mock"
+	tmtypes "github.com/tendermint/tendermint/types"
 )

 func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() {


I guess we can remove this test function after we get these PRs in! :)

Just a note for a follow up

We need to get the Revision stuff settled first. I've left the test cases for that in this test fn for now.

damiannolan · 2022-03-28T14:25:19Z

modules/light-clients/07-tendermint/types/update_test.go

+	revisionHeight := int64(height.RevisionHeight)
+
+	// create modified heights to use for test-cases
+	altVal := tmtypes.NewValidator(altPubKey, revisionHeight)


Isn't the second argument supposed to be voting power?

yes. I'd remove revisionHeight and replace with 100

good catch. This was leftover from the previous test

…elper-fn

colin-axner

Excellent work! Just a few test cases that need fixing

modules/light-clients/07-tendermint/types/update.go

colin-axner · 2022-03-28T15:27:31Z

modules/light-clients/07-tendermint/types/update_test.go

+	revisionHeight := int64(height.RevisionHeight)
+
+	// create modified heights to use for test-cases
+	altVal := tmtypes.NewValidator(altPubKey, revisionHeight)


yes. I'd remove revisionHeight and replace with 100

modules/light-clients/07-tendermint/types/update_test.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

…elper-fn

colin-axner

Wahoo! :)

* refactor: move misbehaviour validation into verifyMisbehaviour function * begin writing misbehaviour tests * fix misbehaviour test * continue adding misbehaviour test cases * add more test cases to verifyMisbehaviour test * add changing validator set tests * finish rest of tests except revision height testing * Update modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add back misbehaviour type assertion Co-authored-by: Damian Nolan <damiannolan@gmail.com>

* refactor: Adding VerifyClientMessage helper fn to ClientState * refactor: creating verifyHeader priv fn and respective test * refactor: adding initial test cases * refactor: add more test cases * nit: move fns * remove clientState var * refactor: adding different val set test case * refactor: add test case for header with next height and diff validator set * refactor: adding remaining test cases * chore: uncomment previous tests: * fix: chainA -> chainB * chore: comment * refactor: remove consState from api + fix tests * refactor: add verifyHeader to clientState * fix: incorret trusted validators for concensus state test * Update modules/light-clients/07-tendermint/types/update_test.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * chore: add comment * fix: params * refactor: remove timestamp from api * refactor: switch and type * fix: remove height+1 * 02-client refactor: add tests for verifyMisbehaviour (cosmos#1166) * refactor: move misbehaviour validation into verifyMisbehaviour function * begin writing misbehaviour tests * fix misbehaviour test * continue adding misbehaviour test cases * add more test cases to verifyMisbehaviour test * add changing validator set tests * finish rest of tests except revision height testing * Update modules/light-clients/07-tendermint/types/misbehaviour_handle_test.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * add back misbehaviour type assertion Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

## Overview Closes: cosmos#1119  ## Checklist  - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords

refactor: Adding VerifyClientMessage helper fn to ClientState

73047c4

seantking requested review from colin-axner, fedekunze and AdityaSripal as code owners March 15, 2022 11:06

seantking marked this pull request as draft March 15, 2022 11:07

seantking requested review from charleenfei and crodriguezvega March 15, 2022 11:07

seantking mentioned this pull request Mar 15, 2022

02-client refactor: creating ValidateClientMessage fn for Header and Misbehaviour on 07-tendermint #1115

Closed

9 tasks

seantking and others added 2 commits March 21, 2022 14:31

Merge branch '02-client-refactor' into sean/issue#879-create-verify-h…

628401d

…elper-fn

refactor: creating verifyHeader priv fn and respective test

8dd6dab

colin-axner reviewed Mar 23, 2022

View reviewed changes

modules/light-clients/07-tendermint/types/update.go Outdated Show resolved Hide resolved

seantking force-pushed the sean/issue#879-create-verify-helper-fn branch from 5b76483 to 70c2973 Compare March 24, 2022 07:57

refactor: adding initial test cases

b04e491

seantking force-pushed the sean/issue#879-create-verify-helper-fn branch from 70c2973 to b04e491 Compare March 24, 2022 07:58

refactor: add more test cases

afb2ff5

seantking requested a review from colin-axner March 24, 2022 10:00

seantking added 2 commits March 24, 2022 16:05

nit: move fns

465b801

remove clientState var

c6f9909

colin-axner reviewed Mar 24, 2022

View reviewed changes

seantking added 5 commits March 24, 2022 18:42

refactor: adding different val set test case

c5c5eb0

refactor: add test case for header with next height and diff validato…

f27b7b2

…r set

refactor: adding remaining test cases

3676401

chore: uncomment previous tests:

64dda21

fix: chainA -> chainB

76b600d

seantking commented Mar 25, 2022

View reviewed changes

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved

chore: comment

76eec4f

seantking marked this pull request as ready for review March 25, 2022 15:40

colin-axner reviewed Mar 28, 2022

View reviewed changes

modules/light-clients/07-tendermint/types/update.go Outdated Show resolved Hide resolved

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved

seantking added 2 commits March 28, 2022 15:15

refactor: remove consState from api + fix tests

98310be

refactor: add verifyHeader to clientState

ec00c35

Merge branch '02-client-refactor' into sean/issue#879-create-verify-h…

98599f8

…elper-fn

damiannolan reviewed Mar 28, 2022

View reviewed changes

Merge branch '02-client-refactor' into sean/issue#879-create-verify-h…

7839463

…elper-fn

colin-axner reviewed Mar 28, 2022

View reviewed changes

seantking and others added 8 commits March 28, 2022 17:33

fix: incorret trusted validators for concensus state test

0148f35

Update modules/light-clients/07-tendermint/types/update_test.go

795a4fc

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

chore: add comment

7a1d046

fix: params

926de94

refactor: remove timestamp from api

36b5ebc

refactor: switch and type

cbbf854

Merge branch '02-client-refactor' into sean/issue#879-create-verify-h…

d42c9c0

…elper-fn

fix: remove height+1

fa4184f

colin-axner approved these changes Mar 28, 2022

View reviewed changes

seantking enabled auto-merge (squash) March 28, 2022 16:16

damiannolan approved these changes Mar 28, 2022

View reviewed changes

colin-axner mentioned this pull request Mar 28, 2022

Update godoc for VerifyClientMessage in 07-tendermint #1185

Closed

3 tasks

seantking merged commit fadd9d0 into 02-client-refactor Mar 28, 2022

seantking deleted the sean/issue#879-create-verify-helper-fn branch March 28, 2022 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

02-client refactor: Adding VerifyClientMessage helper fn #1119

02-client refactor: Adding VerifyClientMessage helper fn #1119

seantking commented Mar 15, 2022 •

edited

Loading

colin-axner left a comment

damiannolan left a comment

damiannolan Mar 28, 2022

damiannolan Mar 28, 2022

damiannolan Mar 28, 2022

damiannolan Mar 28, 2022

seantking Mar 28, 2022

damiannolan Mar 28, 2022

colin-axner Mar 28, 2022

seantking Mar 28, 2022

colin-axner left a comment

colin-axner Mar 28, 2022

colin-axner left a comment

02-client refactor: Adding VerifyClientMessage helper fn #1119

02-client refactor: Adding VerifyClientMessage helper fn #1119

Conversation

seantking commented Mar 15, 2022 • edited Loading

Description

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan 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

colin-axner left a comment

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

seantking commented Mar 15, 2022 •

edited

Loading