Bump cosmos sdk #173

KonradStaniec · 2022-10-22T11:15:33Z

bumps cosmos-sdk to 0.46.3 (and some transitive deps)

SebastianElvis

It's quite challenging to dodge all the weird errors when bumping a dependency with breaking changes! I only have minor comments and it would be good to see others' comments as well.

SebastianElvis · 2022-10-23T23:30:54Z

app/app.go


 	// ... other modules keepers
 	// TODO: Create IBC keeper

 	// register the proposal types
-	govRouter := govtypes.NewRouter()
-	govRouter.AddRoute(govtypes.RouterKey, govtypes.ProposalHandler).
+	// Register the proposal types


This line looks duplicated

SebastianElvis · 2022-10-23T23:33:56Z

app/test_helpers.go

@@ -241,7 +377,7 @@ func createIncrementalAccounts(accNum int) []sdk.AccAddress {
 		buffer.WriteString("A58856F0FD53BF058B4909A21AEC019107BA6") // base address string

 		buffer.WriteString(numString) // adding on final two digits to make addresses unique
-		res, _ := sdk.AccAddressFromHex(buffer.String())
+		res, _ := sdk.AccAddressFromHexUnsafe(buffer.String())


Why is this function not safe anymore?

from godoc:

// Note, this function is considered unsafe as it may produce an AccAddress from // otherwise invalid input, such as a transaction hash. Please use // AccAddressFromBech32.

so presumably it was always unsafe but now it is clear from name, and when accepting addresses from outside we should always use bech32. As this is used only in test it is probably fine here.

vitsalis

Amazing work! Few comments

vitsalis · 2022-10-24T06:45:43Z

app/app.go

@@ -272,7 +276,7 @@ func NewBabylonApp(

 	// add keepers
 	app.AccountKeeper = authkeeper.NewAccountKeeper(
-		appCodec, keys[authtypes.StoreKey], app.GetSubspace(authtypes.ModuleName), authtypes.ProtoBaseAccount, maccPerms,
+		appCodec, keys[authtypes.StoreKey], app.GetSubspace(authtypes.ModuleName), authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix,


This is cosmos (ref). Should we use bbn here?

vitsalis · 2022-10-24T06:47:22Z

app/app.go

@@ -489,7 +504,7 @@ func NewBabylonApp(
 		capability.NewAppModule(appCodec, *app.CapabilityKeeper),
 		feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry),
 		gov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper),
-		mint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper),
+		mint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper, nil),


Why do we set types.InflationCalculationFn to nil?

because this inflation calculation to:
ic = types.DefaultInflationCalculationFn

underneath. This should be fine for now as we do not have any tokenomics design yet.

vitsalis · 2022-10-24T06:49:01Z

app/app.go

@@ -707,7 +726,7 @@ func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino
 	paramsKeeper.Subspace(minttypes.ModuleName)
 	paramsKeeper.Subspace(distrtypes.ModuleName)
 	paramsKeeper.Subspace(slashingtypes.ModuleName)
-	paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govtypes.ParamKeyTable())
+	paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govv1.ParamKeyTable())


We seem to be using both govv1 and govv1beta1 in this module. Should we use the same version?

I guess it's because some parts which were in beta1 has been deprecated but we keep using it.

tbh I mostly got that from cosmos-sdk simapp which I treat as kinda blueprint how default wiring should looks like.

My guess simapp not migrated fully to govv1 therefore it is in this half finished state. I will leave some todo here to investigate if there is better way to wire it 👍

vitsalis · 2022-10-24T06:55:50Z

cmd/babylond/cmd/cmd_test.go

@@ -20,5 +20,5 @@ func TestInitCmd(t *testing.T) {
 		fmt.Sprintf("--%s=%s", cli.FlagOverwrite, "true"), // Overwrite genesis.json, in case it already exists
 	})

-	require.NoError(t, svrcmd.Execute(rootCmd, app.DefaultNodeHome))
+	require.NoError(t, svrcmd.Execute(rootCmd, "", app.DefaultNodeHome))


Why do we set "" as the envPrefix?

from cosmos-sdk 0.46 is possible to have custom prefix for env variables - cosmos/cosmos-sdk#10950, for now we do not have such prefix therefore we can have it as empty string.

I will extract this to separate variable and add comment about it.

vitsalis · 2022-10-24T07:01:07Z

x/btclightclient/types/msgs_test.go

