chore: set up IBCTestStakingKeeper interface by charleenfei · Pull Request #2028 · cosmos/ibc-go

charleenfei · 2022-08-17T12:54:05Z

Description

closes: #1971

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

colin-axner

LGTM, you'll need to update the GetStakingKeeper function in simapp/app.go as well. The readme needs to be updated too. Left a few comments

testing/types/expected_keepers.go

testing/chain_test.go

testing/types/expected_keepers.go

crodriguezvega · 2022-08-19T14:20:16Z

testing/README.md

 	// ibc-go additions
 	GetBaseApp() *baseapp.BaseApp
-	GetStakingKeeper() stakingkeeper.Keeper
+	GetIBCTestStakingKeeper() ibctestingtypes.StakingKeeper


I honestly liked the name GetStakingKeeper, since the name of the type already indicates that this is for testing, so I consider that is redundant to add Test in the name of the function... But no problem if we decide to rename it.

i renamed on request from @ValarDragon in the issue, i don't have a particularly strong opinion either way tho. I can see the value add in being explicit, but it is true that the type is also quite informative.

I think now that the interface returns the ibctestingtypes.StakingKeeper it can be redundant. I do agree GetStakingKeeper is cleaner, but I understand this function will be implemented in app.go for a production chain.

@ValarDragon do you still prefer GetIBCTestStakingKeeper given the new return value

crodriguezvega

Thanks for picking this up, @charleenfei!

crodriguezvega · 2022-08-19T14:21:06Z

testing/chain_test.go

 	require.True(t, ok)

-	val := chainA.App.GetStakingKeeper().GetValidators(chainA.GetContext(), 4)
+	val := chainA.GetSimApp().StakingKeeper.GetValidators(chainA.GetContext(), 4)


Is it not possible to keep using App.GetIBCTestStakingKeeper() and add GetValidators and Delegate to the StakingKeeper interface?

i think the idea is that in our simApp, we would continue using the staking module's staking keeper so we don't actually need to add these two methods as they are already there.

it's just that in the testing chain and testing app setup, we would like to enable osmosis to swap out their customised interfluid staking keeper so they can use our testing setup. this is why we only need to add that one method which is used in the testing/chain.go file to the new interface.

I actually requested those functions to not be added. I think we should keep our interfaces as small as possible (ideally we wouldn't even need the staking keeper interface)

colin-axner

LGTM. Could you add a changelog entry?

I'm also okay leaving the GetStakingKeeper name as is and doing in a followup pr if it is still desired. I don't have strong preferences though

testing/simapp/app.go

colin-axner

followup ACK

testing/types/expected_keepers.go

damiannolan

Nice work!

codecov-commenter · 2022-08-22T21:15:54Z

Codecov Report

Merging #2028 (eea436e) into main (358ab8d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2028   +/-   ##
=======================================
  Coverage   80.03%   80.03%           
=======================================
  Files         167      167           
  Lines       11784    11784           
=======================================
  Hits         9431     9431           
  Misses       1936     1936           
  Partials      417      417

Impacted Files	Coverage Δ
modules/apps/transfer/keeper/keeper.go	`91.17% <ø> (ø)`
modules/core/05-port/keeper/keeper.go	`72.41% <100.00%> (ø)`

* initial interface definition * update simapp * update godocs (cherry picked from commit 4d4dbcf)

colin-axner · 2022-08-23T10:27:02Z

We should probably add a note about the API change in the migration docs. Maybe link the simapp change we have as well?

* initial interface definition * update simapp * update godocs (cherry picked from commit 4d4dbcf) Co-authored-by: Charly <[email protected]> Co-authored-by: Damian Nolan <[email protected]>

* initial interface definition * update simapp * update godocs (cherry picked from commit 4d4dbcf) # Conflicts: # CHANGELOG.md # testing/app.go # testing/simapp/app.go

* chore: set up IBCTestStakingKeeper interface (#2028) * initial interface definition * update simapp * update godocs (cherry picked from commit 4d4dbcf) # Conflicts: # CHANGELOG.md # testing/app.go # testing/simapp/app.go * resolving conflicts Co-authored-by: Charly <[email protected]> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>

initial interface definition

c278d65

colin-axner reviewed Aug 17, 2022

View reviewed changes

testing/types/expected_keepers.go Show resolved Hide resolved

testing/chain_test.go Outdated Show resolved Hide resolved

testing/types/expected_keepers.go Outdated Show resolved Hide resolved

charleenfei added 3 commits August 18, 2022 15:18

wip

4e374f5

update simapp

62916e1

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

5c5aa52

charleenfei marked this pull request as ready for review August 18, 2022 14:40

charleenfei requested review from AdityaSripal, chatton, crodriguezvega, damiannolan and seantking as code owners August 18, 2022 14:40

crodriguezvega reviewed Aug 19, 2022

View reviewed changes

charleenfei added 2 commits August 19, 2022 20:32

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

3673054

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

46f0480

colin-axner approved these changes Aug 22, 2022

View reviewed changes

testing/simapp/app.go Show resolved Hide resolved

crodriguezvega added the backport-to-v5.0.x label Aug 22, 2022

charleenfei added 3 commits August 22, 2022 13:07

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

bd27422

updates re: comments

d1d88da

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

92f44ea

colin-axner approved these changes Aug 22, 2022

View reviewed changes

testing/types/expected_keepers.go Outdated Show resolved Hide resolved

colin-axner requested a review from crodriguezvega August 22, 2022 14:57

charleenfei added 2 commits August 22, 2022 17:17

update godocs

b27f8e5

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

ae50357

damiannolan approved these changes Aug 22, 2022

View reviewed changes

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

eea436e

Merge branch 'main' into charly/update_testing_stakingkeeper_interface

782e84d

seantking approved these changes Aug 23, 2022

View reviewed changes

charleenfei merged commit 4d4dbcf into main Aug 23, 2022

charleenfei deleted the charly/update_testing_stakingkeeper_interface branch August 23, 2022 07:54

mergify bot pushed a commit that referenced this pull request Aug 23, 2022

chore: set up IBCTestStakingKeeper interface (#2028)

041c799

* initial interface definition * update simapp * update godocs (cherry picked from commit 4d4dbcf)

mergify bot mentioned this pull request Aug 23, 2022

chore: set up IBCTestStakingKeeper interface (backport #2028) #2080

Merged

colin-axner mentioned this pull request Aug 23, 2022

IBC testing package GetStakingKeeper change needs migation docs entry #2083

Closed

3 tasks

crodriguezvega added the backport-to-v5.1.x label Aug 23, 2022

chatton mentioned this pull request Aug 24, 2022

Consider using git-cliff to generate CHANGELOG.md #2106

Closed

3 tasks

damiannolan added the backport-to-v6.0.x label Oct 6, 2022

mergify bot mentioned this pull request Oct 6, 2022

chore: set up IBCTestStakingKeeper interface (backport #2028) #2487

Merged

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

Conversation

charleenfei commented Aug 17, 2022

Description

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crodriguezvega Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

charleenfei Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

colin-axner Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

crodriguezvega left a comment

Choose a reason for hiding this comment

Uh oh!

crodriguezvega Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

charleenfei Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colin-axner Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 22, 2022

Codecov Report

Uh oh!

colin-axner commented Aug 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

charleenfei Aug 19, 2022 •

edited

Loading