-
Notifications
You must be signed in to change notification settings - Fork 625
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: add runtime Env to icacontroller and fix v0.52 e2e tests #7587
Conversation
Maybe we need to update linter versions or something in CI. Not sure what the arm64 build failure is too. Probably need to look into it |
Module safe query e2e test via interchain accounts is failing with:
This looks like something is malformed when executing the interchain accounts tx via MsgRecvPacket edit: looks like its coming from here -> https://github.com/cosmos/cosmos-sdk/blob/main/x/bank/keeper/grpc_query.go#L40-L43 Fixed in f9b7314. Looks like some address change crept in here. There was a TODO inline which needed to be fixed. The address must be bech32. |
Remaining test failures:
Solved:
Solved: Query context was missing time via header info. cosmos/cosmos-sdk#22732 ✅
Other than this its just linters that need to be fixed.. which looks like another can of worms |
Update on 1. above. The
I think the failure is possibly related to Solved: |
…de, update code to expect tendermint client temporarily
// TODO(damian): revert this extra err info | ||
return nil, errorsmod.Wrapf(ibcerrors.ErrUnpackAny, "cannot unpack Any into ClientState %T, typeURL: %s, cachedValue: %v", protoAny, protoAny.TypeUrl, protoAny.GetCachedValue()) |
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.
Revert 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.
Does it need an issue to make sure we don't forget, or will you remember? :)
@@ -642,3 +643,59 @@ func (suite *KeeperTestSuite) TestIBCSoftwareUpgrade() { | |||
}) | |||
} | |||
} | |||
|
|||
func (suite *KeeperTestSuite) TestIBCScheduledUpgradeProposal() { |
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.
This can also be deleted I think once cosmos/cosmos-sdk#22779 is solved, but its also no harm keeping around as it routes a msg for scheduleIBCUpgrade through gov
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.
Maybe a comment with a link to that for context?
if err := k.EventService.EventManager(ctx).EmitKV(icatypes.EventTypePacket, attributes...); err != nil { | ||
panic(err) | ||
} |
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.
This error should be propagated and returned from this func.
The likelihood of these errors surfacing from EventManager
are pretty much zero, but at some point in a future far away it may be possible for it to error - given a different impl of event service.
None the less, its good practise to actually return the errors and not panic and rely on grpc recovery interceptor
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.
Should it have a todo comment for that, then?
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! Left a couple of questions and minor comments, but looks great to me otherwise 🔥
@@ -74,22 +74,13 @@ func (s *UpgradeTestSuite) UpgradeChain(ctx context.Context, chain *cosmos.Cosmo | |||
Info: fmt.Sprintf("upgrade version test from %s to %s", currentVersion, upgradeVersion), | |||
} | |||
|
|||
if testvalues.GovV1MessagesFeatureReleases.IsSupported(chain.Config().Images[0].Version) { |
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.
Is this just not needed anymore, or will this break compatibility tests in any way we need to deal with later?
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.
Pretty sure it should be fine. As 0.47 (ibc-go/v7) has govv1 msg execution.
@@ -558,6 +558,18 @@ func (s *E2ETestSuite) createWalletOnChainIndex(ctx context.Context, amount, cha | |||
wallet := interchaintest.GetAndFundTestUsers(s.T(), ctx, strings.ReplaceAll(s.T().Name(), " ", "-"), sdkmath.NewInt(amount), chain)[0] | |||
// note the GetAndFundTestUsers requires the caller to wait for some blocks before the funds are accessible. | |||
s.Require().NoError(test.WaitForBlocks(ctx, 2, chain)) | |||
|
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.
💀 ooof
Is this the only way to do this now?
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.
SendCoins to an account no longer creates the account in x/auth. It just stores the coins to the address space in x/bank.
The account only gets created when there has been an action/tx made with the key.
You could probably exec any kind of tx here but this is what @chatton done and it works.
Instances where this came up in unit tests (where SendCoins to account was being used to create an account) I just swapped to directly using the x/auth keeper but that's only possible in unit testing where you have that kind of control
if err := k.EventService.EventManager(ctx).EmitKV(icatypes.EventTypePacket, attributes...); err != nil { | ||
panic(err) | ||
} |
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.
Should it have a todo comment for that, then?
@@ -29,11 +31,10 @@ func (suite *KeeperTestSuite) TestMigratorMigrateParams() { | |||
func() { | |||
suite.chainA.GetSimApp().ICAControllerKeeper = icacontrollerkeeper.NewKeeper( | |||
suite.chainA.Codec, | |||
runtime.NewKVStoreService(suite.chainA.GetSimApp().GetKey(icacontrollertypes.StoreKey)), | |||
runtime.NewEnvironment(runtime.NewKVStoreService(suite.chainA.GetSimApp().GetKey(icacontrollertypes.StoreKey)), log.NewNopLogger()), |
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.
I guess we'll have to do some upgrade doc changes for this. Are we tracking that anywhere?
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.
Typically in an epic we'd have for an sdk upgrade we'd have a bullet point for keeping track of this.
@@ -642,3 +643,59 @@ func (suite *KeeperTestSuite) TestIBCSoftwareUpgrade() { | |||
}) | |||
} | |||
} | |||
|
|||
func (suite *KeeperTestSuite) TestIBCScheduledUpgradeProposal() { |
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.
Maybe a comment with a link to that for context?
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.
Quickly skimmed, looking good. Trusting the tests. Thanks for shifting through the upgrade swamp 🙏
Quality Gate passed for 'ibc-go'Issues Measures |
* go mod changes * remove cometabci direct dep * fix all errors * appmodule & simapp cleanup * 08wasm fixes * get 08-wasm compiling * another simapp * another simapp * pass in environment * fix compilation errors * some fixes * build * fix other simapps * linting * remove unsed code * go mod changes * add pool module * anothersimapp * linting fixes * protocol pool module accounts * callback fixes * simapp fixes * register interfaces * fix feeibc * changes++ * fix missed conflict * fix wasm client * linting fixes * fix tests * linting * fix simapp tests * fix some tests * fix e2e go mod * go mod changes * remove grandpa * Gomod changes and minor updates (#7549) * Remove module.HasProposalMsgs * Update ante handler * Bump versions * x * Cleanup * chore: use same tags for all go mods * chore: update go mod files, revert callbacks simapp changes * chore: fix tendermint client test suite * chore: fix tests in TestAnteDecoratorCheckTx * chore: fix event tests * chore: fix compiler error in simapp * deps: update to latest of cosmossdk.io/store main to fix proofs test * chore: tidy all * fix: remove ibcmock module acc from blocked addrs * chore: fix proto failures and Gov errors * chore: fix ica test * chore: fix a few compiler errs in 08-wasm * fix: update ica account already exists test to front run account creation correctly * chore: fix ica test for address gen * chore: re-add log to returned error * chore: update go mods * chore: fixed build errors in wasm app.go * chore: cleanup callbacks simapp * chore: fixing simulation functions * chore: reconfigure simd cmd and fix TestInitCmd * feat: add runtime Env to icacontroller and fix v0.52 e2e tests (#7587) * feat: adding runtime Environment * chore: run make lint-fix * chore: remove duplicate import * chore: rm duplicate import * chore: migrate to env KVStoreService in icacontroller * chore: fix duplicate import * chore: rm kv store service * chore: refactor events to use environment * chore: make lint-fix * chore: make tidy-all * chore: rm msg service router from icacontroller * fix: remove 32 bit arm arch from callbacks ci builds * test: try cache false for callbacks linter * chore: pin to updated interchaintest fork * chore: revert relayer count to 10 * chore: continued work on upgrading e2e tests to 0.52 * chore: transfer e2e passing * chore: fix upgrade build errors * chore: bump interchaintest * chore: update sample config * chore: reconfigure protoany to use gogoproto any in favour of codec types * chore: tidy imports in e2e * fix: address failure in module safe query ica e2e test * bump golangci version to 1.60 * lint: remove duplicate import * chore: use gogoprotoany in favour of codec types any. rm ProtoCodecMarshaler for Codec iface * chore: bump go version in Dockerfile * chore: add todo * lint: fix unnecessary cast * deps: upgrade sdk to head of release/v0.52.x, upgrade store to latest main * deps: bump to head of release/v0.52 * fix: add unit test to reproduce e2e test failure for ScheduleIBCUpgrade, update code to expect tendermint client temporarily * fix: error in tests * chore: address comments from pr --------- Co-authored-by: chatton <[email protected]> Co-authored-by: Nikolas De Giorgis <[email protected]> * chore: linter fixes * lint: ignore redunant import alias * chore: rm redudant semi-colon in tendermint.proto * chore: rm redundant doc string regarding e2e-compatibility tests - tests now use annoations for compatibility checks * chore: update dead links * fix: apply fix for x/tx cosmos-sdk #22574 * chore: make lint-fix * fix: use hermes v1.10.4 for ci configs * fix: apply fix for cosmos-sdk #22779 and update dependencies * fix: unpin local sdk to HEAD of release/v0.52 in simapp go.mod * chore: revert docs/versioned_docs to main * chore: ammendments to linter configs * chore: update var name in expected keeper * chore: revert diffs in clienttypes/height_test * chore: propagate error from event emission in icacontroller * test: revert unnecessary diffs in test fn * chore: update comment in test chain * Update e2e/tests/interchain_accounts/base_test.go Co-authored-by: DimitrisJim <[email protected]> * chore: nit on import aliasing * refactor: rename instances of expected keepers from accountKeeper -> authKeeper where apt * chore: readd TestABCICodeDeterminism to 04-channel types * refactor: remove redundant AppModuleBasics * chore: rm GetStakingKeeper api from testing app * chore: update version in wasm dockerfile * chore: update the go work example file version * chore: rm UnwrapSDKContext in ica where possible * fix: import path in e2e from suggestion --------- Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: chatton <[email protected]> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Nikolas De Giorgis <[email protected]> Co-authored-by: DimitrisJim <[email protected]>
Description
Initially adding runtime Environment to ica to address events propagation issue with unwrapping context.
All tests seem to be now passing on this branch,
except for some build err in e2e unit tests which we will address separately.Update: E2E and unit tests passing on this branch. Issues created for follow up items.
ref: #7223
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.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.