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

refactor: use branch service in 29-fee #7732

Merged
merged 7 commits into from
Dec 19, 2024
Merged

refactor: use branch service in 29-fee #7732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b9f801f
refactor: use branch service in 29-fee
damiannolan Dec 18, 2024
fb67b7e
chore: move comment for readability
damiannolan Dec 18, 2024
62d34a3
refactor: use branch service in timeout fee distribution
damiannolan Dec 18, 2024
6441cfc
refactor: complete refactor to branch service and remove cached context
damiannolan Dec 18, 2024
9805a6d
Merge branch 'main' into damian/fee-env-branch-service
damiannolan Dec 18, 2024
342bfc5
chore: make lint-fix
damiannolan Dec 18, 2024
8a15a79
Merge branch 'main' into damian/fee-env-branch-service
DimitrisJim Dec 19, 2024
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
204 changes: 104 additions & 100 deletions modules/apps/29-fee/keeper/escrow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
"bytes"
"context"
"errors"
"fmt"

errorsmod "cosmossdk.io/errors"
Expand All @@ -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
Expand Down Expand Up @@ -46,38 +48,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)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the concept here, but I have to admit I don't love the use of errors as control flow. It feels a lot like using exceptions as control flow and while it works, it is quite ugly. Another potential way to do this would be to not shadow the ctx variable so that you could write to state directly here and then return an error. Of course, that has some downsides too as it makes it more likely to accidentally use the wrong context inside the here (and it feels weird to write to state inside the branch callback).

Not sure which I prefer, they are both ugly to me :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Could another potential solution be to just do this loop before branching? We only really need it for the last line, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your rational in general. I don't really love the use of errors as control flow per se as well.

On the other hand this is an exceptional circumstance, and in most code flows we will likely not be needing partial state changes and such, so I would be fine to keep it like this.

I kinda like the idea of not shadowing the ctx var and seeing how that behaves - in theory I think it should(?) work. But I also think its less explicit than what we are doing here already and having clear and readable code is more preferable imo when dealing with funds.

Could another potential solution be to just do this loop before branching? We only really need it for the last line, right?

This could potentially be an opt also maybe. But I'd like to stray from spending too much time on this rn given how much this module is used and its lifespan going forward.

}

// 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))
}

// forward relayer address will be empty if conversion fails
forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer)
k.distributePacketFeeOnAcknowledgement(ctx, refundAddr, forwardAddr, reverseRelayer, 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.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)
}
Expand All @@ -104,35 +107,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
}

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.distributePacketFeeOnTimeout(ctx, refundAddr, timeoutRelayer, 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
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)
}
Expand All @@ -151,36 +155,36 @@ 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
if err := k.emitDistributeFeeEvent(ctx, refundAccAddress.String(), fee); err != nil {
panic(err)
}
}

if err := k.emitDistributeFeeEvent(ctx, refundAccAddress.String(), fee); err != nil {
panic(err)
}
} else {
if err := k.emitDistributeFeeEvent(ctx, receiver.String(), fee); err != nil {
panic(err)
}
return nil
}); err != nil {
k.Logger.Error("error distributing fee", "error", err.Error())
}

// write the cache
writeFn()
if err := k.emitDistributeFeeEvent(ctx, receiver.String(), fee); err != nil {
panic(err)
}
}

// RefundFeesOnChannelClosure will refund all fees associated with the given port and channel identifiers.
Expand All @@ -190,52 +194,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
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
}
}

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(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
}
1 change: 1 addition & 0 deletions modules/apps/29-fee/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 distribution failed")
)
Loading