refactor: fund pool - ensure free slot

app/upgrades/v1_4/upgrade.go

@@ -198,7 +198,6 @@ func migrateFundersAndPools(
 		fundersKeeper.SetFundingState(ctx, &fundersTypes.FundingState{
 			PoolId:                oldPool.Id,
 			ActiveFunderAddresses: []string{},
-			TotalAmount:           oldPool.TotalFunds,
 		})
 
 		poolKeeper.SetPool(ctx, poolTypes.Pool{

x/funders/keeper/logic_funders.go

@@ -89,11 +89,11 @@ func (k Keeper) GetLowestFunding(fundings []types.Funding) (lowestFunding *types
 		return nil, fmt.Errorf("no active fundings")
 	}
 
-	lowestFunding = &fundings[0]
-	for _, funding := range fundings {
-		if funding.Amount < lowestFunding.Amount {
-			lowestFunding = &funding
+	lowestFundingIndex := 0
+	for i := range fundings {
+		if fundings[i].Amount < fundings[lowestFundingIndex].Amount {
+			lowestFundingIndex = i
 		}
 	}
-	return lowestFunding, nil
+	return &fundings[lowestFundingIndex], nil
 }

x/funders/keeper/msg_server_defund_pool.go

@@ -50,9 +50,6 @@ func (k msgServer) DefundPool(goCtx context.Context, msg *types.MsgDefundPool) (
 
 	// Save funding and funding state
 	k.SetFunding(ctx, &funding)
-	if funding.Amount == 0 {
-		fundingState.SetInactive(&funding)
-	}
 	k.SetFundingState(ctx, &fundingState)
 
 	// Emit a defund event.

x/funders/keeper/msg_server_fund_pool.go

@@ -10,14 +10,37 @@ import (
 	errorsTypes "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
-func (k msgServer) defundLowestFunding(
-	ctx sdk.Context,
-	lowestFunding *types.Funding,
-	fundingState *types.FundingState,
-	poolId uint64,
-) error {
-	err := util.TransferFromModuleToAddress(k.bankKeeper, ctx, types.ModuleName, lowestFunding.FunderAddress, lowestFunding.Amount)
-	if err != nil {
+// ensureFreeSlot makes sure that a funder can add funding to a given pool.
+// If this is not possible an appropriate error is returned.
+// A pool has a fixed amount of funding-slots. If there are still free slots
+// a funder can just join (even with the smallest funding possible).
+// If all slots are taken, it checks if the new funding has more funds
+// than the current lowest funding in that pool.
+// If so, the lowest funding gets removed from the pool, so that the
+// new funding can be added.
+// CONTRACT: no KV Writing on newFunding and fundingState
+func (k Keeper) ensureFreeSlot(ctx sdk.Context, newFunding *types.Funding, fundingState *types.FundingState) error {
+
+	activeFundings := k.GetActiveFundings(ctx, *fundingState)
+	// check if slots are still available
+	if len(activeFundings) < types.MaxFunders {
+		return nil
+	}
+
+	lowestFunding, _ := k.GetLowestFunding(activeFundings)
+
+	if lowestFunding.FunderAddress == newFunding.FunderAddress {
+		// Funder already has a funding slot
+		return nil
+	}
+
+	// Check if lowest funding is lower than new funding based on amount (amount per bundle is ignored)
+	if newFunding.Amount < lowestFunding.Amount {
+		return errors.Wrapf(errorsTypes.ErrLogic, types.ErrFundsTooLow.Error(), lowestFunding.Amount)
+	}
+
+	// Defund lowest funder
+	if err := util.TransferFromModuleToAddress(k.bankKeeper, ctx, types.ModuleName, lowestFunding.FunderAddress, lowestFunding.Amount); err != nil {
 		return err
 	}
 
@@ -27,10 +50,11 @@ func (k msgServer) defundLowestFunding(
 
 	// Emit a defund event.
 	_ = ctx.EventManager().EmitTypedEvent(&types.EventDefundPool{
-		PoolId:  poolId,
+		PoolId:  fundingState.PoolId,
 		Address: lowestFunding.FunderAddress,
 		Amount:  subtracted,
 	})
+
 	return nil
 }
 
@@ -49,8 +73,7 @@ func (k msgServer) FundPool(goCtx context.Context, msg *types.MsgFundPool) (*typ
 	}
 
 	// Pool has to exist
-	err := k.poolKeeper.AssertPoolExists(ctx, msg.PoolId)
-	if err != nil {
+	if err := k.poolKeeper.AssertPoolExists(ctx, msg.PoolId); err != nil {
 		return nil, err
 	}
 
@@ -80,6 +103,9 @@ func (k msgServer) FundPool(goCtx context.Context, msg *types.MsgFundPool) (*typ
 		}
 	}
 
+	// Check compatibility of updated funding with params
+	// i.e minimum funding per bundle
+	//     and minimum funding amount
 	params := k.GetParams(ctx)
 	if funding.AmountPerBundle < params.MinFundingAmountPerBundle {
 		return nil, errors.Wrapf(errorsTypes.ErrInvalidRequest, types.ErrAmountPerBundleTooLow.Error(), params.MinFundingAmountPerBundle)
@@ -88,48 +114,19 @@ func (k msgServer) FundPool(goCtx context.Context, msg *types.MsgFundPool) (*typ
 		return nil, errors.Wrapf(errorsTypes.ErrInvalidRequest, types.ErrMinFundingAmount.Error(), params.MinFundingAmount)
 	}
 
-	var defunding *types.Funding = nil
-
-	activeFundings := k.GetActiveFundings(ctx, fundingState)
-
-	// Check if funding limit is exceeded
-	if len(activeFundings) >= types.MaxFunders {
-		lowestFunding, err := k.GetLowestFunding(activeFundings)
-		if err != nil {
-			util.PanicHalt(k.upgradeKeeper, ctx, err.Error())
-		}
-
-		// Check if lowest funding is lower than new funding based on amount (amount per bundle is ignored)
-		if lowestFunding.Amount < funding.Amount {
-			// If so, check if lowest funding is from someone else
-			if lowestFunding.FunderAddress != funding.FunderAddress {
-				// Prepare to defund lowest funding
-				defunding = lowestFunding
-			}
-		} else {
-			return nil, errors.Wrapf(errorsTypes.ErrLogic, types.ErrFundsTooLow.Error(), lowestFunding.Amount)
-		}
+	// Kicks out lowest funder if all slots are taken and new funder is about to fund more.
+	// Otherwise, an error is thrown
+	// funding and fundingState are not written to the KV-Store. Everything else is handled safely.
+	if err := k.ensureFreeSlot(ctx, &funding, &fundingState); err != nil {
+		return nil, err
 	}
 
 	// User is allowed to fund
 	// Let's see if he has enough funds
 	if err := util.TransferFromAddressToModule(k.bankKeeper, ctx, msg.Creator, types.ModuleName, msg.Amount); err != nil {
-		// if err := util.TransferFromAddressToModule(k.bankKeeper, ctx, msg.Creator, "pool", msg.Amount); err != nil {
 		return nil, err
 	}
 
-	// Check if defunding is necessary
-	if defunding != nil {
-		err := k.defundLowestFunding(ctx, defunding, &fundingState, msg.PoolId)
-		if err != nil {
-			// TODO: should we panic here?
-			// This can only happen if:
-			// - we don't have enough funds which would be a corrupt state
-			// - the funder address is being blacklisted
-			util.PanicHalt(k.upgradeKeeper, ctx, err.Error())
-		}
-	}
-
 	// Funding must be active
 	fundingState.SetActive(&funding)