Skip to content
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

Update Grandpa Contract Test to work with SDK v0.50 #4959

Merged
merged 121 commits into from
Oct 26, 2023

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Oct 26, 2023

Description

closes: #4800

E2E test run here - the test failures are actually using images from main and are should not be relevant for this PR. If these are still failing after we sync with main, I will look into them.

The scope of work for this issue grew over time as additional bugs popped up. The following changes were needed to support sdk 50 with the test.

  • Adds support for the hyperpace relayer. It can now be specified when using the relayer.New fn.
  • split up the SetupChainsRelayerAndChannel into sub functions to enable more granular control in tests. e.g. the connection and channel handshakes themselves are under test.
  • Add grpc endpoints for wasm types.
  • Update occurrences of cosmos.CosmosChain to ibc.Chain, this is to allow other implementations to be subbed in. (used only in this test suite.)
  • Increase voting period used in tests. ( MsgStoreCode is the largest message used in the tests so far, and required this increase (shout out to @colin-axner for the suggestion ❤️ )
  • DefaultGasValue needed to be bumped, the existing value was not enough for the test.
  • Interchain test bumped to include this fix which was also required due to large tx sizes.

I created this issue on the SDK to cover the CLI errors that I ran into. The current code does not make use of simd q for gov proposals as it fails when there is an embedded Any.

Next Steps:


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.

@chatton chatton marked this pull request as ready for review October 26, 2023 13:06
@chatton chatton changed the title Update Grandpa Contract Test to work with SDK v0.50 [Don't Review Yet] Update Grandpa Contract Test to work with SDK v0.50 Oct 26, 2023
@@ -107,6 +107,9 @@ include contrib/devtools/Makefile

BUILD_TARGETS := build install

tidy-all:
./scripts/go-mod-tidy-all.sh
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included this from main to reduce headaches when messing with multiple go mods.

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot woot!! 🚀 Superb work my friend!! All looks fantastic to me. I left some notes/suggestions, but mostly just commented on things that might have gotten overlooked after all the debugging. Happy with how it is now :)

e2e/testsuite/tx.go Outdated Show resolved Hide resolved
e2e/testsuite/testconfig.go Outdated Show resolved Hide resolved
e2e/testsuite/testconfig.go Outdated Show resolved Hide resolved
e2e/tests/wasm/grandpa_test.go Outdated Show resolved Hide resolved
e2e/tests/wasm/grandpa_test.go Outdated Show resolved Hide resolved
@faddat
Copy link
Contributor

faddat commented Oct 26, 2023

Thank you

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incredible! Thank you.

func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channelOpts ...func(*ibc.CreateChannelOptions)) (ibc.Relayer, ibc.ChannelOutput) {
chainA, chainB := s.GetChains()
func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channelOpts func(*ibc.CreateChannelOptions), chainSpecOpts ...ChainOptionConfiguration) (ibc.Relayer, ibc.ChannelOutput) {
chainA, chainB := s.GetChains(chainSpecOpts...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could add this as a new function, something like SetupChainsRelayerAndChannelWithChainSpecs, which would help reduce the diffs in the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern there is that we don't really need an additional function (yet!), since we either configure everything, or we do just the chains like in the new test that's being added.

I'm hoping the diff wasn't too bad 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's alright. Makes sense!

e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testvalues/values.go Show resolved Hide resolved
@chatton
Copy link
Contributor Author

chatton commented Oct 26, 2023

I'm going to merge this PR, I'm seeing that there is one test failure that is using the regular simd (not the wasm image). I plan on syncing the feature branch with main and then re-running that test. I will dig into it further if it fails again then!

@chatton chatton merged commit 1d31c73 into feat/wasm-clients Oct 26, 2023
58 of 59 checks passed
@chatton chatton deleted the cian/issue#4800-refactor-grandpa-test branch October 26, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants