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

sdk 50 #3808

Closed
wants to merge 71 commits into from
Closed

sdk 50 #3808

wants to merge 71 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jun 9, 2023

This is a very rough draft pr that upgrades ibc-go to sdk v0.50.x -- the idea is to have
this software available to speed up deployment on chains and in common ecosystem
libraries like wasmd.

  • sdk 50 first go
  • add go workspace

testing/chain.go Outdated
@@ -211,7 +212,7 @@
// QueryProofForStore performs an abci query with the given key and returns the proto encoded merkle proof
// for the query and the height at which the proof will succeed on a tendermint verifier.
func (chain *TestChain) QueryProofForStore(storeKey string, key []byte, height int64) ([]byte, clienttypes.Height) {
res := chain.App.Query(abci.RequestQuery{
res, err := chain.App.Query(abci.RequestQuery{

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This definition of err is never used.
testing/chain.go Outdated
@@ -235,7 +236,7 @@
// QueryUpgradeProof performs an abci query with the given key and returns the proto encoded merkle proof
// for the query and the height at which the proof will succeed on a tendermint verifier.
func (chain *TestChain) QueryUpgradeProof(key []byte, height uint64) ([]byte, clienttypes.Height) {
res := chain.App.Query(abci.RequestQuery{
res, err := chain.App.Query(abci.RequestQuery{

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This definition of err is never used.
testing/simapp/export.go Fixed Show fixed Hide fixed
testing/simapp/export.go Fixed Show fixed Hide fixed
@@ -103,12 +103,12 @@
// reinitialize all validators
app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
// donate any unwithdrawn outstanding reward fraction tokens to the community pool
scraps := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
feePool := app.DistrKeeper.GetFeePool(ctx)
scraps, err := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This definition of err is never used.
scraps := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
feePool := app.DistrKeeper.GetFeePool(ctx)
scraps, err := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
feePool, err := app.DistrKeeper.FeePool.Get(ctx)

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This definition of err is never used.
@@ -329,14 +345,16 @@

// SDK module keepers

app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, keys[authtypes.StoreKey], authtypes.ProtoBaseAccount, maccPerms, sdk.GetConfig().GetBech32AccountAddrPrefix(), authtypes.NewModuleAddress(govtypes.ModuleName).String())
// add keepers
app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())

Check warning

Code scanning / CodeQL

Directly using the bech32 constants

Directly using the bech32 constants instead of the configuration values
@@ -28,7 +28,7 @@
app.prepForZeroHeightGenesis(ctx, jailAllowedAddrs)
}

genState := app.mm.ExportGenesis(ctx, app.appCodec)
genState, err := app.ModuleManager.ExportGenesis(ctx, app.appCodec)

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This definition of err is never used.
@faddat
Copy link
Contributor Author

faddat commented Jun 15, 2023

Hey, the biggest remaining issue is this:

    /Users/faddat/ibc-go/modules/apps/transfer/keeper/suite.go:87: test panicked: type_url /ibc.applications.fee.v1.MsgRegisterPayee has not been registered yet. Before calling RegisterService, you must register all interfaces by calling the `RegisterInterfaces` method on module.BasicManager. Each module should call `msgservice.RegisterMsgServiceDesc` inside its `RegisterInterfaces` method with the `_Msg_serviceDesc` generated by proto-gen

And interestingly I have hit this several times recently, always when dealing with situations where I've rebuilt / redid protos. Anyone have any ideas?

Resolved, by addint the modulebasic manager to app.go

@faddat
Copy link
Contributor Author

faddat commented Jun 15, 2023

The next most frequent error is:

                Error:          Received unexpected error:
                                invalid height: 1; expected: 2
                Test:           TestTypesTestSuite/TestMarshalClientUpdateProposalProposal
    --- FAIL: TestTypesTestSuite/TestMarshalConsensusStateWithHeight (0.06s)
        chain.go:292: 
                Error Trace:    /Users/faddat/ibc-go/testing/chain.go:292
                                                        /Users/faddat/ibc-go/testing/coordinator.go:185
                                                        /Users/faddat/ibc-go/testing/chain.go:158
                                                        /Users/faddat/ibc-go/testing/chain.go:187
                                                        /Users/faddat/ibc-go/testing/coordinator.go:40
                                                        /Users/faddat/ibc-go/modules/core/02-client/types/msgs_test.go:28
                                                        /Users/faddat/go/pkg/mod/github.com/stretchr/testify@v1.8.4/suite/suite.go:187

Anyone know where we set expected for this?

@tac0turtle
Copy link
Member

after looking through the cosmos-sdk, it seems like we should get the validator set updates from the endblocker.

Is that correct?

updating the docs here cosmos/cosmos-sdk#16501. just about done but it has the begin and endblocker changes

modules/capability/go.mod Outdated Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented Jun 16, 2023

My most recent commit breaks a lot of things. I think there may be a design question for @crodriguezvega & the IBC team -- The thing that this seems to break most is the testing style that is used here in ibc-go.

Maybe we line up a call?

Note: I made this commit in part by using the goland side by side view and looking at the diff between the SDK's simapp. Where possible, I made ibc's simapp look like sdk's simapp.

@faddat
Copy link
Contributor Author

faddat commented Jun 16, 2023

nb:

https://github.com/cosmos/cosmos-sdk/blob/13e61bd0c5c7950d137bbdfb171f95d87179ed29/CHANGELOG.md?plain=1#L43

This is at a minimum going to affect how we work with packet.go

// GetBytes returns the JSON marshalled interchain account packet data.
func (iapd InterchainAccountPacketData) GetBytes() []byte {
	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
}

// GetBytes returns the JSON marshalled interchain account CosmosTx.
func (ct CosmosTx) GetBytes() []byte {
	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ct))
}

I assume that is a determinsim hack to ensure all nodes see the same thing?

For now I am going to do like:

// Deprecated: please delete this code eventually.
func mustSortJSON(bz []byte) []byte {
	var c any
	err := json.Unmarshal(bz, &c)
	if err != nil {
		panic(err)
	}
	js, err := json.Marshal(c)
	if err != nil {
		panic(err)
	}
	return js
}

but please note that the way I am doing it feels hacky violates DRY and should probably be changed at some point.

@faddat
Copy link
Contributor Author

faddat commented Jun 17, 2023

Yesterday I did a pretty interesting exercise where I tried to make the simapp folder match the SDK's as closely as I could.

I am going to do the same today, but with the main branch.

I think this will reduce the changeset.

@faddat
Copy link
Contributor Author

faddat commented Jun 17, 2023

Also might start this branch over, depends on findings from that.

@faddat
Copy link
Contributor Author

faddat commented Jun 17, 2023

@crodriguezvega hey, I think that what I will do here, is cherry pick commits beginning iwth the upgrade-sdk-v0.50 branch, just to keep this cleaner. I have merged main here, for example and I don't think that is desired.

@faddat
Copy link
Contributor Author

faddat commented Jun 17, 2023

closing in favor of a new branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants