!upgrade: Cosmos SDK to v0.50.x, ibc v8

[tuantran1702]

Overview

Closes #941

This upgrade bumps the project's SDK to version v0.50.3, IBC to v8 but does not supposed to introduce any changes to existing application functionality. This document outlines the major areas of change and their impact.

Current issues

• Wasmbindings(in review)
• x/airdrop (In review)
• x/interchainstaking(In review)
• icq-relayer
• tpsCounter
• utils folder(proofs.go)
• transferMiddleware change(from main branch)
• upgrade.go, upgrade_test.go(test failing for unknown reasons)
• refactor remaining changes according to main branch to achieve parity(should do last to avoid confusion)

High-level Changes

Bump Cosmos SDK from v0.46.16 to v0.50.3
Bumping IBC from v5 to v8
Refactor modules to match new SDK syntax
Update both quicksilver and third-party proto-file to use newer libraries.
Migrate deprecated methods from older SDK to quicksilver

Detailed Changes

App folder

• Basic Module Manager

• Remove app.RegisterRoutes

• Remove the need to passing encodingConfig to NewQuicksilver, just create both Codec and LegacyAmino inside the function body

interfaceRegistry, err := types.NewInterfaceRegistryWithOptions(types.InterfaceRegistryOptions{
		ProtoFiles: proto.HybridResolver,
		SigningOptions: signing.Options{
			AddressCodec: address.Bech32Codec{
				Bech32Prefix: sdk.GetConfig().GetBech32AccountAddrPrefix(),
			},
			ValidatorAddressCodec: address.Bech32Codec{
				Bech32Prefix: sdk.GetConfig().GetBech32ValidatorAddrPrefix(),
			},
		},
	})
	if err != nil {
		panic(err)
	}
	appCodec := codec.NewProtoCodec(interfaceRegistry)
	cdc := codec.NewLegacyAmino()
	txConfig := authtx.NewTxConfig(appCodec, authtx.DefaultSignModes)
	

• Refactor BeginBlocker and EndBlocker function signature, now they are just accepting one single context argument

- BeginBlock(sdk.Context, abci.RequestBeginBlock)
+ BeginBlock(context.Context) error

- EndBlock(sdk.Context, abci.RequestEndBlock) []abci.ValidatorUpdate
+ EndBlock(context.Context) error

• Removal of DeliverTx in BaseApp interface https://github.com/cosmos/cosmos-sdk/blob/release/v0.50.x/UPGRADING.md#baseapp

  BaseApp calls of BeginBlock & Endblock are now private but are still exposed to the application to define via the Manager type. FinalizeBlock is public and should be used in order to test and run operations. This means that although BeginBlock & Endblock no longer exist in the ABCI interface, they are automatically called by BaseApp during FinalizeBlock. Specifically, the order of operations is BeginBlock -> DeliverTx (for all txs) -> EndBlock.
  They do deliverTx privately, during the finalizeBlock method of ABCI. https://github.com/cosmos/cosmos-sdk/blob/7685abb8857773c147b7ba49496e8b9b06d13c0d/baseapp/abci.go#L713-L862. Not sure how we can add custom logic to the deliverTx phase now, to support logic of tpsCounter

• Add custom logic for FinalizeBlock, to migrate the store correctly

  quicksilver/app/app.go

  Lines 532 to 551 in 0bec6c1

  	func (app *Quicksilver) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) {
  	// when skipping sdk 47 for sdk 50, the upgrade handler is called too late in BaseApp
  	// this is a hack to ensure that the migration is executed when needed and not panics
  	app.once.Do(func() {
  	ctx := app.NewUncachedContext(false, tmproto.Header{})
  	if _, err := app.ConsensusParamsKeeper.Params(ctx, &consensusparamtypes.QueryParamsRequest{}); err != nil {
  	// prevents panic: consensus key is nil: collections: not found: key 'no_key' of type github.com/cosmos/gogoproto/tendermint.types.ConsensusParams
  	// sdk 47:
  	// Migrate Tendermint consensus parameters from x/params module to a dedicated x/consensus module.
  	// see https://github.com/cosmos/cosmos-sdk/blob/v0.47.0/simapp/upgrades.go#L66
  	baseAppLegacySS := app.ParamsKeeper.Subspace(baseapp.Paramspace)
  	err := baseapp.MigrateParams(sdk.UnwrapSDKContext(ctx), baseAppLegacySS, app.ConsensusParamsKeeper.ParamsStore)
  	if err != nil {
  	panic(err)
  	}
  	}
  	})
  	return app.BaseApp.FinalizeBlock(req)
  	}

• Refactor MakeEncodingConfig. Now generated by creating a tempApp, then construct an encodingConfig from it. app/encoding.go

Cmd folder

Refactor based on the change on App folder, see detail on the https://github.com/quicksilver-zone/quicksilver/pull/1060/files#diff-b648a0ba5f73d63678f411584d6e97f8413a398ea4e50232311650b7f6b53ba1

