From b5449068e9ebf5d9671b18a405c10e7c93f43efa Mon Sep 17 00:00:00 2001 From: Michael Tsitrin <114929630+mtsitrin@users.noreply.github.com> Date: Tue, 17 Dec 2024 18:23:12 +0200 Subject: [PATCH] fix(ibc): remove denom registration fees. validate OnRecvPacket initially (#1660) --- app/keepers/keepers.go | 2 +- x/delayedack/ibc_middleware.go | 16 +++++++++++++++- x/delayedack/keeper/finalize.go | 3 +-- x/denommetadata/ibc_middleware.go | 19 +------------------ x/denommetadata/ibc_middleware_test.go | 20 ++------------------ x/denommetadata/types/expected_keepers.go | 5 ----- 6 files changed, 20 insertions(+), 45 deletions(-) diff --git a/app/keepers/keepers.go b/app/keepers/keepers.go index 7900f5559..4afd30ba2 100644 --- a/app/keepers/keepers.go +++ b/app/keepers/keepers.go @@ -555,7 +555,7 @@ func (a *AppKeepers) InitTransferStack() { packetforwardkeeper.DefaultRefundTransferPacketTimeoutTimestamp, ) - a.TransferStack = denommetadatamodule.NewIBCModule(a.TransferStack, a.DenomMetadataKeeper, a.RollappKeeper, a.TxFeesKeeper) + a.TransferStack = denommetadatamodule.NewIBCModule(a.TransferStack, a.DenomMetadataKeeper, a.RollappKeeper) // already instantiated in SetupHooks() a.delayedAckMiddleware.Setup( delayedackmodule.WithIBCModule(a.TransferStack), diff --git a/x/delayedack/ibc_middleware.go b/x/delayedack/ibc_middleware.go index 929af8eb7..8cd5a8f83 100644 --- a/x/delayedack/ibc_middleware.go +++ b/x/delayedack/ibc_middleware.go @@ -1,6 +1,8 @@ package delayedack import ( + "errors" + errorsmod "cosmossdk.io/errors" "github.com/cometbft/cometbft/libs/log" sdk "github.com/cosmos/cosmos-sdk/types" @@ -96,11 +98,23 @@ func (w IBCMiddleware) OnRecvPacket( return w.IBCModule.OnRecvPacket(ctx, packet, relayer) } + // Run the underlying app's OnRecvPacket callback + // with cache context to avoid state changes and report the receipt result. + // Only save the packet if the underlying app's callback succeeds. + cacheCtx, _ := ctx.CacheContext() + ack := w.IBCModule.OnRecvPacket(cacheCtx, packet, relayer) + if ack == nil { + return uevent.NewErrorAcknowledgement(ctx, errors.New("delayed ack is not supported by the underlying IBC module")) + } + if !ack.Success() { + return ack + } + rollappPacket := w.savePacket(ctx, packet, transfer, relayer, commontypes.RollappPacket_ON_RECV, nil) err = w.EIBCDemandOrderHandler(ctx, rollappPacket, transfer.FungibleTokenPacketData) if err != nil { - return uevent.NewErrorAcknowledgement(ctx, errorsmod.Wrap(err, "delayed ack")) + return uevent.NewErrorAcknowledgement(ctx, errorsmod.Wrap(err, "EIBC demand order handler")) } return nil diff --git a/x/delayedack/keeper/finalize.go b/x/delayedack/keeper/finalize.go index 06abc050c..5a61515ca 100644 --- a/x/delayedack/keeper/finalize.go +++ b/x/delayedack/keeper/finalize.go @@ -110,8 +110,7 @@ func (k Keeper) writeRecvAck(rollappPacket commontypes.RollappPacket, ack export to ensure the ack matches what the rollapp expects. */ rollappPacket = rollappPacket.RestoreOriginalTransferTarget() - err = k.WriteAcknowledgement(ctx, chanCap, rollappPacket.Packet, ack) - return + return k.WriteAcknowledgement(ctx, chanCap, rollappPacket.Packet, ack) } } diff --git a/x/denommetadata/ibc_middleware.go b/x/denommetadata/ibc_middleware.go index 7fbc63054..48f1516c9 100644 --- a/x/denommetadata/ibc_middleware.go +++ b/x/denommetadata/ibc_middleware.go @@ -15,12 +15,9 @@ import ( "github.com/dymensionxyz/sdk-utils/utils/uevent" "github.com/dymensionxyz/sdk-utils/utils/uibc" - commontypes "github.com/dymensionxyz/dymension/v3/x/common/types" "github.com/dymensionxyz/dymension/v3/x/denommetadata/types" ) -var RegistrationFeeAmt = commontypes.DYM.QuoRaw(10) // 0.1 DYM - var _ porttypes.IBCModule = &IBCModule{} // IBCModule implements the ICS26 callbacks for the transfer middleware @@ -28,7 +25,6 @@ type IBCModule struct { porttypes.IBCModule keeper types.DenomMetadataKeeper rollappKeeper types.RollappKeeper - txFeesKeeper types.TxFeesKeeper } // NewIBCModule creates a new IBCModule given the keepers and underlying application @@ -36,13 +32,11 @@ func NewIBCModule( app porttypes.IBCModule, keeper types.DenomMetadataKeeper, rollappKeeper types.RollappKeeper, - txFeesKeeper types.TxFeesKeeper, ) IBCModule { return IBCModule{ IBCModule: app, keeper: keeper, rollappKeeper: rollappKeeper, - txFeesKeeper: txFeesKeeper, } } @@ -93,22 +87,11 @@ func (im IBCModule) OnRecvPacket( return im.IBCModule.OnRecvPacket(ctx, packet, relayer) } - // charge denom metadata registration fee - baseDenom, err := im.txFeesKeeper.GetBaseDenom(ctx) - if err != nil { - return uevent.NewErrorAcknowledgement(ctx, err) - } - - registrationFee := sdk.NewCoin(baseDenom, RegistrationFeeAmt) - err = im.txFeesKeeper.ChargeFeesFromPayer(ctx, relayer, registrationFee, nil) - if err != nil { - return uevent.NewErrorAcknowledgement(ctx, err) - } - // adjust the denom metadata with the IBC denom dm.Base = ibcDenom dm.DenomUnits[0].Denom = dm.Base if err = im.keeper.CreateDenomMetadata(ctx, *dm); err != nil { + // TODO: remove? already checked above if errorsmod.IsOf(err, gerrc.ErrAlreadyExists) { return im.IBCModule.OnRecvPacket(ctx, packet, relayer) } diff --git a/x/denommetadata/ibc_middleware_test.go b/x/denommetadata/ibc_middleware_test.go index bd20618c7..c61f6bf78 100644 --- a/x/denommetadata/ibc_middleware_test.go +++ b/x/denommetadata/ibc_middleware_test.go @@ -29,7 +29,6 @@ func TestIBCModule_OnRecvPacket(t *testing.T) { name string keeper *mockDenomMetadataKeeper rollappKeeper *mockRollappKeeper - txFeesKeeper *mockTxFeeKeeper memoData *memoData wantAck exported.Acknowledgement @@ -110,7 +109,7 @@ func TestIBCModule_OnRecvPacket(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { app := &mockIBCModule{} - im := denommetadata.NewIBCModule(app, tt.keeper, tt.rollappKeeper, tt.txFeesKeeper) + im := denommetadata.NewIBCModule(app, tt.keeper, tt.rollappKeeper) var memo string if tt.memoData != nil { memo = mustMarshalJSON(tt.memoData) @@ -346,7 +345,6 @@ func TestIBCModule_OnAcknowledgementPacket(t *testing.T) { type fields struct { IBCModule porttypes.IBCModule rollappKeeper *mockRollappKeeper - txFeesKeeper *mockTxFeeKeeper } type args struct { packetData *transfertypes.FungibleTokenPacketData @@ -526,7 +524,7 @@ func TestIBCModule_OnAcknowledgementPacket(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - m := denommetadata.NewIBCModule(tt.fields.IBCModule, nil, tt.fields.rollappKeeper, tt.fields.txFeesKeeper) + m := denommetadata.NewIBCModule(tt.fields.IBCModule, nil, tt.fields.rollappKeeper) packet := channeltypes.Packet{} @@ -747,17 +745,3 @@ func (m mockBankKeeper) SetDenomMetaData(ctx sdk.Context, denomMetaData banktype func (m mockBankKeeper) GetDenomMetaData(sdk.Context, string) (banktypes.Metadata, bool) { return m.returnMetadata, m.returnMetadata.Base != "" } - -type mockTxFeeKeeper struct { - err error -} - -// ChargeFeesFromPayer implements types.TxFeesKeeper. -func (m *mockTxFeeKeeper) ChargeFeesFromPayer(ctx sdk.Context, payer sdk.AccAddress, takerFeeCoin sdk.Coin, beneficiary *sdk.AccAddress) error { - return m.err -} - -// GetBaseDenom implements types.TxFeesKeeper. -func (m *mockTxFeeKeeper) GetBaseDenom(ctx sdk.Context) (denom string, err error) { - return "adym", nil -} diff --git a/x/denommetadata/types/expected_keepers.go b/x/denommetadata/types/expected_keepers.go index 7a5754e70..d8bb59f8d 100644 --- a/x/denommetadata/types/expected_keepers.go +++ b/x/denommetadata/types/expected_keepers.go @@ -29,8 +29,3 @@ type RollappKeeper interface { HasRegisteredDenom(ctx sdk.Context, rollappID, denom string) (bool, error) ClearRegisteredDenoms(ctx sdk.Context, rollappID string) error } - -type TxFeesKeeper interface { - GetBaseDenom(ctx sdk.Context) (denom string, err error) - ChargeFeesFromPayer(ctx sdk.Context, payer sdk.AccAddress, takerFeeCoin sdk.Coin, beneficiary *sdk.AccAddress) error -}