Check that client state is zeroed out for ibc client upgrade proposals #1529

chatton · 2022-06-13T12:24:20Z

Description

closes: #291

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

chatton · 2022-06-13T12:28:11Z

modules/core/02-client/types/proposal_test.go

 				suite.Require().NoError(err)
 			}, true,
 		},
 		{
 			"fails validate abstract - empty title", func() {
-				proposal, err = types.NewUpgradeProposal("", ibctesting.Description, plan, cs)
+				proposal, err = types.NewUpgradeProposal("", ibctesting.Description, plan, cs.ZeroCustomFields())


previous tests did not have zeroed out state.

charleenfei

same comment as previous PR otherwise but otherwise nice one

charleenfei · 2022-06-13T14:39:23Z

CHANGELOG.md

@@ -81,6 +81,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output
 * (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries.
 * (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure.
+* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) Client state must be zeroed out for `UpgradeProposals` to pass validation.


same comment about reasoning as in the other PR

I think it would be clearer if we refer to ClientState as a type name without space

damiannolan

I think we can target this for the 02-client-refactor branch based off this comment

Thanks @chatton!

damiannolan · 2022-06-14T09:14:54Z

modules/core/02-client/types/proposal.go

-
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
+	"reflect"


nit: group import statements like so:

import ( // standard library imports "fmt" "testing" // external library imports "github.com/stretchr/testify/require" abci "github.com/tendermint/tendermint/abci/types" // ibc-go library imports "github.com/cosmos/ibc-go/modules/core/23-commitment/types" )

damiannolan · 2022-06-14T09:16:41Z

CHANGELOG.md

@@ -81,6 +81,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output
 * (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries.
 * (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure.
+* (modules/core/02-client)[\#1529](https://github.com/cosmos/ibc-go/pull/1529) Client state must be zeroed out for `UpgradeProposals` to pass validation.


I think it would be clearer if we refer to ClientState as a type name without space

crodriguezvega

I agree with @charleenfei's and @damiannolan's comments. Other than that, nice work, @chatton!

codecov-commenter · 2022-06-20T08:57:35Z

Codecov Report

Merging #1529 (5b3028d) into main (22a51ec) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1529   +/-   ##
=======================================
  Coverage   80.29%   80.30%           
=======================================
  Files         166      166           
  Lines       12304    12307    +3     
=======================================
+ Hits         9880     9883    +3     
  Misses       1959     1959           
  Partials      465      465

Impacted Files	Coverage Δ
modules/core/02-client/types/proposal.go	`87.01% <100.00%> (+0.52%)`	⬆️

…roed-out-for-ibc-client-upgrade-proposals

modules/core/02-client/types/proposal.go

colin-axner

Great work! :)

Agree with above comments ^

…roed-out-for-ibc-client-upgrade-proposals

…r-ibc-client-upgrade-proposals' of https://github.com/cosmos/ibc-go into cian/issue#291-check-that-client-state-is-zeroed-out-for-ibc-client-upgrade-proposals

modules/core/02-client/types/proposal.go

@@ -2,6 +2,7 @@

 import (
 	"fmt"
+	"reflect"


chatton · 2022-07-08T09:02:38Z

Closing in favour of #1676

chatton added 3 commits June 13, 2022 12:32

wip: Added check for zeroed out custom fields in ValidateBasic

d51c228

test: Added test for validate basic with non-zeroed fields.

2e614a9

chore: Adding CHANGELOG.md entry.

fd0f457

chatton commented Jun 13, 2022

View reviewed changes

chatton marked this pull request as ready for review June 13, 2022 12:28

chatton requested review from colin-axner, fedekunze, AdityaSripal, crodriguezvega, seantking, charleenfei and damiannolan as code owners June 13, 2022 12:28

charleenfei approved these changes Jun 13, 2022

View reviewed changes

damiannolan reviewed Jun 14, 2022

View reviewed changes

crodriguezvega approved these changes Jun 20, 2022

View reviewed changes

chatton added 2 commits June 20, 2022 09:48

merge main

0ba2ab9

chore: addressed PR feedback, updated CHANGELOG.md

6d8f3d5

Merge branch 'main' into cian/issue#291-check-that-client-state-is-ze…

5b3028d

…roed-out-for-ibc-client-upgrade-proposals

crodriguezvega added the backport-to-v4.0.x label Jun 30, 2022

Merge branch 'main' into cian/issue#291-check-that-client-state-is-ze…

64893c3

…roed-out-for-ibc-client-upgrade-proposals

github-advanced-security bot found potential problems Jul 5, 2022

View reviewed changes

modules/core/02-client/types/proposal.go Fixed Show fixed Hide fixed

colin-axner approved these changes Jul 7, 2022

View reviewed changes

Merge branch 'main' into cian/issue#291-check-that-client-state-is-ze…

595731a

…roed-out-for-ibc-client-upgrade-proposals

chatton changed the base branch from main to 02-client-refactor July 7, 2022 13:33

chatton changed the base branch from 02-client-refactor to main July 7, 2022 13:33

chatton added 2 commits July 7, 2022 14:39

chore: addressing PR feedback

b1a3203

Merge branch 'cian/issue#291-check-that-client-state-is-zeroed-out-fo…

98a179b

…r-ibc-client-upgrade-proposals' of https://github.com/cosmos/ibc-go into cian/issue#291-check-that-client-state-is-zeroed-out-for-ibc-client-upgrade-proposals

github-advanced-security bot found potential problems Jul 7, 2022

View reviewed changes

modules/core/02-client/types/proposal.go

@@ -2,6 +2,7 @@

import (

"fmt"

"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

chatton closed this Jul 8, 2022

chatton deleted the cian/issue#291-check-that-client-state-is-zeroed-out-for-ibc-client-upgrade-proposals branch December 19, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check that client state is zeroed out for ibc client upgrade proposals #1529

Check that client state is zeroed out for ibc client upgrade proposals #1529

chatton commented Jun 13, 2022 •

edited

Loading

chatton Jun 13, 2022 •

edited

Loading

charleenfei left a comment

charleenfei Jun 13, 2022

damiannolan Jun 14, 2022

damiannolan left a comment

damiannolan Jun 14, 2022

damiannolan Jun 14, 2022

crodriguezvega left a comment

codecov-commenter commented Jun 20, 2022 •

edited

Loading

colin-axner left a comment

chatton commented Jul 8, 2022

Check that client state is zeroed out for ibc client upgrade proposals #1529

Check that client state is zeroed out for ibc client upgrade proposals #1529

Conversation

chatton commented Jun 13, 2022 • edited Loading

Description

chatton Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

charleenfei Jun 13, 2022

Choose a reason for hiding this comment

damiannolan Jun 14, 2022

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Jun 14, 2022

Choose a reason for hiding this comment

damiannolan Jun 14, 2022

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 20, 2022 • edited Loading

Codecov Report

colin-axner left a comment

Choose a reason for hiding this comment

chatton commented Jul 8, 2022

chatton commented Jun 13, 2022 •

edited

Loading

chatton Jun 13, 2022 •

edited

Loading

codecov-commenter commented Jun 20, 2022 •

edited

Loading