Wasmbinding folder

Use NewMsgSubmitProposal to submit the MsgStoreCode

quicksilver/wasmbinding/test/custom_query_test.go

Lines 116 to 129 in 5393bba

	msgProposal, err := govv1.NewMsgSubmitProposal([]sdk.Msg{src}, sdk.NewCoins(sdk.NewInt64Coin("qck", 100000000)), govAddress, "test", "test", "test", true)
	require.NoError(t, err)
	// when stored
	// _, err = govKeeper.SubmitProposal(ctx, []sdk.Msg{msgContent}, "testing123")
	// require.NoError(t, err)
	// and proposal execute
	// em := sdk.NewEventManager()
	handler := govKeeper.Router().Handler(msgProposal)
	if handler == nil {
	t.Fatal("proposal handler not found")
	}
	_, err = handler(ctx, msgProposal)

Proto files

Quicksilver protos

• Change Int and Dec belong to old cosmos-sdk/types to cosmossdk/math by modifying the buf.yaml: https://github.com/quicksilver-zone/quicksilver/pull/1060/files#diff-456c026faff792a6ca7bd293f2453164c317ac23712c908b8526eef74e95c4d2
• Regenerated new protobuf files

Thirdparty protos

Overall, the changes are:
+ Gas datatype: from cosmos-sdktypes.Gas to plain uint64, since SDK50 removed Gas types
+ cosmos-sdktypes Dec and Int are replaced with cosmossdk.io/math LegacyDec and Int
+ fileDescriptor value are changed

Third party protos file(*.pb.go) are updated, generated by cloning the original changes, change their proto to use newer cosmos sdk math and gogoproto libraries to their protos, then generate pb.go files using protocgen.sh scripts. Finally, move the newly generated proto files to Quicksilver repos, and make some required changes.

Utility files

Migrated deprecated SDK46 banktypes method to quicksilver "bankutils" packages
utils/bankutils/bank.go, not sure if it is correct way to handle the deprecated stuffs, I just want to keep the old functionality.

Modules changes

Overview

The Module changes section is for describing the changes in each module
Since we update the SDK version to v0.50, all modules need to follow the change accordingly. So, most of the changes are just replace types.math-> cosmossdk.io/Math, Dec->LegacyDec, cosmos-sdk/stores->seperate stores packages. These changes are pretty trivial, so it won't be discussed in detailed

x/airdrop

Current status: In progress, some tests has not beed fixed/logic not refactored

No notable changes

x/claimsmanager

Current status: In Review
Similar to x/airdrop changes, but no existing bugs remain.

x/interchainquery

Current status: In Review
No notable changes, similar changes to x/airdrop

x/interchainstaking

client folder

Current status: In review
No notable changes

keeper folder

Current status: In review
a major change is introduced. I bring the x/auth txBuilder to the quicksilver repo, in order to construct a valid sdk.Tx from msgs. https://github.com/quicksilver-zone/quicksilver/pull/1060/files#diff-c0cf771f814c0fd6ba394ed186f92fd959d422f61279ec57aba38cc8ef83372b

since sdk.TxDecoder now return sdk.Tx, so isn't able to return tx.Tx anymore
https://github.com/cosmos/cosmos-sdk/blob/7685abb8857773c147b7ba49496e8b9b06d13c0d/x/auth/tx/decoder.go

I constructed the Tx using the new TxBuilder, and the tests seems to work nicely with the tests: https://github.com/quicksilver-zone/quicksilver/blob/tuan/upgrade-sdk-v0.50.x/x/interchainstaking/keeper/receipt_test.go#L146-L155

x/lsmtypes

Status: In review, test passed
No significant changes.
Detail changes: https://github.com/quicksilver-zone/quicksilver/pull/1060/files#diff-ace71f7aadb021f0ed0cabb67986e08b4ba90919b1733d397534760efbb67052

x/mint

Status: In review, test passed
No significant changes
https://github.com/quicksilver-zone/quicksilver/pull/1060/files#diff-97ab51578583f8ac4f726ae9ea3c6f6496483d85b488a0b37546accc310a87aa

x/participantsreward

Status: In review, test passed
using bankutils as replacement of SDK46 banktypes methods

Overall, still just changes similar to above modules.

x/supply

Status: In review, test passed
No significant changes

x/tokenfactory

Status: In review, test passed
No significant changes

Current issues

Wasmbindings

Currently, I am not able to fix some of the tests due to newer wasmtypes
Update(24 Jan): fixed in 5393bba
Updated changes in the Detailed Changes section

Utilities Files

utils/proofs.go

   // TODO: ibc v8 remove the GetRoot method of ConsensusState, so can not 	
path := commitmenttypes.NewMerklePath([]string{module, url.PathEscape(string(key))}...)

