-
Notifications
You must be signed in to change notification settings - Fork 679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Integrate ICS Provider #1938
Conversation
Codecov Report
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
|
Hi, can you fix lint |
app/keepers/keepers.go
Outdated
|
||
// if _, _, err := streaming.LoadStreamingServices(bApp, appOpts, appCodec, appKeepers.keys); err != nil { | ||
// tmos.Exit(err.Error()) | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why comment out this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was temporarily removed due to a previous version conflict...bringing it back in
tests/e2e/go.mod
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we okay with using our branch of ibc-go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does our fork of ibc-go change anything besides the testing framework?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have a concrete update by Monday, discussed a possible solution for the release here.
tests/e2e/go.mod
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this contain the ICS changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used the same version as ICS https://github.com/cosmos/interchain-security/blob/main/go.mod#L6
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just some nits.
app/upgrades/v8/upgrades_test.go
Outdated
// send tokens from bank module to source address | ||
// test helper | ||
// helper.seedAccount(sourceAddress, amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment code.
|
||
func TestCCVTestSuite(t *testing.T) { | ||
ccvSuite := e2e.NewCCVTestSuite[*app.GaiaApp, *appConsumer.App]( | ||
// Pass in ibctesting.AppIniters for gaia (provider) and consumer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This PR fulfills the minimum requirement #1909 for integrating ICS into Gaia as a provider chain.
Outstanding Issues