From b9f801f507a1e0fcb3508136245e42a216a696b9 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 18 Dec 2024 16:17:47 +0100 Subject: [PATCH 1/5] refactor: use branch service in 29-fee --- modules/apps/29-fee/keeper/escrow.go | 43 +++++++++++++++------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 648475e09e0..2dfb263eec2 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -3,6 +3,7 @@ package keeper import ( "bytes" "context" + "errors" "fmt" errorsmod "cosmossdk.io/errors" @@ -11,6 +12,7 @@ import ( "github.com/cosmos/ibc-go/v9/modules/apps/29-fee/types" channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types" + ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors" ) // escrowPacketFee sends the packet fee to the 29-fee module account to hold in escrow @@ -48,38 +50,39 @@ func (k Keeper) escrowPacketFee(ctx context.Context, packetID channeltypes.Packe // DistributePacketFeesOnAcknowledgement pays all the acknowledgement & receive fees for a given packetID while refunding the timeout fees to the refund account. func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx context.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) { - // cache context before trying to distribute fees + // use branched multistore for distribution of fees. // if the escrow account has insufficient balance then we want to avoid partially distributing fees - sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/5917 - cacheCtx, writeFn := sdkCtx.CacheContext() + if err := k.BranchService.Execute(ctx, func(ctx context.Context) error { + // forward relayer address will be empty if conversion fails + forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer) - // forward relayer address will be empty if conversion fails - forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer) + for _, packetFee := range packetFees { + if !k.EscrowAccountHasBalance(ctx, packetFee.Fee.Total()) { + return ibcerrors.ErrInsufficientFunds + } - for _, packetFee := range packetFees { - if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) { + // check if refundAcc address works + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) + if err != nil { + panic(fmt.Errorf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) + } + + k.distributePacketFeeOnAcknowledgement(ctx, refundAddr, forwardAddr, reverseRelayer, packetFee) + } + + return nil + }); err != nil { + if errors.Is(err, ibcerrors.ErrInsufficientFunds) { // if the escrow account does not have sufficient funds then there must exist a severe bug // the fee module should be locked until manual intervention fixes the issue // a locked fee module will simply skip fee logic, all channels will temporarily function as // fee disabled channels - // NOTE: we use the uncached context to lock the fee module so that the state changes from - // locking the fee module are persisted + // NOTE: we lock the fee module on error return so that the state changes from are persisted k.lockFeeModule(ctx) return } - - // check if refundAcc address works - refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) - if err != nil { - panic(fmt.Errorf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) - } - - k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } - // write the cache - writeFn() - // removes the fees from the store as fees are now paid k.DeleteFeesInEscrow(ctx, packetID) } From fb67b7eb154162249cfc16df437f15d0586b9679 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 18 Dec 2024 16:20:22 +0100 Subject: [PATCH 2/5] chore: move comment for readability --- modules/apps/29-fee/keeper/escrow.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 2dfb263eec2..661b46926c5 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -58,6 +58,7 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx context.Context, forwa for _, packetFee := range packetFees { if !k.EscrowAccountHasBalance(ctx, packetFee.Fee.Total()) { + // NOTE: we lock the fee module on error return so that the state changes are persisted return ibcerrors.ErrInsufficientFunds } @@ -77,7 +78,6 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx context.Context, forwa // the fee module should be locked until manual intervention fixes the issue // a locked fee module will simply skip fee logic, all channels will temporarily function as // fee disabled channels - // NOTE: we lock the fee module on error return so that the state changes from are persisted k.lockFeeModule(ctx) return } From 62d34a3bd7d5de8e96c6fdc6bceb56d9dd8fe419 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 18 Dec 2024 16:28:51 +0100 Subject: [PATCH 3/5] refactor: use branch service in timeout fee distribution --- modules/apps/29-fee/keeper/escrow.go | 37 ++++++++++++++-------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 661b46926c5..b0712e2fbb7 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -109,35 +109,36 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx context.Context, refund // DistributePacketFeesOnTimeout pays all the timeout fees for a given packetID while refunding the acknowledgement & receive fees to the refund account. func (k Keeper) DistributePacketFeesOnTimeout(ctx context.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) { - // cache context before trying to distribute fees + // use branched multistore for distribution of fees. // if the escrow account has insufficient balance then we want to avoid partially distributing fees - sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/5917 - cacheCtx, writeFn := sdkCtx.CacheContext() + if err := k.BranchService.Execute(ctx, func(ctx context.Context) error { + for _, packetFee := range packetFees { + if !k.EscrowAccountHasBalance(ctx, packetFee.Fee.Total()) { + // NOTE: we lock the fee module on error return so that the state changes are persisted + return ibcerrors.ErrInsufficientFunds + } + + // check if refundAcc address works + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) + if err != nil { + panic(fmt.Errorf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) + } + + k.distributePacketFeeOnTimeout(ctx, refundAddr, timeoutRelayer, packetFee) + } - for _, packetFee := range packetFees { - if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) { + return nil + }); err != nil { + if errors.Is(err, ibcerrors.ErrInsufficientFunds) { // if the escrow account does not have sufficient funds then there must exist a severe bug // the fee module should be locked until manual intervention fixes the issue // a locked fee module will simply skip fee logic, all channels will temporarily function as // fee disabled channels - // NOTE: we use the uncached context to lock the fee module so that the state changes from - // locking the fee module are persisted k.lockFeeModule(ctx) return } - - // check if refundAcc address works - refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) - if err != nil { - panic(fmt.Errorf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) - } - - k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) } - // write the cache - writeFn() - // removing the fee from the store as the fee is now paid k.DeleteFeesInEscrow(ctx, packetID) } From 6441cfc25d76da3290642742f083fe4b81b0fc86 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 18 Dec 2024 17:01:48 +0100 Subject: [PATCH 4/5] refactor: complete refactor to branch service and remove cached context --- modules/apps/29-fee/keeper/escrow.go | 116 +++++++++++++-------------- modules/apps/29-fee/types/errors.go | 1 + 2 files changed, 59 insertions(+), 58 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index b0712e2fbb7..21ed95428a2 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -157,32 +157,32 @@ func (k Keeper) distributePacketFeeOnTimeout(ctx context.Context, refundAddr, ti // If the distribution fails for any reason (such as the receiving address being blocked), // the state changes will be discarded. func (k Keeper) distributeFee(ctx context.Context, receiver, refundAccAddress sdk.AccAddress, fee sdk.Coins) { - // cache context before trying to distribute fees - sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/7223 - cacheCtx, writeFn := sdkCtx.CacheContext() + // use branched multistore before trying to distribute fees + if err := k.BranchService.Execute(ctx, func(ctx context.Context) error { + err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, receiver, fee) + if err != nil { + if bytes.Equal(receiver, refundAccAddress) { + // if sending to the refund address already failed, then return (no-op) + return errorsmod.Wrapf(types.ErrRefundDistributionFailed, "receiver address: %s", receiver) + } - err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, receiver, fee) - if err != nil { - if bytes.Equal(receiver, refundAccAddress) { - k.Logger.Error("error distributing fee", "receiver address", receiver, "fee", fee) - return // if sending to the refund address already failed, then return (no-op) - } + // if an error is returned from x/bank and the receiver is not the refundAccAddress + // then attempt to refund the fee to the original sender + err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddress, fee) + if err != nil { + // if sending to the refund address fails, no-op + return errorsmod.Wrapf(types.ErrRefundDistributionFailed, "receiver address: %s", refundAccAddress) + } - // if an error is returned from x/bank and the receiver is not the refundAccAddress - // then attempt to refund the fee to the original sender - err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddress, fee) - if err != nil { - k.Logger.Error("error refunding fee to the original sender", "refund address", refundAccAddress, "fee", fee) - return // if sending to the refund address fails, no-op + emitDistributeFeeEvent(ctx, refundAccAddress.String(), fee) } - emitDistributeFeeEvent(ctx, refundAccAddress.String(), fee) - } else { - emitDistributeFeeEvent(ctx, receiver.String(), fee) + return nil + }); err != nil { + k.Logger.Error("error distributing fee", "error", err.Error()) } - // write the cache - writeFn() + emitDistributeFeeEvent(ctx, receiver.String(), fee) } // RefundFeesOnChannelClosure will refund all fees associated with the given port and channel identifiers. @@ -192,52 +192,52 @@ func (k Keeper) distributeFee(ctx context.Context, receiver, refundAccAddress sd func (k Keeper) RefundFeesOnChannelClosure(ctx context.Context, portID, channelID string) error { identifiedPacketFees := k.GetIdentifiedPacketFeesForChannel(ctx, portID, channelID) - // cache context before trying to distribute fees + // use branched multistore for distribution of fees. // if the escrow account has insufficient balance then we want to avoid partially distributing fees - sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/5917 - cacheCtx, writeFn := sdkCtx.CacheContext() - - for _, identifiedPacketFee := range identifiedPacketFees { - var unRefundedFees []types.PacketFee - for _, packetFee := range identifiedPacketFee.PacketFees { - - if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) { - // if the escrow account does not have sufficient funds then there must exist a severe bug - // the fee module should be locked until manual intervention fixes the issue - // a locked fee module will simply skip fee logic, all channels will temporarily function as - // fee disabled channels - // NOTE: we use the uncached context to lock the fee module so that the state changes from - // locking the fee module are persisted - k.lockFeeModule(ctx) - - // return a nil error so state changes are committed but distribution stops - return nil - } - - refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) - if err != nil { - unRefundedFees = append(unRefundedFees, packetFee) - continue + if err := k.BranchService.Execute(ctx, func(ctx context.Context) error { + for _, identifiedPacketFee := range identifiedPacketFees { + var unRefundedFees []types.PacketFee + for _, packetFee := range identifiedPacketFee.PacketFees { + + if !k.EscrowAccountHasBalance(ctx, packetFee.Fee.Total()) { + // NOTE: we lock the fee module on error return so that the state changes are persisted + return ibcerrors.ErrInsufficientFunds + } + + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) + if err != nil { + unRefundedFees = append(unRefundedFees, packetFee) + continue + } + + // refund all fees to refund address + if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAddr, packetFee.Fee.Total()); err != nil { + unRefundedFees = append(unRefundedFees, packetFee) + continue + } } - // refund all fees to refund address - if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAddr, packetFee.Fee.Total()); err != nil { - unRefundedFees = append(unRefundedFees, packetFee) - continue + if len(unRefundedFees) > 0 { + // update packet fees to keep only the unrefunded fees + packetFees := types.NewPacketFees(unRefundedFees) + k.SetFeesInEscrow(ctx, identifiedPacketFee.PacketId, packetFees) + } else { + k.DeleteFeesInEscrow(ctx, identifiedPacketFee.PacketId) } } - if len(unRefundedFees) > 0 { - // update packet fees to keep only the unrefunded fees - packetFees := types.NewPacketFees(unRefundedFees) - k.SetFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId, packetFees) - } else { - k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId) + return nil + }); err != nil { + if errors.Is(err, ibcerrors.ErrInsufficientFunds) { + // if the escrow account does not have sufficient funds then there must exist a severe bug + // the fee module should be locked until manual intervention fixes the issue + // a locked fee module will simply skip fee logic, all channels will temporarily function as + // fee disabled channels + k.lockFeeModule(ctx) + + return nil // commit state changes to lock module and stop fee distribution } } - // write the cache - writeFn() - return nil } diff --git a/modules/apps/29-fee/types/errors.go b/modules/apps/29-fee/types/errors.go index 22dd35000a2..9d3980a3369 100644 --- a/modules/apps/29-fee/types/errors.go +++ b/modules/apps/29-fee/types/errors.go @@ -17,4 +17,5 @@ var ( ErrRelayerNotFoundForAsyncAck = errorsmod.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement") ErrFeeModuleLocked = errorsmod.Register(ModuleName, 11, "the fee module is currently locked, a severe bug has been detected") ErrUnsupportedAction = errorsmod.Register(ModuleName, 12, "unsupported action") + ErrRefundDistributionFailed = errorsmod.Register(ModuleName, 13, "refund distrubition failed") ) From 342bfc5228c428029814e33698593a9e0f2fd017 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 18 Dec 2024 17:06:29 +0100 Subject: [PATCH 5/5] chore: make lint-fix --- modules/apps/29-fee/types/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/29-fee/types/errors.go b/modules/apps/29-fee/types/errors.go index 9d3980a3369..f38dedbfa2b 100644 --- a/modules/apps/29-fee/types/errors.go +++ b/modules/apps/29-fee/types/errors.go @@ -17,5 +17,5 @@ var ( ErrRelayerNotFoundForAsyncAck = errorsmod.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement") ErrFeeModuleLocked = errorsmod.Register(ModuleName, 11, "the fee module is currently locked, a severe bug has been detected") ErrUnsupportedAction = errorsmod.Register(ModuleName, 12, "unsupported action") - ErrRefundDistributionFailed = errorsmod.Register(ModuleName, 13, "refund distrubition failed") + ErrRefundDistributionFailed = errorsmod.Register(ModuleName, 13, "refund distribution failed") )