This line doesn't work anymore, and I haven't figured an alternative method to this

x/airdrop

x/airdrop/claim_handler.go

Currently the way we iterate through proposals:

k.govKeeper.IterateProposals(ctx, func(proposal govv1.Proposal) (stop bool) {
		_, found := k.govKeeper.GetVote(ctx, proposal.Id, addr)
		if found {
			voted = true
			return true
		}
		return false
	})

In SDK 50 they don't support the IterateProposals anymore, docs

They said that the state of gov modules is now managed by the Collection API, but I haven't figured a way to access the collections. cosmos/cosmos-sdk#16171

In the gov tests of Cosmos SDK, they access the Proposal field of govKeeper directly, but not sure how we could achieve the same thing, proposal_test.go

Update(24 Jan): Fixed in e0b669b, need review @joe-bowman @faddat

x/interchainstaking

• Not sure why the testchain generated using setupTestZone always has the "-1" at the end, so I need to modify some tests. for example "testchain2" -> "testchain2-1", not sure if it is my fault or some changes in the IBC library

• x/interchainstaking/zones_test.go
  Now, when the types.Zone is initialize like this

zone = types.Zone{
		ConnectionId: "conn-test",
		ChainId:      chainID,
		LocalDenom:   "uqck",
		BaseDenom:    "qck",

	}

then the RedemptionRate, the LastRedemptionRate and the Tvl value, which is all sdkmath.LegacyDec types, is stored as *big.Int(nil). But when it is stored and retrieved from the store, it is store in a different way.

Tvl: (math.LegacyDec) {
- i: (*big.Int)(<nil>)

+ i: (*big.Int)({
+ neg: (bool) false,
+ abs: (big.nat) <nil>

+ })
},

I resolve this by initialize it in the test

	zone = types.Zone{
		ConnectionId: "conn-test",
		ChainId:      chainID,
		LocalDenom:   "uqck",
		BaseDenom:    "qck",
		// TODO: Not sure if should initialized here or not.
		RedemptionRate:     sdkmath.LegacyNewDec(0),
		LastRedemptionRate: sdkmath.LegacyNewDec(0),
		Tvl:                sdkmath.LegacyNewDec(0),
	}

but not sure if this issue would cause any problem in production

icq-relayer

After bumping ibc-go to v8, a problem arise, that the MsgUpdateClient doesn't have Header anymore, and also the PackHeader and UnpackHeader method also been removed from the clienttypes package. Haven't figured out a way to fix it yet
https://github.com/quicksilver-zone/quicksilver/blob/main/icq-relayer/pkg/runner/run.go#L639-L649

Final thoughts

Most of the bugs I encountered during fixing the code is because the required module isn't properly registered, under new Cosmos SDK. The tests are very helpful to catch these kind of bugs, and also comparing the app initialization code from both https://github.com/CosmWasm/wasmd/blob/main/app/app.go and https://github.com/cosmos/ibc-go/blob/v8.0.0/testing/simapp/app.go prove to be very helpful for my upgrade process

[faddat]

hey @tropicaldog -- nice work here -- btw here's the v50 upgrade for lens, which is a dependency of quicksilver:

• https://github.com/notional-labs/lens

[faddat]

@tropicaldog FYI it's become known that comet v0.38.0 - v0.38.4 just plain don't work, so I have bumped those dependencies.

If you're looking for some guidance here, basically, I would say that we should try and:

• ensure that it compiles
• see if we can start up the chain in testnet, even without an upgrade handler
• ensure that we don't use vote extensions

[faddat]

hey @tropicaldog -- if you were wondering why nothing you did could fix it, please know that all of the issues you've been facing come from upstream. v0.38.3 was a build of comet fraught with issues and it's now been retracted.

[tuantran1702]

@faddat do you think it's good idea to postpone the upgrade and wait a little bit until the upstream code is more mature?

[faddat]

But yes but we should prepare the upgrade anyway. That will actually help us learn a bit more about the maturity of the code base.

So basically we can think about the next steps in segments. First of all we have to make it so that the chain runs from the binary that we produce, secondly we need to figure out tests. Thirdly, we need to write the upgrade handler. It's pretty important to us to get this upgrade finished relatively rapidly, and I believe that the problem with comet 38 is actually related to vote extensions. I have been working with the bera chain team about these issues.

So to answer your question, yes we definitely want to wait until it's more mature but also no we don't want to wait because then we might wait forever. So let's do our best to get the upgrade prepared and all of us will monitor the upstream situation because you're absolutely not wrong the upstream situation is a little weird.

This makes sense to you?

[faddat]

Well look you got a gift in the form of merge conflicts.

Let's try and work through these together

[tuantran1702]

@faddat yeah these protobuf files will certainly be diverging from the main branch at some point, so not really a big deal I guess.

[faddat]

Basically we want to always merge from main so that we are working against the latest code from the main branch.