feat: Integrate ICS Provider #1938

glnro · 2022-12-05T09:06:29Z

This PR fulfills the minimum requirement #1909 for integrating ICS into Gaia as a provider chain.

Outstanding Issues

Replace Cosmos SDK when ICS changes are merged
Replace ICS with tagged version when IBC changes are merged (blocked by Figure out IBC version situation interchain-security#585)
Add Gaia Provider test to ICS

codecov · 2022-12-05T09:10:43Z

Codecov Report

Merging #1938 (90977e3) into main (c446c23) will increase coverage by 1.24%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1938      +/-   ##
==========================================
+ Coverage   79.20%   80.44%   +1.24%     
==========================================
  Files          23       23              
  Lines        1577     1616      +39     
==========================================
+ Hits         1249     1300      +51     
+ Misses        274      262      -12     
  Partials       54       54

Impacted Files	Coverage Δ
app/app.go	`75.67% <100.00%> (+3.32%)`	⬆️
app/keepers/keepers.go	`98.92% <100.00%> (+1.71%)`	⬆️
app/keepers/keys.go	`86.20% <100.00%> (ø)`
app/modules.go	`100.00% <100.00%> (ø)`
x/globalfee/module.go	`88.67% <0.00%> (+3.77%)`	⬆️

yaruwangway · 2022-12-05T15:04:24Z

Hi, can you fix lint

yaruwangway · 2022-12-06T11:10:41Z

app/keepers/keepers.go

+
+ // if _, _, err := streaming.LoadStreamingServices(bApp, appOpts, appCodec, appKeepers.keys); err != nil {
+ // tmos.Exit(err.Error())
+ // }


why comment out this ?

It was temporarily removed due to a previous version conflict...bringing it back in

mpoke · 2022-12-07T14:44:15Z

tests/e2e/go.mod

+
+replace (
+ github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.45.12-0.20221116140330-9c145c827001
+ github.com/cosmos/ibc-go/v3 => github.com/informalsystems/ibc-go/v3 v3.4.1-0.20221202165607-3dc5ba251371


Are we okay with using our branch of ibc-go?

That's a really good question. IIRC our changes are just testing related, so it's possible that we could use mainline 3.4 here without breaking anything in the actual code

Yeah just reverted

@jtremback @mpoke We actually need to keep the dependency for informalsystems/ibc-go as otherwise GaiaApp will be incompatible and unable to implement e2eutil.ProviderApp. This is because ICS's e2e ProviderApp must implement the following which is not available if we use IBC v3.4.1

import ibcclienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" func (appKeepers *AppKeepers) GetStakingKeeper() ibcclienttypes.StakingKeeper { return appKeepers.StakingKeeper }

Does our fork of ibc-go change anything besides the testing framework?

Will have a concrete update by Monday, discussed a possible solution for the release here.

mpoke · 2022-12-07T14:45:59Z

tests/e2e/go.mod

@@ -144,3 +144,11 @@ require (
 nhooyr.io/websocket v1.8.6 // indirect
 sigs.k8s.io/yaml v1.3.0 // indirect
 )
+
+replace (
+ github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.45.12-0.20221116140330-9c145c827001


Does this contain the ICS changes?

Used the same version as ICS https://github.com/cosmos/interchain-security/blob/main/go.mod#L6

glnro · 2023-01-04T17:01:57Z

@glnro the reason TestAppStateDeterminism is currently failing is due to the way the cosmos sdk simulation package sets up the test, details below:

TestAppStateDeterminism uses SimulateFromSeed. This function takes in an AppStateFn, which accepts the gaia app's SimulationManager.

Firstly, simulationModules() is missing an entry for the provider module. This means the SimulationManager mentioned above is initialized without the provider module. This causes downstream problems when the AppStateFn called in TestAppStateDeterminism, eventually calls AppStateRandomizedFn, which calls simManager.GenerateGenesisStates(simState) on the sim manager. I believe adding an entry to simulationModules() (within this repo) fixes the issue.

Next, the default genesis state created in AppStateRandomizedFn, uses the ModuleBasics global var. I believe we need to add an entry for the ICS provider's app module basic here. This change would lie in cosmos sdk.

When making the above two changes, the TestAppStateDeterminism did not pass, but it at least gave me an error that was different than before, and seemingly unrelated to ICS:
--- FAIL: TestAppStateDeterminism (1.11s)
panic: invalid height: 2; expected: 1 [recovered]
	panic: invalid height: 2; expected: 1 [recovered]
	panic: invalid height: 2; expected: 1
Hope this can at least help point us in right direction toward fixing the issue.

I'm not sure we should be using Simapp long term anyway, but making the change locally in the SDK I was able to get the tests to pass. @alexanderbez Not sure what the SDK would advise moving forward. We have plans to remove references to simapp in the future, but they've been deprioritized in favor of integrating ICS.

* Fix simapp dependence * Fix coverage and linting * Update sim ignore

mpoke

LGTM. Just some nits.

mpoke · 2023-01-16T17:51:05Z

app/upgrades/v8/upgrades_test.go

+ // send tokens from bank module to source address
+ // test helper
+ // helper.seedAccount(sourceAddress, amount)


Remove comment code.

mpoke · 2023-01-16T17:56:35Z

tests/ics/interchain_security_test.go

+
+func TestCCVTestSuite(t *testing.T) {
+ ccvSuite := e2e.NewCCVTestSuite[*app.GaiaApp, *appConsumer.App](
+ // Pass in ibctesting.AppIniters for gaia (provider) and consumer.


Improve comment - https://github.com/cosmos/interchain-security/blob/7ddcf8710d99654d1d5c2aba1fc061887a7c37f4/tests/e2e/instance_test.go#L68 (see cosmos/interchain-security#660).

sainoe

LGTM

sonarcloud · 2023-01-17T19:56:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

lg added 3 commits December 4, 2022 20:47

Update dependencies and compile

b0a2cc0

Add ICS

18ce46d

Add gov prop

7cb04f2

glnro requested review from okwme, alexanderbez, zmanian, yaruwangway, crodriguezvega, jackzampolin and mmulji-ic as code owners December 5, 2022 09:06

glnro mentioned this pull request Dec 5, 2022

v0.45 compatible Rho Meta Issue #1909

Closed

27 tasks

lg and others added 8 commits December 5, 2022 16:51

Fix lint

b206190

Merge branch 'main' into glnro/ics-v45

e4907c0

Fix commenting

5edbc63

Fix commenting

a539f3e

Add GetStakingKeeper for ICS e2e test

aec5b30

Gaia implements ibc TestingApp

3a9bf13

Update correct return type for GetStakingKeeper

4ee8c07

Fix ibc import

00643a4

glnro mentioned this pull request Dec 6, 2022

test: Add CCV Test Suite with Gaia provider cosmos/interchain-security#559

Merged

2 tasks

Fix linting

81f918d

yaruwangway reviewed Dec 6, 2022

View reviewed changes

Comment back LoadStreamingServices

7437c41

glnro requested a review from yaruwangway December 6, 2022 12:05

mmulji-ic and others added 2 commits December 6, 2022 14:15

Merge branch 'main' into glnro/ics-v45

4cc2502

Merge branch 'main' into glnro/ics-v45

c6a8425

mpoke reviewed Dec 7, 2022

View reviewed changes

Change IBC back to v3.4

2669bc2

glnro and others added 18 commits January 4, 2023 19:00

Merge branch 'main' into glnro/ics-v45

c6d5c5a

Bump ics

347d8fc

Fix sdk import

d9d916e

Resolve conflicts

b51d0a5

Update sdk to tagged release

b43a8a7

Merge branch 'main' into glnro/ics-v45

6e94762

Merge branch 'main' into glnro/ics-v45

8150927

Fix simapp dependence (#1983)

65737b9

* Fix simapp dependence * Fix coverage and linting * Update sim ignore

Update codecov rules

d09a941

Merge branch 'main' into glnro/ics-v45

67beb00

Merge branch 'main' into glnro/ics-v45

6b3e169

Update ics dependency

c3c3586

Update ICS dependency

a07d2bd

Update code cov

7f64668

Merge branch 'main' into glnro/ics-v45

ddfb8d7

Update ICS to tagged version

2897762

Update codecov

f3e7890

Add imported ICS test

8ac9165

mpoke requested review from sainoe and jtremback January 16, 2023 17:47

mpoke approved these changes Jan 16, 2023

View reviewed changes

mpoke removed the request for review from okwme January 16, 2023 18:13

sainoe approved these changes Jan 17, 2023

View reviewed changes

lg added 3 commits January 17, 2023 20:49

Merge branch 'main' into glnro/ics-v45

dd66079

Make requested test comment changes

963219d

Remove commented code

90977e3

glnro merged commit e665d82 into main Jan 17, 2023

glnro deleted the glnro/ics-v45 branch January 17, 2023 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Integrate ICS Provider #1938

feat: Integrate ICS Provider #1938

glnro commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading

yaruwangway commented Dec 5, 2022

yaruwangway Dec 6, 2022

glnro Dec 6, 2022

mpoke Dec 7, 2022

jtremback Dec 7, 2022

glnro Dec 7, 2022

glnro Dec 9, 2022

mpoke Dec 10, 2022

glnro Dec 16, 2022

mpoke Dec 7, 2022

glnro Dec 7, 2022

glnro commented Jan 4, 2023 •

edited

Loading

mpoke left a comment

mpoke Jan 16, 2023

mpoke Jan 16, 2023

sainoe left a comment

sonarcloud bot commented Jan 17, 2023

feat: Integrate ICS Provider #1938

feat: Integrate ICS Provider #1938

Conversation

glnro commented Dec 5, 2022 • edited Loading

codecov bot commented Dec 5, 2022 • edited Loading

Codecov Report

yaruwangway commented Dec 5, 2022

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

Choose a reason for hiding this comment

glnro commented Jan 4, 2023 • edited Loading

mpoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainoe left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 17, 2023

glnro commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading

glnro commented Jan 4, 2023 •

edited

Loading