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.

Sign up for GitHub

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

chore(x/distribution): use cosmossdk.io/core/codec instead of github.com/cosmos/cosmos-sdk/codec #23302

Closed
wants to merge 1 commit into from
Closed

chore(x/distribution): use cosmossdk.io/core/codec instead of github.com/cosmos/cosmos-sdk/codec #23302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions x/distribution/migrations/v4/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import (
gogotypes "github.com/cosmos/gogoproto/types"

"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/codec"
"cosmossdk.io/core/store"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -46,6 +46,9 @@ func GetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStore
// SetPreviousProposerConsAddr set the proposer public key for this block.
func SetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStoreService, cdc codec.BinaryCodec, consAddr sdk.ConsAddress) error {
kvStore := storeService.OpenKVStore(ctx)
bz := cdc.MustMarshal(&gogotypes.BytesValue{Value: consAddr})
bz, err := cdc.Marshal(&gogotypes.BytesValue{Value: consAddr})
if err != nil {
panic(err)
}
Comment on lines +49 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling pattern

The current implementation catches the error from Marshal only to immediately panic. This is inconsistent with the function's error return type and creates misleading API behavior. Consider either:

  1. Return the error directly:
 bz, err := cdc.Marshal(&gogotypes.BytesValue{Value: consAddr})
 if err != nil {
-    panic(err)
+    return err
 }
  1. Or if panicking is truly intended, use MustMarshal:
-bz, err := cdc.Marshal(&gogotypes.BytesValue{Value: consAddr})
-if err != nil {
-    panic(err)
-}
+bz := cdc.MustMarshal(&gogotypes.BytesValue{Value: consAddr})

Committable suggestion skipped: line range outside the PR's diff.

return kvStore.Set(OldProposerKey, bz)
}
8 changes: 6 additions & 2 deletions x/distribution/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"cosmossdk.io/collections"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/codec"
"cosmossdk.io/core/registry"
"cosmossdk.io/schema"
"cosmossdk.io/x/distribution/client/cli"
Expand All @@ -19,7 +20,6 @@ import (
"cosmossdk.io/x/distribution/types"

sdkclient "github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/simsx"
"github.com/cosmos/cosmos-sdk/types/module"
simtypes "github.com/cosmos/cosmos-sdk/types/simulation"
Expand Down Expand Up @@ -115,7 +115,11 @@ func (am AppModule) RegisterMigrations(mr appmodule.MigrationRegistrar) error {

// DefaultGenesis returns default genesis state as raw bytes for the distribution module.
func (am AppModule) DefaultGenesis() json.RawMessage {
return am.cdc.MustMarshalJSON(types.DefaultGenesisState())
data, err := am.cdc.MarshalJSON(types.DefaultGenesisState())
if err != nil {
panic(err)
}
return data
}

// ValidateGenesis performs genesis state validation for the distribution module.
Expand Down
58 changes: 43 additions & 15 deletions x/distribution/simulation/decoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import (
"bytes"
"fmt"

"cosmossdk.io/core/codec"
"cosmossdk.io/x/distribution/types"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/kv"
)
Expand All @@ -18,47 +18,75 @@ func NewDecodeStore(cdc codec.Codec) func(kvA, kvB kv.Pair) string {
switch {
case bytes.Equal(kvA.Key[:1], types.FeePoolKey):
var feePoolA, feePoolB types.FeePool
cdc.MustUnmarshal(kvA.Value, &feePoolA)
cdc.MustUnmarshal(kvB.Value, &feePoolB)
if err := cdc.Unmarshal(kvA.Value, &feePoolA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &feePoolB); err != nil {
panic(err)
}
return fmt.Sprintf("%v\n%v", feePoolA, feePoolB)

case bytes.Equal(kvA.Key[:1], types.ValidatorOutstandingRewardsPrefix):
var rewardsA, rewardsB types.ValidatorOutstandingRewards
cdc.MustUnmarshal(kvA.Value, &rewardsA)
cdc.MustUnmarshal(kvB.Value, &rewardsB)
if err := cdc.Unmarshal(kvA.Value, &rewardsA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &rewardsB); err != nil {
panic(err)
}
return fmt.Sprintf("%v\n%v", rewardsA, rewardsB)

case bytes.Equal(kvA.Key[:1], types.DelegatorWithdrawAddrPrefix):
return fmt.Sprintf("%v\n%v", sdk.AccAddress(kvA.Value), sdk.AccAddress(kvB.Value))

case bytes.Equal(kvA.Key[:1], types.DelegatorStartingInfoPrefix):
var infoA, infoB types.DelegatorStartingInfo
cdc.MustUnmarshal(kvA.Value, &infoA)
cdc.MustUnmarshal(kvB.Value, &infoB)
if err := cdc.Unmarshal(kvA.Value, &infoA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &infoB); err != nil {
panic(err)
}
return fmt.Sprintf("%v\n%v", infoA, infoB)

case bytes.Equal(kvA.Key[:1], types.ValidatorHistoricalRewardsPrefix):
var rewardsA, rewardsB types.ValidatorHistoricalRewards
cdc.MustUnmarshal(kvA.Value, &rewardsA)
cdc.MustUnmarshal(kvB.Value, &rewardsB)
if err := cdc.Unmarshal(kvA.Value, &rewardsA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &rewardsB); err != nil {
panic(err)
}
return fmt.Sprintf("%v\n%v", rewardsA, rewardsB)

case bytes.Equal(kvA.Key[:1], types.ValidatorCurrentRewardsPrefix):
var rewardsA, rewardsB types.ValidatorCurrentRewards
cdc.MustUnmarshal(kvA.Value, &rewardsA)
cdc.MustUnmarshal(kvB.Value, &rewardsB)
if err := cdc.Unmarshal(kvA.Value, &rewardsA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &rewardsB); err != nil {
panic(err)
}
return fmt.Sprintf("%v\n%v", rewardsA, rewardsB)

case bytes.Equal(kvA.Key[:1], types.ValidatorAccumulatedCommissionPrefix):
var commissionA, commissionB types.ValidatorAccumulatedCommission
cdc.MustUnmarshal(kvA.Value, &commissionA)
cdc.MustUnmarshal(kvB.Value, &commissionB)
if err := cdc.Unmarshal(kvA.Value, &commissionA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &commissionB); err != nil {
panic(err)
}
return fmt.Sprintf("%v\n%v", commissionA, commissionB)

case bytes.Equal(kvA.Key[:1], types.ValidatorSlashEventPrefix):
var eventA, eventB types.ValidatorSlashEvent
cdc.MustUnmarshal(kvA.Value, &eventA)
cdc.MustUnmarshal(kvB.Value, &eventB)
if err := cdc.Unmarshal(kvA.Value, &eventA); err != nil {
panic(err)
}
if err := cdc.Unmarshal(kvB.Value, &eventB); err != nil {
panic(err)
}
Comment on lines +21 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Reduce code duplication in error handling

The current implementation repeats the same error handling pattern 14 times. Consider creating a helper function to reduce duplication:

+func mustUnmarshalWithContext[T any](cdc codec.Codec, bz []byte, out T) {
+    if err := cdc.Unmarshal(bz, out); err != nil {
+        panic(err)
+    }
+}

 func NewDecodeStore(cdc codec.Codec) func(kvA, kvB kv.Pair) string {
     return func(kvA, kvB kv.Pair) string {
         switch {
         case bytes.Equal(kvA.Key[:1], types.FeePoolKey):
             var feePoolA, feePoolB types.FeePool
-            if err := cdc.Unmarshal(kvA.Value, &feePoolA); err != nil {
-                panic(err)
-            }
-            if err := cdc.Unmarshal(kvB.Value, &feePoolB); err != nil {
-                panic(err)
-            }
+            mustUnmarshalWithContext(cdc, kvA.Value, &feePoolA)
+            mustUnmarshalWithContext(cdc, kvB.Value, &feePoolB)

This would:

  1. Reduce code duplication
  2. Make the code more maintainable
  3. Keep the same functionality
  4. Make it easier to modify error handling in the future if needed

Committable suggestion skipped: line range outside the PR's diff.

return fmt.Sprintf("%v\n%v", eventA, eventB)

default:
Expand Down
Loading