fix(market): close all losing bid escrows on lease creation

Author: Zblocker64

x/escrow/keeper/keeper.go

@@ -1091,11 +1091,15 @@ func (k *keeper) saveAccount(ctx sdk.Context, obj *account) error {
 	store.Set(key, k.cdc.MustMarshal(&obj.State))
 
 	if obj.State.State == etypes.StateClosed || obj.State.State == etypes.StateOverdrawn {
-		// call hooks
-		for _, hook := range k.hooks.onAccountClosed {
-			err := hook(ctx, obj.Account)
-			if err != nil {
-				return err
+		// OnEscrowAccountClosed is deployment-only (deployment teardown). Bid and other scopes
+		// must not invoke it — DeploymentIDFromEscrowID rejects non-deployment IDs and would
+		// fail AccountClose during e.g. CreateLease loser processing.
+		if obj.ID.Scope == escrowid.ScopeDeployment {
+			for _, hook := range k.hooks.onAccountClosed {
+				err := hook(ctx, obj.Account)
+				if err != nil {
+					return err
+				}
 			}
 		}
 	}

x/escrow/keeper/keeper_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
+	escrowid "pkg.akt.dev/go/node/escrow/id/v1"
 	"pkg.akt.dev/go/node/escrow/module"
 	etypes "pkg.akt.dev/go/node/escrow/types/v1"
 	"pkg.akt.dev/go/testutil"
@@ -446,3 +447,80 @@ func Test_PaymentCreate_later(t *testing.T) {
 		require.Equal(t, ctx.BlockHeight()-1, acct.State.SettledAt)
 	}
 }
+
+// Test_AccountCloseHooksRunOnlyForDeploymentEscrow guards the regression where bid escrow
+// AccountClose invoked the same onAccountClosed hooks as deployment escrows. The market
+// hook parses obj.ID as a deployment escrow id; ScopeBid ids made AccountClose fail and
+// broke CreateLease's loser loop. Hooks must run only for deployment-scoped accounts.
+func Test_AccountCloseHooksRunOnlyForDeploymentEscrow(t *testing.T) {
+	ssuite := state.SetupTestSuite(t)
+	ctx := ssuite.Context()
+	ekeeper := ssuite.EscrowKeeper()
+	bkeeper := ssuite.BankKeeper()
+
+	var hookScopes []escrowid.Scope
+	ekeeper.AddOnAccountClosedHook(func(_ sdk.Context, acc etypes.Account) error {
+		hookScopes = append(hookScopes, acc.ID.Scope)
+		return nil
+	})
+
+	mockBMEWithdrawals := func() {
+		bkeeper.
+			On("SendCoinsFromModuleToModule", mock.Anything, module.ModuleName, bmemodule.ModuleName, mock.Anything).
+			Return(nil).Maybe()
+		bkeeper.
+			On("MintCoins", mock.Anything, bmemodule.ModuleName, mock.Anything).
+			Return(nil).Maybe()
+		bkeeper.
+			On("BurnCoins", mock.Anything, bmemodule.ModuleName, mock.Anything).
+			Return(nil).Maybe()
+		bkeeper.
+			On("SendCoinsFromModuleToAccount", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+			Return(nil).Maybe()
+	}
+
+	bidID := testutil.BidID(t)
+	bidAid := bidID.ToEscrowAccountID()
+	require.Equal(t, escrowid.ScopeBid, bidAid.Scope)
+
+	bidOwner := testutil.AccAddress(t)
+	bidAmt := testutil.ACTCoin(t, 800)
+	ssuite.MockBMEForDeposit(bidOwner, bidAmt)
+	require.NoError(t, ekeeper.AccountCreate(ctx, bidAid, bidOwner, []etypes.Depositor{{
+		Owner:   bidOwner.String(),
+		Height:  ctx.BlockHeight(),
+		Balance: sdk.NewDecCoinFromCoin(bidAmt),
+	}}))
+
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 5)
+	mockBMEWithdrawals()
+	require.NoError(t, ekeeper.AccountClose(ctx, bidAid))
+	require.Empty(t, hookScopes, "no onAccountClosed hook should run for bid escrows")
+
+	bidAcct, err := ekeeper.GetAccount(ctx, bidAid)
+	require.NoError(t, err)
+	require.Equal(t, etypes.StateClosed, bidAcct.State.State)
+
+	hookScopes = nil
+	depAid := testutil.DeploymentID(t).ToEscrowAccountID()
+	require.Equal(t, escrowid.ScopeDeployment, depAid.Scope)
+
+	depOwner := testutil.AccAddress(t)
+	depAmt := testutil.ACTCoinRandom(t)
+	ssuite.MockBMEForDeposit(depOwner, depAmt)
+	require.NoError(t, ekeeper.AccountCreate(ctx, depAid, depOwner, []etypes.Depositor{{
+		Owner:   depOwner.String(),
+		Height:  ctx.BlockHeight(),
+		Balance: sdk.NewDecCoinFromCoin(depAmt),
+	}}))
+
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 5)
+	mockBMEWithdrawals()
+	require.NoError(t, ekeeper.AccountClose(ctx, depAid))
+	require.Equal(t, []escrowid.Scope{escrowid.ScopeDeployment}, hookScopes,
+		"registered hooks must run for deployment escrow closes")
+
+	depAcct, err := ekeeper.GetAccount(ctx, depAid)
+	require.NoError(t, err)
+	require.Equal(t, etypes.StateClosed, depAcct.State.State)
+}

x/market/handler/handler_test.go

@@ -1369,6 +1369,132 @@ func TestCloseBidUnknownOrder(t *testing.T) {
 	require.Error(t, err)
 }
 
+// Regression: CreateLease must mark every other open bid on the order lost, close each
+// loser's bid escrow, and never stop the loser loop early. Mainnet had BID_OPEN + open
+// bid escrow when deployment-only onAccountClosed hooks ran against ScopeBid accounts.
+func TestCreateLease_AllOtherBidsOnOrderMarkedLost(t *testing.T) {
+	for _, tc := range []struct {
+		name      string
+		numLosers int
+	}{
+		{name: "one_loser", numLosers: 1},
+		{name: "two_losers", numLosers: 2},
+		{name: "three_losers", numLosers: 3},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			defaultDeposit, err := dtypes.DefaultParams().MinDepositFor("uact")
+			require.NoError(t, err)
+
+			suite := setupTestSuite(t)
+			ctx := suite.Context()
+
+			deployment := testutil.Deployment(t)
+			group := testutil.DeploymentGroup(t, deployment.ID, 0)
+			group.GroupSpec.Resources = testutil.Resources(t, testutil.WithDenom("uact"))
+
+			owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner)
+
+			dmsg := &dtypes.MsgCreateDeployment{
+				ID:     deployment.ID,
+				Groups: dtypes.GroupSpecs{group.GroupSpec},
+				Deposit: deposit.Deposit{
+					Amount:  defaultDeposit,
+					Sources: deposit.Sources{deposit.SourceBalance},
+				},
+			}
+
+			suite.PrepareMocks(func(ts *state.TestSuite) {
+				ts.BankKeeper().
+					On("SendCoinsFromAccountToModule", mock.Anything, owner, emodule.ModuleName, sdk.NewCoins(dmsg.Deposit.Amount)).
+					Return(nil).Once()
+			})
+
+			res, err := suite.dhandler(ctx, dmsg)
+			require.NoError(t, err)
+			require.NotNil(t, res)
+
+			order, found := suite.MarketKeeper().GetOrder(ctx, mv1.OrderID{
+				Owner: deployment.ID.Owner,
+				DSeq:  deployment.ID.DSeq,
+				GSeq:  1,
+				OSeq:  1,
+			})
+			require.True(t, found)
+
+			attr := group.GroupSpec.Requirements.Attributes
+			totalBids := tc.numLosers + 1
+			addrs := make([]sdk.AccAddress, totalBids)
+			bmsgs := make([]*mvbeta.MsgCreateBid, totalBids)
+			for i := 0; i < totalBids; i++ {
+				prov := suite.createProvider(attr)
+				addr, err := sdk.AccAddressFromBech32(prov.Owner)
+				require.NoError(t, err)
+				addrs[i] = addr
+				bmsgs[i] = &mvbeta.MsgCreateBid{
+					ID:    mv1.MakeBidID(order.ID, addr),
+					Price: sdk.NewDecCoin(sdkutil.DenomUact, sdkmath.NewInt(int64(i+1))),
+					Deposit: deposit.Deposit{
+						Amount:  mvbeta.DefaultBidMinDepositACT,
+						Sources: deposit.Sources{deposit.SourceBalance},
+					},
+				}
+			}
+
+			suite.PrepareMocks(func(ts *state.TestSuite) {
+				for i := range bmsgs {
+					ts.MockBMEForDeposit(addrs[i], bmsgs[i].Deposit.Amount)
+				}
+			})
+
+			for _, bm := range bmsgs {
+				res, err := suite.handler(ctx, bm)
+				require.NoError(t, err)
+				require.NotNil(t, res)
+			}
+
+			winnerID := mv1.MakeBidID(order.ID, addrs[0])
+			loserIDs := make([]mv1.BidID, tc.numLosers)
+			for i := 0; i < tc.numLosers; i++ {
+				loserIDs[i] = mv1.MakeBidID(order.ID, addrs[i+1])
+			}
+
+			// Loser bid AccountClose settles deposit back to provider; bank hooks must be allowed.
+			suite.PrepareMocks(func(ts *state.TestSuite) {
+				ts.BankKeeper().
+					On("SendCoinsFromModuleToAccount", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+					Return(nil).Maybe().
+					On("SendCoinsFromModuleToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+					Return(nil).Maybe()
+			})
+
+			lmsg := &mvbeta.MsgCreateLease{BidID: winnerID}
+			leaseRes, err := suite.handler(ctx, lmsg)
+			require.NoError(t, err)
+			require.NotNil(t, leaseRes)
+
+			gotW, ok := suite.MarketKeeper().GetBid(ctx, winnerID)
+			require.True(t, ok)
+			require.Equal(t, mvbeta.BidActive, gotW.State)
+
+			winEscrow, err := suite.EscrowKeeper().GetAccount(ctx, winnerID.ToEscrowAccountID())
+			require.NoError(t, err)
+			require.Equal(t, etypes.StateOpen, winEscrow.State.State,
+				"winning bid escrow stays open for the lease")
+
+			for _, lid := range loserIDs {
+				got, ok := suite.MarketKeeper().GetBid(ctx, lid)
+				require.True(t, ok, "bid %v", lid)
+				require.Equal(t, mvbeta.BidLost, got.State, "bid %v", lid)
+
+				eacc, err := suite.EscrowKeeper().GetAccount(ctx, lid.ToEscrowAccountID())
+				require.NoError(t, err)
+				require.Equal(t, etypes.StateClosed, eacc.State.State,
+					"loser bid escrow must close (no deployment hook on ScopeBid)")
+			}
+		})
+	}
+}
+
 func (st *testSuite) createLease() (mv1.LeaseID, mvbeta.Bid, mvbeta.Order) {
 	st.t.Helper()
 	bid, order := st.createBid()

x/market/handler/server.go

@@ -225,11 +225,19 @@ func (ms msgServer) CreateLease(goCtx context.Context, msg *mvbeta.MsgCreateLeas
 	ms.keepers.Market.OnBidMatched(ctx, bid)
 
 	// close losing bids
+	//
+	// Every other BID_OPEN on this order must run OnBidLost. Do not stop the loop when
+	// AccountClose fails: returning true from the callback skips remaining bids, leaving them
+	// BID_OPEN while the winning lease is already committed (mainnet-visible bug).
 	ms.keepers.Market.WithBidsForOrder(ctx, msg.BidID.OrderID(), mvbeta.BidOpen, func(cbid mvbeta.Bid) bool {
 		ms.keepers.Market.OnBidLost(ctx, cbid)
 
-		if err = ms.keepers.Escrow.AccountClose(ctx, cbid.ID.ToEscrowAccountID()); err != nil {
-			return true
+		if closeErr := ms.keepers.Escrow.AccountClose(ctx, cbid.ID.ToEscrowAccountID()); closeErr != nil {
+			ctx.Logger().Error(
+				"escrow AccountClose failed for losing bid after lease created; bid already marked lost",
+				"bid", cbid.ID.String(),
+				"error", closeErr,
+			)
 		}
 		return false
 	})