@@ -1,13 +1,16 @@
 package types_test

 import (
+	sdkmath "cosmossdk.io/math"


Why not use github.com/cosmos-sdk/cosmos/math?

because from 0.46 the old github.com/cosmos-sdk/cosmos/math module was deleted, and separae go module under path "cosmossdk.io/math" was released. Now it is separate library with sperate go.mod file.

gitferry

Great work! Thanks for making the bump happen! Only left some tiny comments

gitferry · 2022-10-24T07:23:49Z

app/test_helpers.go

-// Setup initializes a new BabylonApp. A Nop logger is set in BabylonApp.
-func Setup(isCheckTx bool) *BabylonApp {
-	app, genesisState := setup(!isCheckTx, 5)
+// NewSimappWithCustomOptions initializes a new SimApp with custom options.


Suggested change

// NewSimappWithCustomOptions initializes a new SimApp with custom options.

// NewBabyblonAppWithCustomOptions initializes a new BabylonApp with custom options.

gitferry · 2022-10-24T07:35:52Z

app/utils.go

@@ -58,7 +59,8 @@ func InitPrivSigner(clientCtx client.Context, nodeDir string, kr keyring.Keyring
 	}
 	wrappedPV := privval.LoadOrGenWrappedFilePV(pvKeyFile, pvStateFile)

-	// setup client context
+	// TODO this should probably not create separate config, but rahter accept it


Nice findings! Could you please create an issue for this just in case we forget it?

gitferry · 2022-10-24T07:47:57Z

x/checkpointing/keeper/bls_signer_test.go

+	if err != nil {
+		t.Fatalf("Failed to start test")
+	}


Suggested change

if err != nil {

t.Fatalf("Failed to start test")

}

require.NoError(t, err)

gitferry · 2022-10-24T07:52:04Z

x/epoching/simulation/decoder_test.go

@@ -24,7 +24,7 @@ var (
 )

 func TestDecodeStore(t *testing.T) {
-	cdc := simapp.MakeTestEncodingConfig().Marshaler
+	cdc := simapp.MakeTestEncodingConfig().Codec


Suggested change

cdc := simapp.MakeTestEncodingConfig().Codec

cdc := app.MakeTestEncodingConfig().Codec

Making this suggestion is because maybe we should use "github.com/babylonchain/babylon/app" other than simapp

aakoshh · 2022-10-24T09:47:34Z

app/test_helpers.go

 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
+	ksec256k1 "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"


What "ksec"?

it was supposed to be abbreviation from keysec256k1 which makes little sense thinking about it now. I will leave import as secp256k1

aakoshh · 2022-10-24T09:49:32Z

app/test_helpers.go

+	privVal := datagen.NewPV()
+	pubKey, err := privVal.GetPubKey()
+	require.NoError(t, err)
+	// create validator set with single validator
+	validator := tmtypes.NewValidator(pubKey, 1)
+	valSet := tmtypes.NewValidatorSet([]*tmtypes.Validator{validator})
+
+	// generate genesis account
+	senderPrivKey := ksec256k1.GenPrivKey()
+
+	acc := authtypes.NewBaseAccount(senderPrivKey.PubKey().Address().Bytes(), senderPrivKey.PubKey(), 0, 0)
+	balance := banktypes.Balance{
+		Address: acc.GetAddress().String(),
+		Coins:   sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100000000000000))),
+	}


The docs of this method could mention that there will only be a single validator with some hardcoded amount of tokens, explain what the purpose of it all is.

Sure will add comment.

It is necessary to create at least one validator here as from cosmos-sdk 0.46, there must be at least one validator in validator set during InitGenesis call (which is called when starting babylonapp)

aakoshh · 2022-10-24T09:56:18Z

app/test_helpers.go

+			Jailed:            false,
+			Status:            stakingtypes.Bonded,
+			Tokens:            bondAmt,
+			DelegatorShares:   sdk.OneDec(),


Why does everyone has "1 base denom unit"?

I that every validator delegating to themselves? Does it represent 100%?

hmm from what I understand from godoc:
delegator_shares defines total shares issued to a validator's delegators.
it is not percentage but rather total value of delgated shares.

From what I understand from cosmos-sdk docs this is something which eases up accounting when slashing.

### Delegator Shares When one Delegates tokens to a Validator they are issued a number of delegator shares based on a dynamic exchange rate, calculated as follows from the total number of tokens delegated to the validator and the number of shares issued so far: `Shares per Token = validator.TotalShares() / validator.Tokens()` Only the number of shares received is stored on the DelegationEntry. When a delegator then Undelegates, the token amount they receive is calculated from the number of shares they currently hold and the inverse exchange rate: `Tokens per Share = validator.Tokens() / validatorShares()` These `Shares` are simply an accounting mechanism. They are not a fungible asset. The reason for this mechanism is to simplify the accounting around slashing. Rather than iteratively slashing the tokens of every delegation entry, instead the Validators total bonded tokens can be slashed, effectively reducing the value of each issued delegator share.

From code perspective, it cannot be set to 0 as then InitGenesis will fail with:

ok github.com/babylonchain/babylon/x/btclightclient/types (cached) --- FAIL: TestInitGenesis (0.01s) panic: division by zero [recovered] panic: division by zero

aakoshh · 2022-10-24T10:01:22Z

app/test_helpers.go

