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

[Dependencies] bump cosmos-sdk to v0.50.10 #865

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

okdas
Copy link
Member

@okdas okdas commented Oct 7, 2024

Summary

Bumps cosmos-sdk to the latest version, along with its dependencies.

Type of change

go get github.com/cosmos/[email protected]

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@okdas okdas added push-image CI related - pushes images to ghcr.io devnet devnet-test-e2e dependencies Pull requests that update a dependency file labels Oct 7, 2024
@okdas okdas self-assigned this Oct 7, 2024
Copy link

github-actions bot commented Oct 7, 2024

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 865)
Grafana network dashboard for devnet-issue-865

Copy link

github-actions bot commented Oct 7, 2024

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

@okdas
Copy link
Member Author

okdas commented Oct 7, 2024

After this is merged, protocol team members need to run to replace deprecated mockgen with uber fork.

go install "go.uber.org/mock/[email protected]" && mockgen --version

@okdas
Copy link
Member Author

okdas commented Oct 8, 2024

Unable to generate mocks, even after upgrading to uber's mockgen (as go's mock is deprecated).

Will need to come back to investigate further. It seems like a change in v0.50.10.

50.9..50.10 changes here.

The error I'm getting:

Reflection: can't yet turn interface { ProtoMessage(); Reset(); String() string } (interface) into a model.Type
Reflection: can't yet turn interface { ProtoMessage(); Reset(); String() string } (interface) into a model.Type
prog.go:15:2: no required module provides package github.com/pokt-network/poktroll/pkg/client: go.mod file not found in current directory or any parent directory; see 'go help modules'
prog.go:13:2: no required module provides package go.uber.org/mock/mockgen/model: go.mod file not found in current directory or any parent directory; see 'go help modules'
2024/10/07 16:58:06 Loading input failed: exit status 1
pkg/client/interface.go:4: running "mockgen": exit status 1

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Oct 8, 2024

Preliminary observations:

Given the output, mockgen seems to be struggling with something that appears to be the cosmostypes.Msg / proto.Message interface:

{ ProtoMessage(); Reset(); String() string }

I've eliminated all mockgen calls as being problematic except for these two in pkg/client/interfaces.go:

//go:generate mockgen -destination=../../testutil/mockclient/tx_client_mock.go -package=mockclient . TxContext,TxClient
// ...
//go:generate mockgen -destination=../../testutil/mockclient/cosmos_tx_builder_mock.go -package=mockclient github.com/cosmos/cosmos-sdk/client TxBuilder

Specifically, it seems to be the TxClient and TxBuilder types, which makes sense given they both use cosmostypes.Msg:

type TxClient interface {
	SignAndBroadcast(
		ctx context.Context,
		msgs ...cosmostypes.Msg,
	) either.AsyncError
}

pkg/client/interface.go:81

	TxBuilder interface {
		GetTx() signing.Tx

		SetMsgs(msgs ...sdk.Msg) error
		// ...
	}

godocs

Investigating the mockgen v0.4.0 source seems to indicate that it can only handle interface types which either have no methods or are an error. However, the original mockgen seems to have the same code in this area (even in v1.6.0, what we were using); more context is required to understand what's changed. 🤔

The use of a generated prog.go for reflection mode seems to be going away in the next version as reflectMode() has been replaced with packageModeParser#parsePackage. I tried to install the main version (go install "github.com/uber-go/mock@main") but the result of running mockgen was a different error:

2024/10/08 12:06:55 Loading input failed: extract interfaces from package: parse interface: error parsing TxClient: parse method: error parsing func(ctx context.Context, msgs ...interface{ProtoMessage(); Reset(); String() string}) github.com/pokt-network/poktroll/pkg/either.AsyncError: parse parameter type: error parsing interface{ProtoMessage(); Reset(); String() string}: cannot handle non-empty unnamed interfaces
pkg/client/interface.go:4: running "mockgen": exit status 1

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Oct 8, 2024

More observations

I've been experimenting with the TxClient interface:

  • Switching between cosmostypes.Msg and proto.Message doesn't seem to make any difference.

  • Defining a new interface that either embeds or implements proto.Message makes the error go away for TxClient.

    While this might be a slightly ugly workaround for TxClient, this doesn't solve for TxBuilder as it's buried in cosmos-sdk source.

  • Nothing seems to have changed with respect to cosmostypes.Msg or proto.Message between cosmos-sdk 0.50.9 -> 0.50.10 & cosmos/gogoproto 1.6.0 -> 1.7.0.

  • At this point, my next thought would be to explore using source mode instead of reflect mode but again, this doesn't solve for TxBuilder unless we start cloning or otherwise pulling down cosmos-sdk source files for development and testing. 🙄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file devnet devnet-test-e2e push-image CI related - pushes images to ghcr.io
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants