feat(connection): migrate connection params by DimitrisJim · Pull Request #3650 · cosmos/ibc-go

DimitrisJim · 2023-05-24T18:19:33Z

Description

closes: #3501

Commit Message / Changelog Entry

feat(connection): migrate connection params

see the guidelines for commit messages. (view raw markdown for examples)

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 and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

DimitrisJim · 2023-05-24T18:20:16Z

leaving this as a draft until #3640 is merged.

codecov-commenter · 2023-05-24T18:24:30Z

Codecov Report

❌ Patch coverage is 66.19718% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.74%. Comparing base (d751bc9) to head (6c0a635).
⚠️ Report is 1678 commits behind head on main.

Files with missing lines	Patch %	Lines
modules/core/03-connection/types/params_legacy.go	33.33%	8 Missing ⚠️
modules/core/module.go	0.00%	5 Missing and 1 partial ⚠️
modules/core/03-connection/types/msgs.go	66.66%	5 Missing ⚠️
modules/core/03-connection/keeper/migrations.go	66.66%	2 Missing and 1 partial ⚠️
modules/core/03-connection/keeper/keeper.go	89.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3650      +/-   ##
==========================================
- Coverage   78.80%   78.74%   -0.06%     
==========================================
  Files         187      187              
  Lines       12846    12888      +42     
==========================================
+ Hits        10123    10149      +26     
- Misses       2292     2308      +16     
  Partials      431      431

Files with missing lines	Coverage Δ
modules/core/03-connection/keeper/verify.go	`79.22% <100.00%> (ø)`
modules/core/03-connection/types/codec.go	`100.00% <100.00%> (ø)`
modules/core/03-connection/types/keys.go	`100.00% <ø> (ø)`
modules/core/03-connection/types/params.go	`100.00% <ø> (+21.73%)`	⬆️
modules/core/keeper/keeper.go	`91.66% <ø> (ø)`
modules/core/keeper/msg_server.go	`55.95% <100.00%> (+0.71%)`	⬆️
modules/core/03-connection/keeper/keeper.go	`90.37% <89.47%> (+0.93%)`	⬆️
modules/core/03-connection/keeper/migrations.go	`57.14% <66.66%> (+17.14%)`	⬆️
modules/core/03-connection/types/msgs.go	`84.07% <66.66%> (-1.41%)`	⬇️
modules/core/module.go	`44.30% <0.00%> (-3.00%)`	⬇️
... and 1 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

crodriguezvega

I think looks good, @DimitrisJim! I just left a couple of comments.

modules/core/03-connection/types/msgs.go

modules/core/03-connection/keeper/keeper_test.go

DimitrisJim · 2023-05-31T07:40:11Z

e2e/testsuite/grpc_query.go


 	s.grpcClients[chain.Config().ChainID] = GRPCClients{
 		ClientQueryClient:      clienttypes.NewQueryClient(grpcConn),
+		ConnectionQueryClient:  connectiontypes.NewQueryClient(grpcConn),


the wonders of allowing default initialization.

I just stumbled upon this error when running the v7.1 upgrade test. :)

Multiple victims of this unforgiving language design

damiannolan

Looks really good so far. Just left a couple of comments :)

modules/core/keeper/keeper.go

modules/core/03-connection/keeper/keeper_test.go

proto/ibc/core/connection/v1/tx.proto

e2e/tests/core/03-connection/connection_test.go

damiannolan

Looks good to me! Great work @DimitrisJim 🙏

modules/core/keeper/msg_server.go

crodriguezvega

Great work, @DimitrisJim!

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

modules/core/03-connection/genesis.go

colin-axner · 2023-06-05T10:22:54Z

modules/core/03-connection/types/msgs.go

+	if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
+	}
+	return msg.Params.Validate()


Params.Validate() returns a fmt.Errorf error, this will result in the default SDK internal error code of 1 since no registered error is returned. Previously this validation was only called in InitGenesis where you don't need to return sdk registered errors, we may want to update this now?

DimitrisJim linked an issue May 24, 2023 that may be closed by this pull request

Migrate 03-connection params to be self managed #3501

Closed

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch 9 times, most recently from 812e67f to b1c4e69 Compare May 30, 2023 17:48

DimitrisJim marked this pull request as ready for review May 30, 2023 17:49

DimitrisJim requested review from AdityaSripal, charleenfei, chatton, colin-axner, crodriguezvega, damiannolan and srdtrk as code owners May 30, 2023 17:49

crodriguezvega reviewed May 30, 2023

View reviewed changes

modules/core/03-connection/types/msgs.go Outdated Show resolved Hide resolved

modules/core/03-connection/keeper/keeper_test.go Show resolved Hide resolved

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch from b1c4e69 to 1cb9375 Compare May 31, 2023 07:03

DimitrisJim commented May 31, 2023

View reviewed changes

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch from 0364f34 to be0461b Compare May 31, 2023 08:32

crodriguezvega mentioned this pull request May 31, 2023

adapt connection e2e params test to use gov v1 message #3704

Closed

1 task

damiannolan reviewed May 31, 2023

View reviewed changes

DimitrisJim mentioned this pull request May 31, 2023

Rename client update param messages to match other modules. #3708

Merged

9 tasks

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch 3 times, most recently from acbc432 to 4ef0dc9 Compare June 1, 2023 08:52

DimitrisJim mentioned this pull request Jun 1, 2023

feat(client): migrate client params #3640

Merged

9 tasks

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch from 4ef0dc9 to adb29be Compare June 1, 2023 10:24

damiannolan approved these changes Jun 1, 2023

View reviewed changes

modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch from b3e9af6 to 52e6730 Compare June 1, 2023 16:27

crodriguezvega approved these changes Jun 2, 2023

View reviewed changes

DimitrisJim and others added 10 commits June 2, 2023 13:12

feat(connection): migrate connection params

a2b0aca

Rename messages to match other modules.

ebd282c

Update proto/ibc/core/connection/v1/tx.proto

0264478

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

Address review comments.

4544b00

Use define const for default delay.

c155e2a

Update modules/core/keeper/msg_server.go

ae4ae1e

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

Regenerate tx files.

5b5d425

fixed typo

1aaf340

rename test

10be7db

Rebase, regenerate.

1925102

DimitrisJim force-pushed the jim/3501-migrate-03-connection-params-to-be-self-managed branch from e1d60fe to 1925102 Compare June 2, 2023 10:16

crodriguezvega reviewed Jun 2, 2023

View reviewed changes

modules/core/03-connection/genesis.go Outdated Show resolved Hide resolved

remove redundant param validation in initgenesis.

6c0a635

DimitrisJim merged commit 96808eb into main Jun 2, 2023

DimitrisJim deleted the jim/3501-migrate-03-connection-params-to-be-self-managed branch June 2, 2023 11:58

colin-axner reviewed Jun 5, 2023

View reviewed changes

Conversation

DimitrisJim commented May 24, 2023

Description

Commit Message / Changelog Entry

Uh oh!

DimitrisJim commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 24, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

crodriguezvega left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DimitrisJim May 31, 2023

Choose a reason for hiding this comment

Uh oh!

crodriguezvega Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

DimitrisJim Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crodriguezvega left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colin-axner Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DimitrisJim commented May 24, 2023 •

edited

Loading

codecov-commenter commented May 24, 2023 •

edited by codecov bot

Loading