+// Setup initializes a new SimApp. A Nop logger is set in SimApp.
+func Setup(t *testing.T, isCheckTx bool) *BabylonApp {


Suggested change

// Setup initializes a new SimApp. A Nop logger is set in SimApp.

func Setup(t *testing.T, isCheckTx bool) *BabylonApp {

// Setup initializes a new BabylonApp. A Nop logger is set in BabylonApp.

func Setup(t *testing.T, isCheckTx bool) *BabylonApp {

I don't see this "Nop logger" in the setup, but the fact a random single validator will be used would be worth mentioning.

yea noop logger is used really deep in call stack in setup function.

func setup(withGenesis bool, invCheckPeriod uint) (*BabylonApp, GenesisState) { db := dbm.NewMemDB() encCdc := MakeTestEncodingConfig() privSigner, err := SetupPrivSigner() if err != nil { panic(err) } app := NewBabylonApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, DefaultNodeHome, invCheckPeriod, encCdc, privSigner, EmptyAppOptions{})

aakoshh · 2022-10-24T10:15:46Z

cmd/babylond/main.go

@@ -15,7 +15,7 @@ func main() {
 	params.SetAddressPrefixes()
 	rootCmd, _ := cmd.NewRootCmd()

-	if err := svrcmd.Execute(rootCmd, app.DefaultNodeHome); err != nil {
+	if err := svrcmd.Execute(rootCmd, "", app.DefaultNodeHome); err != nil {


Maybe instead of passing these "" around there there could be some const field in the module that makes this readable, like const EmptyWhateverParameter = "" and you could pass that.

aakoshh · 2022-10-24T10:16:52Z

x/btccheckpoint/types/btcutils.go

+//	         r
+//	   		 /  \
+//			 d1    d2
+//	    /  \   / \
+//	   l1  l2 l3 l4


This doesn't look right.

aakoshh

You're awesome, LGTM!

KonradStaniec · 2022-10-24T16:24:13Z

Merged, to avoid blocking repo for too long. If there are other comments or remarks, please leave them here and ping me on slack, I can introduce them in separate pr :)

KonradStaniec added 3 commits October 22, 2022 12:13

Bump compiles

107566e

Bump unit tests pass

6c5ab1e

Bump integration tests pass

46a5bb8

KonradStaniec requested review from vitsalis, SebastianElvis, gitferry, aakoshh and gusin13 October 22, 2022 12:06

SebastianElvis reviewed Oct 23, 2022

View reviewed changes

vitsalis reviewed Oct 24, 2022

View reviewed changes

gitferry approved these changes Oct 24, 2022

View reviewed changes

aakoshh reviewed Oct 24, 2022

View reviewed changes

aakoshh approved these changes Oct 24, 2022

View reviewed changes

Pr comments

0de9dd3

KonradStaniec merged commit ca4f893 into main Oct 24, 2022

KonradStaniec mentioned this pull request Oct 25, 2022

chore: bump Cosmos SDK dependency to v0.46.0 #93

Closed

KonradStaniec deleted the bump-cosmos-sdk branch March 17, 2023 10:58

vitsalis pushed a commit that referenced this pull request Jan 21, 2024

Update wasmvm to latest v1.5.1 (#173)

60008e5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump cosmos sdk #173

Bump cosmos sdk #173

KonradStaniec commented Oct 22, 2022

SebastianElvis left a comment

SebastianElvis Oct 23, 2022

SebastianElvis Oct 23, 2022

KonradStaniec Oct 24, 2022

vitsalis left a comment

vitsalis Oct 24, 2022

vitsalis Oct 24, 2022

KonradStaniec Oct 24, 2022

vitsalis Oct 24, 2022

aakoshh Oct 24, 2022

KonradStaniec Oct 24, 2022

vitsalis Oct 24, 2022

KonradStaniec Oct 24, 2022

vitsalis Oct 24, 2022

KonradStaniec Oct 24, 2022

gitferry left a comment

gitferry Oct 24, 2022

gitferry Oct 24, 2022

KonradStaniec Oct 24, 2022

gitferry Oct 24, 2022

gitferry Oct 24, 2022

aakoshh Oct 24, 2022

KonradStaniec Oct 24, 2022

aakoshh Oct 24, 2022

KonradStaniec Oct 24, 2022

aakoshh Oct 24, 2022 •

edited

Loading

KonradStaniec Oct 24, 2022

aakoshh Oct 24, 2022

KonradStaniec Oct 24, 2022

aakoshh Oct 24, 2022

aakoshh Oct 24, 2022

aakoshh left a comment

KonradStaniec commented Oct 24, 2022

	// NewSimappWithCustomOptions initializes a new SimApp with custom options.
	// NewBabyblonAppWithCustomOptions initializes a new BabylonApp with custom options.

	cdc := simapp.MakeTestEncodingConfig().Codec
	cdc := app.MakeTestEncodingConfig().Codec

		"github.com/cosmos/cosmos-sdk/crypto/keyring"
		ksec256k1 "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"

		// Setup initializes a new SimApp. A Nop logger is set in SimApp.
		func Setup(t testing.T, isCheckTx bool) BabylonApp {

Bump cosmos sdk #173

Bump cosmos sdk #173

Conversation

KonradStaniec commented Oct 22, 2022

SebastianElvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

KonradStaniec commented Oct 24, 2022

aakoshh Oct 24, 2022 •

edited

Loading