chore(docs): improve code comments (#1644)

proto/dymensionxyz/dymension/eibc/demand_order.proto

@@ -36,6 +36,6 @@ message DemandOrder {
     common.RollappPacket.Type type = 10;
     // fulfiller_address is the bech32-encoded address of the account which fulfilled the order.
     string fulfiller_address = 11;
-    // creation_height is the height of the block when order was created.
+    // creation_height is the height of the block on the hub when order was created.
     uint64 creation_height = 12;
 }

proto/dymensionxyz/dymension/eibc/tx.proto

@@ -22,7 +22,7 @@ message MsgFulfillOrder {
     string fulfiller_address = 1;
     // order_id is the unique identifier of the order to be fulfilled.
     string order_id = 2;
-    // expected_fee is the nominal fee set in the order. 
+    // expected_fee is the nominal fee set in the order. Fulfiller will generally make less profit (after deducting bridge fee)
     string expected_fee = 3;
 }
 

x/bridgingfee/ibc_module.go

@@ -94,7 +94,8 @@ func (w IBCModule) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, rel
 	denom := denomutils.GetIncomingTransferDenom(packet, transfer.FungibleTokenPacketData)
 	feeCoin := sdk.NewCoin(denom, feeAmt)
 
-	// charge the bridging fee in cache context
+	// since transfer worked, then receiver should have enough balance to pay
+	// (unless param increased since the delayedck packet was created)
 	err = osmoutils.ApplyFuncIfNoError(ctx, func(ctx sdk.Context) error {
 		return w.txFeesKeeper.ChargeFeesFromPayer(ctx, receiver, feeCoin, nil)
 	})

x/common/types/key_rollapp_packet.go

@@ -41,6 +41,9 @@ func (p *RollappPacket) RollappPacketKey() []byte {
 		p.ProofHeight,
 		p.Type,
 		p.Packet.SourceChannel,
+
+		// Sequence makes it unique during normal operation, but hard fork rollback
+		// can mean it gets reused, so must delete orders on hard fork.
 		p.Packet.Sequence,
 	)
 }

x/common/types/rollapp_packet.go

@@ -110,6 +110,8 @@ func (r RollappPacket) GetAck() (channeltypes.Acknowledgement, error) {
 	return ack, nil
 }
 
