Fee Middleware: Escrow logic #465
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,132 @@ | ||||||
| package keeper | ||||||
|
|
||||||
| import ( | ||||||
| "fmt" | ||||||
|
|
||||||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||||||
|
|
||||||
| "github.com/cosmos/ibc-go/modules/apps/29-fee/types" | ||||||
| channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||||||
| ) | ||||||
|
|
||||||
| // TODO: add optional relayers arr | ||||||
| func (k Keeper) EscrowPacketFee(ctx sdk.Context, refundAcc sdk.AccAddress, fee types.Fee, packetID channeltypes.PacketId) error { | ||||||
| // check if the refund account exists | ||||||
| hasRefundAcc := k.authKeeper.GetAccount(ctx, refundAcc) | ||||||
| if hasRefundAcc == nil { | ||||||
| return sdkerrors.Wrap(types.ErrRefundAccNotFound, fmt.Sprintf("Account with address: %s not found", refundAcc.String())) | ||||||
| } | ||||||
|
|
||||||
| fees := sdk.Coins{ | ||||||
| *fee.AckFee, *fee.ReceiveFee, *fee.TimeoutFee, | ||||||
| } | ||||||
|
|
||||||
| // check if refundAcc has balance for each fee | ||||||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| for _, f := range fees { | ||||||
| hasBalance := k.bankKeeper.HasBalance(ctx, refundAcc, f) | ||||||
| if !hasBalance { | ||||||
| return sdkerrors.Wrap(types.ErrBalanceNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. Should this error be insufficient balance? |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| for _, coin := range fees { | ||||||
| // escrow each fee with account module | ||||||
| if err := k.bankKeeper.SendCoinsFromAccountToModule( | ||||||
AdityaSripal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| ctx, refundAcc, types.ModuleName, sdk.Coins{coin}, | ||||||
| ); err != nil { | ||||||
| return err | ||||||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
| } | ||||||
|
There was a problem hiding this comment. We can be more capital efficient here. Really we just need to escrow the Since the packet will only go down one of those 2 paths. There's never a point where all 3 fees are paid out There was a problem hiding this comment. I was under the impression that each coin could be of a separate denom? There was a problem hiding this comment. That is correct. You can still calculate max. Suppose: Then you need to escrow: If packet gets acked, then you refund: If packet times out, you refund Same logic would hold for sdk.Coins fee There was a problem hiding this comment. You can check if there's a utility function in There was a problem hiding this comment. Okay, thanks, makes sense. There was a problem hiding this comment. I'd prefer to go with the simple approach and do this as a future optimization (like how we pushed consensus state pruning until after launch). It is additional complexity There was a problem hiding this comment. Agree with @colin-axner 👍 |
||||||
|
|
||||||
| // Store fee in state for reference later | ||||||
| // feeInEscrow/<refund-account>/<channel-id>/packet/<sequence-id>/ -> Fee (timeout, ack, onrecv) | ||||||
| k.SetFeeInEscrow(ctx, fee, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func (k Keeper) payFee(ctx sdk.Context, refundAcc, forwardRelayer, reverseRelayer sdk.AccAddress, packetID channeltypes.PacketId) error { | ||||||
| // check if there is a Fee in escrow for the given packetId | ||||||
| feeInEscrow, hasFee := k.GetFeeInEscrow(ctx, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| if !hasFee { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
|
There was a problem hiding this comment. Shouldn't the error indicate for the channel/sequence? |
||||||
| } | ||||||
|
|
||||||
| // get module accAddr | ||||||
| feeModuleAccAddr := k.authKeeper.GetModuleAddress(types.ModuleName) | ||||||
|
|
||||||
| // send ack fee to forward relayer | ||||||
| if feeInEscrow.AckFee != nil { | ||||||
| err := k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, forwardRelayer, sdk.Coins{*feeInEscrow.AckFee}) | ||||||
| if err != nil { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
| feeInEscrow.AckFee = nil | ||||||
| k.SetFeeInEscrow(ctx, feeInEscrow, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| } | ||||||
|
|
||||||
| // send ack fee to forward relayer | ||||||
| if feeInEscrow.ReceiveFee != nil { | ||||||
| err := k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, reverseRelayer, sdk.Coins{*feeInEscrow.AckFee}) | ||||||
| if err != nil { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
| feeInEscrow.ReceiveFee = nil | ||||||
| k.SetFeeInEscrow(ctx, feeInEscrow, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| } | ||||||
|
|
||||||
| // refund timeout fee to refundAddr | ||||||
| if feeInEscrow.TimeoutFee != nil { | ||||||
| err := k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, refundAcc, sdk.Coins{*feeInEscrow.AckFee}) | ||||||
| if err != nil { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
| // set fee as an empty struct (if we reach this point Fee is paid in full) | ||||||
| k.SetFeeInEscrow(ctx, types.Fee{}, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| } | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func (k Keeper) payFeeTimeout(ctx sdk.Context, refundAcc, forwardRelayer, reverseRelayer sdk.AccAddress, packetID channeltypes.PacketId) error { | ||||||
| // check if there is a Fee in escrow for the given packetId | ||||||
| feeInEscrow, hasFee := k.GetFeeInEscrow(ctx, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| if !hasFee { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
|
|
||||||
| // get module accAddr | ||||||
| feeModuleAccAddr := k.authKeeper.GetModuleAddress(types.ModuleName) | ||||||
|
|
||||||
| // send ack fee to forward relayer | ||||||
| if feeInEscrow.AckFee != nil { | ||||||
| err := k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, refundAcc, sdk.Coins{*feeInEscrow.AckFee}) | ||||||
| if err != nil { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
| feeInEscrow.AckFee = nil | ||||||
| k.SetFeeInEscrow(ctx, feeInEscrow, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
|
There was a problem hiding this comment. @AdityaSripal Same issue here when we return an error on SendCoin. We need to keep track of the state of the Fee (say in the case where AckFee is sent but the TimeoutFee fails, we don't want to lose track of the fact that AckFee was escrowed). I think the solution is to only do the send here in the case that all are successful. I was thinking we could use the cacheContext similar to how we are doing this with interchain accounts? There was a problem hiding this comment. checkout What we did in ibc-go was because we wanted the tx to pass (and thus commit IBC state) while reverting any changes in callbacks. That's not necessary here. You already get atomicity out of the box with SDK There was a problem hiding this comment. Ahhh very good. That makes things much simpler. Thank you 🤝 |
||||||
| } | ||||||
|
|
||||||
| // send ack fee to forward relayer | ||||||
| if feeInEscrow.ReceiveFee != nil { | ||||||
| err := k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, refundAcc, sdk.Coins{*feeInEscrow.AckFee}) | ||||||
| if err != nil { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
| feeInEscrow.ReceiveFee = nil | ||||||
| k.SetFeeInEscrow(ctx, feeInEscrow, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| } | ||||||
|
|
||||||
| // refund timeout fee to refundAddr | ||||||
| if feeInEscrow.TimeoutFee != nil { | ||||||
| err := k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, forwardRelayer, sdk.Coins{*feeInEscrow.AckFee}) | ||||||
| if err != nil { | ||||||
| return sdkerrors.Wrap(types.ErrFeeNotFound, fmt.Sprintf("%s", refundAcc.String())) | ||||||
| } | ||||||
| // set fee as an empty struct (if we reach this point Fee is paid in full) | ||||||
| k.SetFeeInEscrow(ctx, types.Fee{}, refundAcc.String(), packetID.ChannelId, packetID.Sequence) | ||||||
| } | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package keeper_test | ||
|
|
||
| import ( | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/cosmos/ibc-go/modules/apps/29-fee/types" | ||
| channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||
seantking marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
|
|
||
| func (suite *KeeperTestSuite) TestEscrowPacketFee() { | ||
| var ( | ||
| err error | ||
| ) | ||
|
|
||
| testCases := []struct { | ||
AdityaSripal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| name string | ||
| malleate func() | ||
| expPass bool | ||
| }{ | ||
| { | ||
| "success", func() {}, true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
|
|
||
| suite.Run(tc.name, func() { | ||
| suite.SetupTest() // reset | ||
|
|
||
| // setup | ||
| refundAcc := suite.chainA.SenderAccount.GetAddress() | ||
| ackFee := &sdk.Coin{Denom: "stake", Amount: sdk.NewInt(100)} | ||
| recieveFee := &sdk.Coin{Denom: "stake", Amount: sdk.NewInt(100)} | ||
| timeoutFee := &sdk.Coin{Denom: "stake", Amount: sdk.NewInt(100)} | ||
| fee := types.Fee{ackFee, recieveFee, timeoutFee} | ||
| packetId := channeltypes.PacketId{ChannelId: "channel-0", PortId: "fee", Sequence: uint64(1)} | ||
|
|
||
| tc.malleate() // explicitly change fields in channel and testChannel | ||
|
|
||
| // escrow the packet fee | ||
| err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), refundAcc, fee, packetId) | ||
|
|
||
| if tc.expPass { | ||
| suite.Require().NoError(err) | ||
| } else { | ||
| suite.Require().Error(err) | ||
| } | ||
|
|
||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if %s is used, .String() should automatically be called (unless I am mistaken?)