Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Switch to collections in the ibchooks module [#2492](https://github.com/provenance-io/provenance/issues/2492).
2 changes: 1 addition & 1 deletion app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ func New(
// Configure the hooks keeper
hooksKeeper := ibchookskeeper.NewKeeper(
appCodec,
keys[ibchookstypes.StoreKey],
runtime.NewKVStoreService(keys[ibchookstypes.StoreKey]),
app.IBCKeeper.ChannelKeeper,
nil,
)
Expand Down
74 changes: 49 additions & 25 deletions x/ibchooks/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import (

"github.com/cometbft/cometbft/crypto/tmhash"

"cosmossdk.io/collections"
corestore "cosmossdk.io/core/store"
sdkerrors "cosmossdk.io/errors"
"cosmossdk.io/log"
storetypes "cosmossdk.io/store/types"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -26,29 +27,46 @@ import (

type (
Keeper struct {
cdc codec.BinaryCodec
storeKey storetypes.StoreKey

cdc codec.BinaryCodec
storeService corestore.KVStoreService
schema collections.Schema
channelKeeper types.ChannelKeeper
ContractKeeper *wasmkeeper.PermissionedKeeper
authority string
// Params stores the module's persistent parameters using the legacy key (0x01).
params collections.Item[types.Params]
// PacketCallbacks stores temporary callback state for packets.
// Maps (channel, sequence) to a contract bech32 address (prefix 0x02).
packetCallbacks collections.Map[collections.Pair[string, uint64], string]
// PacketAckActors maps (channel, sequence) to "contract::hash" bytes.
// Used to store temporary acknowledgment state for each packet (prefix 0x03).
packetAckActors collections.Map[collections.Pair[string, uint64], []byte]
}
)

// NewKeeper returns a new instance of the x/ibchooks keeper
func NewKeeper(
cdc codec.BinaryCodec,
storeKey storetypes.StoreKey,
storeService corestore.KVStoreService,
channelKeeper types.ChannelKeeper,
contractKeeper *wasmkeeper.PermissionedKeeper,
) Keeper {
sb := collections.NewSchemaBuilder(storeService)
keeper := Keeper{
cdc: cdc,
storeKey: storeKey,
channelKeeper: channelKeeper,
ContractKeeper: contractKeeper,
authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(),
cdc: cdc,
storeService: storeService,
channelKeeper: channelKeeper,
ContractKeeper: contractKeeper,
authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(),
params: collections.NewItem(sb, types.ParamsKeyPrefix, "params", codec.CollValue[types.Params](cdc)),
packetCallbacks: collections.NewMap(sb, types.PacketCallbackKeyPrefix, "packet_callbacks", collections.PairKeyCodec(collections.StringKey, collections.Uint64Key), collections.StringValue),
packetAckActors: collections.NewMap(sb, types.PacketAckKeyPrefix, "packet_ack_actors", collections.PairKeyCodec(collections.StringKey, collections.Uint64Key), collections.BytesValue),
}
schema, err := sb.Build()
if err != nil {
panic(err)
}
keeper.schema = schema
return keeper
}

Expand Down Expand Up @@ -108,39 +126,44 @@ func GeneratePacketAckValue(packet channeltypes.Packet, contract string) ([]byte

// StorePacketCallback stores which contract will be listening for the ack or timeout of a packet
func (k Keeper) StorePacketCallback(ctx sdk.Context, channel string, packetSequence uint64, contract string) {
store := ctx.KVStore(k.storeKey)
store.Set(GetPacketCallbackKey(channel, packetSequence), []byte(contract))
if err := k.packetCallbacks.Set(ctx, collections.Join(channel, packetSequence), contract); err != nil {
panic(err)
}
}

// GetPacketCallback returns the bech32 addr of the contract that is expecting a callback from a packet
func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string {
store := ctx.KVStore(k.storeKey)
return string(store.Get(GetPacketCallbackKey(channel, packetSequence)))
v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence))
if err != nil {
return ""
}
return v
}
Comment on lines 135 to 141

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In cosmossdk.io/collections v1.4.0, does Map.Get return collections.ErrNotFound for a missing key, and what other errors can it return (e.g. value decoding errors)?

💡 Result:

In cosmossdk.io/collections v1.4.0, the Map.Get method does return collections.ErrNotFound when a requested key is missing from the underlying storage [1][2]. Beyond ErrNotFound, Map.Get can return other errors, most notably those related to encoding and decoding [1]. Because the collections framework relies on value codecs to deserialize data retrieved from the store, any failure during this deserialization process will result in an error, typically categorized under collections.ErrEncoding (which is aliased to the SDK's codec encoding error) [1]. Additionally, because Map.Get interacts with the underlying KVStore, it can propagate errors returned by the storage layer itself (e.g., database access issues). Developers frequently use errors.Is(err, collections.ErrNotFound) to check for missing keys specifically, allowing them to handle such cases—such as by returning a default value—differently than actual system or data integrity errors [2].

Citations:


🏁 Script executed:

cat -n x/ibchooks/keeper/keeper.go | sed -n '162,184p'

Repository: provenance-io/provenance

Length of output: 970


🏁 Script executed:

rg --type go "func.*GetParams.*Returns" -A 20

Repository: provenance-io/provenance

Length of output: 162


🏁 Script executed:

rg --type go "func.*GetParams" -A 15

Repository: provenance-io/provenance

Length of output: 162


🏁 Script executed:

rg --type go "GetParams" -n -B 2 -A 15

Repository: provenance-io/provenance

Length of output: 162


🏁 Script executed:

find . -name "params.go" -type f | head -5

Repository: provenance-io/provenance

Length of output: 311


🏁 Script executed:

cat -n ./x/exchange/keeper/params.go | grep -A 15 "func.*GetParams"

Repository: provenance-io/provenance

Length of output: 1363


🏁 Script executed:

rg --type go "collections.ErrNotFound" -n -B 3 -A 5 | head -40

Repository: provenance-io/provenance

Length of output: 162


🏁 Script executed:

rg "ErrNotFound" --type go -n | head -20

Repository: provenance-io/provenance

Length of output: 162


Avoid swallowing non-ErrNotFound errors from collections Get.

GetPacketCallback and GetPacketAckActor return empty results on any error from Get. In cosmossdk.io/collections, Map.Get returns collections.ErrNotFound only for missing keys, while other errors (e.g., decoding failures, storage corruption) propagate distinct values. Handling all errors identically masks data integrity issues, potentially dropping in-flight callbacks or actors instead of surfacing the failure.

Match the standard pattern: return empty only when the key is missing, and propagate or panic on actual errors.

🛠 Proposed alignment for GetPacketCallback
 func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string {
 	v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence))
 	if err != nil {
-		return ""
+		if errors.Is(err, collections.ErrNotFound) {
+			return ""
+		}
+		panic(err)
 	}
 	return v
 }
🛠 Proposed alignment for GetPacketAckActor
 func (k Keeper) GetPacketAckActor(ctx sdk.Context, channel string, packetSequence uint64) (string, string) {
 	rawData, err := k.packetAckActors.Get(ctx, collections.Join(channel, packetSequence))
 	if err != nil {
-		return "", ""
+		if errors.Is(err, collections.ErrNotFound) {
+			return "", ""
+		}
+		panic(err)
 	}
 	// ... rest of function
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string {
store := ctx.KVStore(k.storeKey)
return string(store.Get(GetPacketCallbackKey(channel, packetSequence)))
v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence))
if err != nil {
return ""
}
return v
}
func (k Keeper) GetPacketCallback(ctx sdk.Context, channel string, packetSequence uint64) string {
v, err := k.packetCallbacks.Get(ctx, collections.Join(channel, packetSequence))
if err != nil {
if errors.Is(err, collections.ErrNotFound) {
return ""
}
panic(err)
}
return v
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@x/ibchooks/keeper/keeper.go` around lines 135 - 141, GetPacketCallback and
GetPacketAckActor are treating every collections.Get failure as a missing key,
which hides real read/decoding/storage errors. Update both methods to only
return an empty result when the error is collections.ErrNotFound, and otherwise
propagate or surface the unexpected error. Use the existing Keeper methods
GetPacketCallback and GetPacketAckActor plus the packetCallbacks/packetAckActors
collections lookups to apply the standard collections error handling pattern.


// DeletePacketCallback deletes the callback from storage once it has been processed
func (k Keeper) DeletePacketCallback(ctx sdk.Context, channel string, packetSequence uint64) {
store := ctx.KVStore(k.storeKey)
store.Delete(GetPacketCallbackKey(channel, packetSequence))
if err := k.packetCallbacks.Remove(ctx, collections.Join(channel, packetSequence)); err != nil {
panic(err)
}
}

// StorePacketAckActor stores which contract is allowed to send an ack for the packet
func (k Keeper) StorePacketAckActor(ctx sdk.Context, packet channeltypes.Packet, contract string) {
store := ctx.KVStore(k.storeKey)
channel := packet.GetSourceChannel()
packetSequence := packet.GetSequence()

val, err := GeneratePacketAckValue(packet, contract)
if err != nil {
panic(err)
}
store.Set(GetPacketAckKey(channel, packetSequence), val)
if err := k.packetAckActors.Set(ctx, collections.Join(packet.GetSourceChannel(), packet.GetSequence()), val); err != nil {
panic(err)
}
}

// GetPacketAckActor returns the bech32 addr of the contract that is allowed to send an ack for the packet and the packet hash
func (k Keeper) GetPacketAckActor(ctx sdk.Context, channel string, packetSequence uint64) (string, string) {
store := ctx.KVStore(k.storeKey)
rawData := store.Get(GetPacketAckKey(channel, packetSequence))
rawData, err := k.packetAckActors.Get(ctx, collections.Join(channel, packetSequence))
if err != nil {
return "", ""
}
if rawData == nil {
return "", ""
}
Expand All @@ -162,8 +185,9 @@ func (k Keeper) GetPacketAckActor(ctx sdk.Context, channel string, packetSequenc

// DeletePacketAckActor deletes the ack actor from storage once it has been used
func (k Keeper) DeletePacketAckActor(ctx sdk.Context, channel string, packetSequence uint64) {
store := ctx.KVStore(k.storeKey)
store.Delete(GetPacketAckKey(channel, packetSequence))
if err := k.packetAckActors.Remove(ctx, collections.Join(channel, packetSequence)); err != nil {
panic(err)
}
}

// DeriveIntermediateSender derives the sender address to be used when calling wasm hooks
Expand Down
83 changes: 83 additions & 0 deletions x/ibchooks/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package keeper

import (
"strconv"
"strings"

"cosmossdk.io/collections"

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

"github.com/provenance-io/provenance/x/ibchooks/types"
)

// Migrator handles in-place store migrations for the x/ibchooks module.
type Migrator struct {
keeper Keeper
}

// NewMigrator returns a Migrator for the x/ibchooks module.
func NewMigrator(k Keeper) Migrator {
return Migrator{keeper: k}
}

// Migrate1to2 re-keys legacy raw-string packet entries into the new collections:
// - "channel::seq" -> PacketCallbacks[(channel, seq)] (prefix 0x02)
// - "channel::seq::ack" -> PacketAckActors[(channel, seq)] (prefix 0x03)
//
// Params (0x01) is already byte-identical under the new collections.Item and needs no migration.
// Legacy entries are ephemeral, so there are usually few (only in-flight packets) at upgrade time.
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
store := m.keeper.storeService.OpenKVStore(ctx)
it, err := store.Iterator(nil, nil)
if err != nil {
return err
}

type entry struct{ key, val []byte }
var legacy []entry
for ; it.Valid(); it.Next() {
key := it.Key()
if len(key) == 1 && key[0] == types.ParamsKeyBz[0] {
continue
}
if len(key) > 0 && (key[0] == types.PacketCallbackKeyBz[0] || key[0] == types.PacketAckKeyBz[0]) {
continue
}
legacy = append(legacy,
entry{key: append([]byte(nil), key...), val: append([]byte(nil), it.Value()...)})
}
if cerr := it.Close(); cerr != nil {
return cerr
}

for _, e := range legacy {
parts := strings.Split(string(e.key), "::")
switch {
case len(parts) == 2:
seq, perr := strconv.ParseUint(parts[1], 10, 64)
if perr != nil {
continue
}
if err := m.keeper.packetCallbacks.Set(ctx, collections.Join(parts[0], seq), string(e.val)); err != nil {
return err
}
case len(parts) == 3 && parts[2] == "ack":
seq, perr := strconv.ParseUint(parts[1], 10, 64)
if perr != nil {
continue
}
if err := m.keeper.packetAckActors.Set(ctx, collections.Join(parts[0], seq), e.val); err != nil {
return err
}
default:
continue
}
if err := store.Delete(e.key); err != nil {
return err
}
}

ctx.Logger().Info("ibchooks 1->2 migration complete: re-keyed legacy packet entries", "count", len(legacy))
return nil
}
21 changes: 13 additions & 8 deletions x/ibchooks/keeper/params.go
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
package keeper

import (
"errors"

"cosmossdk.io/collections"

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

"github.com/provenance-io/provenance/x/ibchooks/types"
)

// GetParams returns the total set of the module's parameters.
func (k Keeper) GetParams(ctx sdk.Context) (params types.Params) {
store := ctx.KVStore(k.storeKey)
bz := store.Get(types.IbcHooksParamStoreKey)
if bz == nil {
return types.DefaultParams()
params, err := k.params.Get(ctx)
if err != nil {
if errors.Is(err, collections.ErrNotFound) {
return types.DefaultParams()
}
panic(err)
}
k.cdc.MustUnmarshal(bz, &params)
return params
}

// SetParams sets the module's parameters with the provided parameters.
func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
store := ctx.KVStore(k.storeKey)
bz := k.cdc.MustMarshal(&params)
store.Set(types.IbcHooksParamStoreKey, bz)
if err := k.params.Set(ctx, params); err != nil {
panic(err)
}
}
6 changes: 5 additions & 1 deletion x/ibchooks/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ func (AppModule) Name() string {
func (am AppModule) RegisterServices(cfg module.Configurator) {
types.RegisterMsgServer(cfg.MsgServer(), keeper.NewMsgServerImpl(am.keeper))
types.RegisterQueryServer(cfg.QueryServer(), am.keeper)
migrator := keeper.NewMigrator(am.keeper)
if err := cfg.RegisterMigration(types.ModuleName, 1, migrator.Migrate1to2); err != nil {
panic(fmt.Sprintf("failed to register ibchooks migration 1->2: %v", err))
}
}

// InitGenesis performs genesis initialization for the ibchooks module. It returns
Expand Down Expand Up @@ -158,4 +162,4 @@ func (am AppModule) WeightedOperations(_ module.SimulationState) []simtypes.Weig
}

// ConsensusVersion implements AppModule/ConsensusVersion.
func (AppModule) ConsensusVersion() uint64 { return 1 }
func (AppModule) ConsensusVersion() uint64 { return 2 }
15 changes: 14 additions & 1 deletion x/ibchooks/types/keys.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package types

import "cosmossdk.io/collections"

const (
ModuleName = "ibchooks"
StoreKey = "hooks-for-ibc" // not using the module name because of collisions with key "ibc"
Expand All @@ -16,6 +18,17 @@ const (
)

var (

// Params keeps 0x01 (byte-identical to the legacy layout).
ParamsKeyBz = []byte{0x01}
PacketCallbackKeyBz = []byte{0x02}
PacketAckKeyBz = []byte{0x03}

// IbcHooksParamStoreKey key for ibchooks module's params
IbcHooksParamStoreKey = []byte{0x01}
IbcHooksParamStoreKey = ParamsKeyBz

// Collection prefixes.
ParamsKeyPrefix = collections.NewPrefix(ParamsKeyBz)
PacketCallbackKeyPrefix = collections.NewPrefix(PacketCallbackKeyBz)
PacketAckKeyPrefix = collections.NewPrefix(PacketAckKeyBz)
)
Loading