+// restores the packet back to how it looked when hub first received it, to make sure the right ack
+// is written back
 func (r RollappPacket) RestoreOriginalTransferTarget() RollappPacket {
 	transferPacketData := r.MustGetTransferPacketData()
 	if r.OriginalTransferTarget != "" { // It can be empty if the eibc order was never fulfilled

x/delayedack/ibc_middleware.go

@@ -148,6 +148,7 @@ func (w IBCMiddleware) OnAcknowledgementPacket(
 	// Run the underlying app's OnAcknowledgementPacket callback
 	// with cache context to avoid state changes and report the acknowledgement result.
 	// Only save the packet if the underlying app's callback succeeds.
+	// NOTE: this is not an absolute guarantee that it will succeed when the packet is finalized
 	cacheCtx, _ := ctx.CacheContext()
 	err = w.IBCModule.OnAcknowledgementPacket(cacheCtx, packet, acknowledgement, relayer)
 	if err != nil {
@@ -186,6 +187,7 @@ func (w IBCMiddleware) OnTimeoutPacket(
 	// Run the underlying app's OnTimeoutPacket callback
 	// with cache context to avoid state changes and report the timeout result.
 	// Only save the packet if the underlying app's callback succeeds.
+	// NOTE: this is not an absolute guarantee that it will succeed when the packet is finalized
 	cacheCtx, _ := ctx.CacheContext()
 	err = w.IBCModule.OnTimeoutPacket(cacheCtx, packet, relayer)
 	if err != nil {

x/delayedack/keeper/finalize.go

@@ -13,21 +13,17 @@ import (
 	commontypes "github.com/dymensionxyz/dymension/v3/x/common/types"
 )
 
-// FinalizeRollappPacket finalizes a singe packet by its rollapp packet key.
 func (k Keeper) FinalizeRollappPacket(ctx sdk.Context, ibc porttypes.IBCModule, rollappPacketKey string) (*commontypes.RollappPacket, error) {
-	// Get a rollapp packet
 	packet, err := k.GetRollappPacket(ctx, rollappPacketKey)
 	if err != nil {
 		return nil, fmt.Errorf("get rollapp packet: %s: %w", rollappPacketKey, err)
 	}
 
-	// Verify the height is finalized
 	err = k.VerifyHeightFinalized(ctx, packet.RollappId, packet.ProofHeight)
 	if err != nil {
 		return packet, fmt.Errorf("verify height: rollapp '%s': %w", packet.RollappId, err)
 	}
 
-	// Finalize the packet
 	err = k.finalizeRollappPacket(ctx, ibc, packet.RollappId, *packet)
 	if err != nil {
 		return packet, fmt.Errorf("finalize rollapp packet: %w", err)
@@ -36,8 +32,10 @@ func (k Keeper) FinalizeRollappPacket(ctx sdk.Context, ibc porttypes.IBCModule,
 	return packet, nil
 }
 
+// used with osmo helper
 type wrappedFunc func(ctx sdk.Context) error
 
+// ibc = the next IBC module in the stack, to 'resume' the rest of the transfer flow
 func (k Keeper) finalizeRollappPacket(
 	ctx sdk.Context,
 	ibc porttypes.IBCModule,
@@ -55,7 +53,9 @@ func (k Keeper) finalizeRollappPacket(
 	var packetErr error
 	switch rollappPacket.Type {
 	case commontypes.RollappPacket_ON_RECV:
-		// TODO: makes more sense to modify the packet when calling the handler, instead storing in db "wrong" packet
+		// Because we intercepted the packet, the core ibc library wasn't able to write the ack when we first
+		// got the packet. So we try to write it here.
+
 		ack := ibc.OnRecvPacket(ctx, *rollappPacket.Packet, rollappPacket.Relayer)
 		/*
 				We only write the ack if writing it succeeds:
@@ -68,7 +68,7 @@ func (k Keeper) finalizeRollappPacket(
 						non-eibc: sender will get the funds back
 			            eibc:     effective transfer from fulfiller to original target
 		*/
-		if ack != nil {
+		if ack != nil { // NOTE: in practice ack should not be nil, since ibc transfer core module always returns something
 			packetErr = osmoutils.ApplyFuncIfNoError(ctx, k.writeRecvAck(rollappPacket, ack))
 		}
 	case commontypes.RollappPacket_ON_ACK:
@@ -78,8 +78,10 @@ func (k Keeper) finalizeRollappPacket(
 	default:
 		logger.Error("Unknown rollapp packet type")
 	}
-	// Update the packet with the error
 	if packetErr != nil {
+		// NOTE (timeout,ack): in regular (non dymension) IBC, timeout and ack errors are actually supposed
+		//  to cause the delivery transaction to be rejected.
+		//  Here, we already accepted the original msg delivery transaction, we can't retroactively reject it.
 		rollappPacket.Error = packetErr.Error()
 	}
 
@@ -109,6 +111,7 @@ func (k Keeper) writeRecvAck(rollappPacket commontypes.RollappPacket, ack export
 			Here, we do the inverse of what we did when we updated the packet transfer address, when we fulfilled the order
 			to ensure the ack matches what the rollapp expects.
 		*/
+		// TODO: makes more sense to modify the packet when calling the handler, instead storing in db "wrong" packet (big change)
 		rollappPacket = rollappPacket.RestoreOriginalTransferTarget()
 		return k.WriteAcknowledgement(ctx, chanCap, rollappPacket.Packet, ack)
 	}
@@ -132,6 +135,7 @@ func (k Keeper) onTimeoutPacket(rollappPacket commontypes.RollappPacket, ibc por
 }
 
 func (k Keeper) VerifyHeightFinalized(ctx sdk.Context, rollappID string, height uint64) error {
+	// TODO: can extract rollapp keeper IsHeightFinalized method
 	latestFinalizedHeight, err := k.getRollappLatestFinalizedHeight(ctx, rollappID)
 	if err != nil {
 		return err

x/delayedack/keeper/hooks.go

@@ -34,8 +34,8 @@ func (k Keeper) GetEIBCHooks() eibctypes.EIBCHooks {
 
 // AfterDemandOrderFulfilled is called every time a demand order is fulfilled.
 // Once it is fulfilled the underlying packet recipient should be updated to the fulfiller.
-func (k eibcHooks) AfterDemandOrderFulfilled(ctx sdk.Context, demandOrder *eibctypes.DemandOrder, fulfillerAddress string) error {
-	err := k.UpdateRollappPacketTransferAddress(ctx, demandOrder.TrackingPacketKey, fulfillerAddress)
+func (k eibcHooks) AfterDemandOrderFulfilled(ctx sdk.Context, demandOrder *eibctypes.DemandOrder, receiverAddr string) error {
+	err := k.UpdateRollappPacketTransferAddress(ctx, demandOrder.TrackingPacketKey, receiverAddr)
 	if err != nil {
 		return err
 	}

x/delayedack/keeper/msg_server.go

@@ -29,13 +29,14 @@ func NewMsgServer(k Keeper, ibc DelayedAckIBCModule) MsgServer {
 }
 
 func (m MsgServer) FinalizePacket(goCtx context.Context, msg *types.MsgFinalizePacket) (*types.MsgFinalizePacketResponse, error) {
-	err := msg.ValidateBasic()
+	err := msg.ValidateBasic() // TODO: remove, called by sdk
 	if err != nil {
 		return nil, err
 	}
 
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
+	// next middleware is denommetadata, see transfer stack setup
 	_, err = m.k.FinalizeRollappPacket(ctx, m.ibc.NextIBCMiddleware(), string(msg.PendingPacketKey()))
 	if err != nil {
 		return nil, err
@@ -57,7 +58,7 @@ func (m MsgServer) FinalizePacket(goCtx context.Context, msg *types.MsgFinalizeP
 }
 
 func (m MsgServer) FinalizePacketByPacketKey(goCtx context.Context, msg *types.MsgFinalizePacketByPacketKey) (*types.MsgFinalizePacketByPacketKeyResponse, error) {
-	err := msg.ValidateBasic()
+	err := msg.ValidateBasic() // TODO: remove, called by sdk
 	if err != nil {
 		return nil, err
 	}

x/delayedack/keeper/rollapp_packet.go

@@ -112,7 +112,7 @@ func (k Keeper) GetRollappPacket(ctx sdk.Context, rollappPacketKey string) (*com
 func (k Keeper) UpdateRollappPacketTransferAddress(
 	ctx sdk.Context,
 	rollappPacketKey string,
-	address string,
+	address string, // who will get the funds on finalization
 ) error {
 	rollappPacket, err := k.GetRollappPacket(ctx, rollappPacketKey)
 	if err != nil {
@@ -202,7 +202,6 @@ func (k *Keeper) UpdateRollappPacketAfterFinalization(ctx sdk.Context, rollappPa
 
 	newKey := rollappPacket.RollappPacketKey()
 
-	// Call hook subscribers
 	keeperHooks := k.GetHooks()
 	err = keeperHooks.AfterPacketStatusUpdated(ctx, &rollappPacket, string(oldKey), string(newKey))
 	if err != nil {

x/delayedack/keeper/transfer.go

@@ -35,6 +35,7 @@ func (k Keeper) GetValidTransferWithFinalizationInfo(
 		return
 	}
 
+	// TODO: can extract rollapp keeper IsHeightFinalized method
 	finalizedHeight, err := k.getRollappLatestFinalizedHeight(ctx, data.Rollapp.RollappId)
 	if errorsmod.IsOf(err, gerrc.ErrNotFound) {
 		err = nil

x/denommetadata/ibc_middleware.go

@@ -143,6 +143,7 @@ func (im IBCModule) OnAcknowledgementPacket(
 		return gerrc.ErrNotFound
 	}
 
+	// TODO: simplify: can do with just Set*
 	has, err := im.rollappKeeper.HasRegisteredDenom(ctx, rollapp.RollappId, dm.Base)
 	if err != nil {
 		return errorsmod.Wrapf(errortypes.ErrKeyNotFound, "check if rollapp has registered denom: %s", err.Error())
@@ -226,11 +227,11 @@ func (m *ICS4Wrapper) SendPacket(
 	// We need to handle both cases:
 	// 		1. We use the value of `packet.Denom` as the baseDenom
 	//		2. We parse the IBC denom trace into IBC denom hash and prepend it with "ibc/" to get the baseDenom
-	baseDenom := transfertypes.ParseDenomTrace(packet.Denom).IBCDenom()
+	baseDenom := transfertypes.ParseDenomTrace(packet.Denom).IBCDenom() // TODO: rename base denom to ibc denom https://github.com/dymensionxyz/dymension/issues/1650
 
 	has, err := m.rollappKeeper.HasRegisteredDenom(ctx, rollapp.RollappId, baseDenom)
 	if err != nil {
-		return 0, errorsmod.Wrapf(errortypes.ErrKeyNotFound, "check if rollapp has registered denom: %s", err.Error())
+		return 0, errorsmod.Wrapf(errortypes.ErrKeyNotFound, "check if rollapp has registered denom: %s", err.Error()) /// TODO: no .Error()
 	}
 	if has {
 		return m.ICS4Wrapper.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
@@ -244,12 +245,12 @@ func (m *ICS4Wrapper) SendPacket(
 
 	packet.Memo, err = types.AddDenomMetadataToMemo(packet.Memo, denomMetadata)
 	if err != nil {
-		return 0, errorsmod.Wrapf(gerrc.ErrInvalidArgument, "add denom metadata to memo: %s", err.Error())
+		return 0, errorsmod.Wrapf(gerrc.ErrInvalidArgument, "add denom metadata to memo: %s", err.Error()) /// TODO: no .Error()
 	}
 
 	data, err = types.ModuleCdc.MarshalJSON(packet)
 	if err != nil {
-		return 0, errorsmod.Wrapf(errortypes.ErrJSONMarshal, "marshal ICS-20 transfer packet data: %s", err.Error())
+		return 0, errorsmod.Wrapf(errortypes.ErrJSONMarshal, "marshal ICS-20 transfer packet data: %s", err.Error()) /// TODO: no .Error()
 	}
 
 	return m.ICS4Wrapper.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)

x/dymns/keeper/hooks.go

@@ -85,24 +85,12 @@ func (h rollappHooks) AfterTransfersEnabled(_ sdk.Context, _, _ string) error {
 	return nil
 }
 
-// FutureRollappHooks is temporary added to handle future hooks that not available yet.
 type FutureRollappHooks interface {
-	// OnRollAppIdChanged is called when a RollApp's ID is changed, typically due to fraud submission.
-	// It migrates all aliases and Dym-Names associated with the previous RollApp ID to the new one.
-	// This function executes step by step in a branched context to prevent side effects, and any errors
-	// during execution will result in the state changes being discarded.
-	//
-	// Parameters:
-	//   - ctx: The SDK context
-	//   - previousRollAppId: The original ID of the RollApp
-	//   - newRollAppId: The new ID assigned to the RollApp
+	// TODO: remove/deprecate - rollapp id cannot change
 	OnRollAppIdChanged(ctx sdk.Context, previousRollAppId, newRollAppId string)
-	// Just a pseudo method signature, the actual method signature might be different.
-
-	// TODO DymNS: connect to the actual implementation when the hooks are available.
-	//   The implementation of OnRollAppIdChanged assume that both of the RollApp records are exists in the x/rollapp store.
 }
 
+// TODO: Hooks should embed the noop base type, and only implement what they need, instead of repeating the whole interface.
 var _ FutureRollappHooks = rollappHooks{}
 
 func (k Keeper) GetFutureRollAppHooks() FutureRollappHooks {

x/eibc/keeper/hooks.go

@@ -35,19 +35,16 @@ func (k Keeper) GetDelayedAckHooks() delayeacktypes.DelayedAckHooks {
 func (d delayedAckHooks) AfterPacketStatusUpdated(ctx sdk.Context, packet *commontypes.RollappPacket,
 	oldPacketKey string, newPacketKey string,
 ) error {
-	// Get the demand order from the old packet key
 	demandOrderID := types.BuildDemandIDFromPacketKey(oldPacketKey)
 	demandOrder, err := d.GetDemandOrder(ctx, commontypes.Status_PENDING, demandOrderID)
 	if err != nil {
-		// If demand order does not exist, then we don't need to do anything
+		// If demand order does not exist, then we don't need to do anything // TODO: why
 		if errors.Is(err, types.ErrDemandOrderDoesNotExist) {
 			return nil
 		}
 		return err
 	}
-	// Update the demand order tracking packet key
 	demandOrder.TrackingPacketKey = newPacketKey
-	// Update the demand order status according to the underlying packet status
 	_, err = d.UpdateDemandOrderWithStatus(ctx, demandOrder, packet.Status)
 	if err != nil {
 		return err

x/eibc/keeper/keeper.go

@@ -115,10 +115,9 @@ func (k Keeper) SetOrderFulfilled(
 	if err != nil {
 		return err
 	}
-	// if the collector address is not nil, then the receiver address should be the collector address.
-	// Otherwise, the receiver address should be the fulfiller address.
 	receiverAddress := fulfillerAddress
 	if collectorAddress != nil {
+		// optional override
 		receiverAddress = collectorAddress
 	}
 	// Call hooks if fulfilled. This hook should be called only once per fulfillment.

x/eibc/keeper/msg_server.go

@@ -30,7 +30,7 @@ func (m msgServer) FulfillOrder(goCtx context.Context, msg *types.MsgFulfillOrde
 	ctx := sdk.UnwrapSDKContext(goCtx)
 	logger := ctx.Logger()
 
-	err := msg.ValidateBasic()
+	err := msg.ValidateBasic() // TODO: remove, sdk does this
 	if err != nil {
 		return nil, err
 	}
@@ -47,7 +47,6 @@ func (m msgServer) FulfillOrder(goCtx context.Context, msg *types.MsgFulfillOrde
 		return nil, types.ErrExpectedFeeNotMet
 	}
 
-	// Check that the fulfiller has enough balance to fulfill the order
 	fulfillerAccount := m.ak.GetAccount(ctx, msg.GetFulfillerBech32Address())
 	if fulfillerAccount == nil {
 		return nil, types.ErrFulfillerAddressDoesNotExist
@@ -76,7 +75,7 @@ func (m msgServer) FulfillOrderAuthorized(goCtx context.Context, msg *types.MsgF
 	ctx := sdk.UnwrapSDKContext(goCtx)
 	logger := ctx.Logger()
 
-	err := msg.ValidateBasic()
+	err := msg.ValidateBasic() // TODO: remove, sdk does this
 	if err != nil {
 		return nil, err
 	}
@@ -86,6 +85,7 @@ func (m msgServer) FulfillOrderAuthorized(goCtx context.Context, msg *types.MsgF
 		return nil, err
 	}
 
+	// check compat between the fulfillment and current order and packet status
 	if err := m.validateOrder(demandOrder, msg, ctx); err != nil {
 		return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, err.Error())
 	}
@@ -135,6 +135,7 @@ func (m msgServer) FulfillOrderAuthorized(goCtx context.Context, msg *types.MsgF
 	return &types.MsgFulfillOrderAuthorizedResponse{}, nil
 }
 
+// TODO: rename and fix signature (ctx first)
 func (m msgServer) validateOrder(demandOrder *types.DemandOrder, msg *types.MsgFulfillOrderAuthorized, ctx sdk.Context) error {
 	if demandOrder.RollappId != msg.RollappId {
 		return types.ErrRollappIdMismatch
@@ -170,6 +171,8 @@ func (m msgServer) checkIfSettlementValidated(ctx sdk.Context, demandOrder *type
 		return false, fmt.Errorf("get rollapp packet: %w", err)
 	}
 
+	// TODO: extract to rollapp keeper func HaveHeight(..)
+
 	// as it is not currently possible to make IBC transfers without a canonical client,
 	// we can assume that there has to exist at least one state info record for the rollapp
 	stateInfo, ok := m.rk.GetLatestStateInfo(ctx, demandOrder.RollappId)
@@ -179,18 +182,18 @@ func (m msgServer) checkIfSettlementValidated(ctx sdk.Context, demandOrder *type
 
 	lastHeight := stateInfo.GetLatestHeight()
 
+	// TODO: write oneliner
 	if lastHeight < raPacket.ProofHeight {
 		return false, nil
 	}
-
 	return true, nil
 }
 
 // UpdateDemandOrder implements types.MsgServer.
 func (m msgServer) UpdateDemandOrder(goCtx context.Context, msg *types.MsgUpdateDemandOrder) (*types.MsgUpdateDemandOrderResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	err := msg.ValidateBasic()
+	err := msg.ValidateBasic() // TODO: remove, sdk does this
 	if err != nil {
 		return nil, err
 	}
@@ -210,6 +213,7 @@ func (m msgServer) UpdateDemandOrder(goCtx context.Context, msg *types.MsgUpdate
 
 	raPacket, err := m.dack.GetRollappPacket(ctx, demandOrder.TrackingPacketKey)
 	if err != nil {
+		// TODO: isn't this internal error?
 		return nil, err
 	}
 

x/eibc/types/demand_order.go

@@ -164,7 +164,7 @@ func (m *DemandOrder) ValidateOrderIsOutstanding() error {
 		return ErrDemandAlreadyFulfilled
 	}
 	// Check the underlying packet is still relevant (i.e not expired, rejected, reverted)
-	if m.TrackingPacketStatus != commontypes.Status_PENDING {
+	if m.TrackingPacketStatus != commontypes.Status_PENDING { // TODO:remove, there is only one callsite and it already knows it's pending
 		return ErrDemandOrderInactive
 	}
 	return nil