accounts: implement account rename support in stores

accounts/interface.go

@@ -242,11 +242,11 @@ type Store interface {
 	// Accounts retrieves all accounts from the store and un-marshals them.
 	Accounts(ctx context.Context) ([]*OffChainBalanceAccount, error)
 
-	// UpdateAccountBalanceAndExpiry updates the balance and/or expiry of an
-	// account.
-	UpdateAccountBalanceAndExpiry(ctx context.Context, id AccountID,
+	// UpdateAccount updates the balance, expiry and/or label of an account.
+	UpdateAccount(ctx context.Context, id AccountID,
 		newBalance fn.Option[int64],
-		newExpiry fn.Option[time.Time]) error
+		newExpiry fn.Option[time.Time],
+		newLabel fn.Option[string]) error
 
 	// AddAccountInvoice adds an invoice hash to an account.
 	AddAccountInvoice(ctx context.Context, id AccountID,

accounts/rpcserver.go

@@ -122,8 +122,9 @@ func (s *RPCServer) CreateAccount(ctx context.Context,
 func (s *RPCServer) UpdateAccount(ctx context.Context,
 	req *litrpc.UpdateAccountRequest) (*litrpc.Account, error) {
 
-	log.Infof("[updateaccount] id=%s, label=%v, balance=%d, expiration=%d",
-		req.Id, req.Label, req.AccountBalance, req.ExpirationDate)
+	log.Infof("[updateaccount] id=%s, label=%v, balance=%d, "+
+		"expiration=%d, new_label=%v", req.Id, req.Label,
+		req.AccountBalance, req.ExpirationDate, req.NewLabel)
 
 	accountID, err := s.findAccount(ctx, req.Id, req.Label)
 	if err != nil {
@@ -133,7 +134,7 @@ func (s *RPCServer) UpdateAccount(ctx context.Context,
 	// Ask the service to update the account.
 	account, err := s.service.UpdateAccount(
 		ctx, accountID, btcutil.Amount(req.AccountBalance),
-		req.ExpirationDate,
+		req.ExpirationDate, req.NewLabel,
 	)
 	if err != nil {
 		return nil, err

accounts/service.go

@@ -301,7 +301,8 @@ func (s *InterceptorService) NewAccount(ctx context.Context,
 // if it exists.
 func (s *InterceptorService) UpdateAccount(ctx context.Context,
 	accountID AccountID, accountBalance btcutil.Amount,
-	expirationDate int64) (*OffChainBalanceAccount, error) {
+	expirationDate int64, newLabel string) (*OffChainBalanceAccount,
+	error) {
 
 	s.Lock()
 	defer s.Unlock()
@@ -334,9 +335,17 @@ func (s *InterceptorService) UpdateAccount(ctx context.Context,
 		balance = fn.Some(int64(accountBalance) * 1000)
 	}
 
+	// If a new label was provided, wrap it in an option. An empty
+	// string is treated as "no update requested" because protobuf
+	// cannot distinguish an absent field from the zero value "".
+	var label fn.Option[string]
+	if newLabel != "" {
+		label = fn.Some(newLabel)
+	}
+
 	// Create the actual account in the macaroon account store.
-	err := s.store.UpdateAccountBalanceAndExpiry(
-		ctx, accountID, balance, expiry,
+	err := s.store.UpdateAccount(
+		ctx, accountID, balance, expiry, label,
 	)
 	if err != nil {
 		return nil, fmt.Errorf("unable to update account: %w", err)

accounts/sql_migration_test.go

@@ -134,9 +134,10 @@ func TestAccountStoreMigration(t *testing.T) {
 				require.NoError(t, err)
 				require.False(t, acct1.HasExpired())
 
-				err = kvStore.UpdateAccountBalanceAndExpiry(
+				err = kvStore.UpdateAccount(
 					ctx, acct1.ID, fn.None[int64](),
 					fn.Some(time.Now().Add(time.Minute)),
+					fn.None[string](),
 				)
 				require.NoError(t, err)
 			},

accounts/store_kvdb.go

@@ -128,7 +128,6 @@ func (s *BoltStore) NewAccount(ctx context.Context, balance lnwire.MilliSatoshi,
 	}
 
 	if len(label) > 0 {
-
 		accounts, err := s.Accounts(ctx)
 		if err != nil {
 			return nil, fmt.Errorf("error checking label "+
@@ -181,13 +180,54 @@ func (s *BoltStore) NewAccount(ctx context.Context, balance lnwire.MilliSatoshi,
 	return account, nil
 }
 
-// UpdateAccountBalanceAndExpiry updates the balance and/or expiry of an
-// account.
+// UpdateAccount updates the balance and/or expiration date of an existing
+// off-chain account.
 //
 // NOTE: This is part of the Store interface.
-func (s *BoltStore) UpdateAccountBalanceAndExpiry(_ context.Context,
+func (s *BoltStore) UpdateAccount(ctx context.Context,
 	id AccountID, newBalance fn.Option[int64],
-	newExpiry fn.Option[time.Time]) error {
+	newExpiry fn.Option[time.Time],
+	newLabel fn.Option[string]) error {
+
+	// If a new label is set, we need to check for its uniqueness. Since the
+	// check requires a read transaction, we must do it before we start the
+	// update transaction.
+	var labelErr error
+	newLabel.WhenSome(func(label string) {
+		if err := checkLabel(label); err != nil {
+			labelErr = err
+			return
+		}
+
+		if len(label) > 0 {
+			accounts, err := s.Accounts(ctx)
+			if err != nil {
+				labelErr = fmt.Errorf("error checking label "+
+					"uniqueness: %w", err)
+				return
+			}
+
+			for _, other := range accounts {
+				if other.ID == id {
+					continue
+				}
+
+				if other.Label == label {
+					labelErr = fmt.Errorf(
+						"an account with the "+
+							"label '%s' "+
+							"already "+
+							"exists: %w",
+						label,
+						ErrLabelAlreadyExists)
+					return
+				}
+			}
+		}
+	})
+	if labelErr != nil {
+		return labelErr
+	}
 
 	update := func(account *OffChainBalanceAccount) error {
 		newBalance.WhenSome(func(balance int64) {
@@ -196,6 +236,9 @@ func (s *BoltStore) UpdateAccountBalanceAndExpiry(_ context.Context,
 		newExpiry.WhenSome(func(expiry time.Time) {
 			account.ExpirationDate = expiry
 		})
+		newLabel.WhenSome(func(label string) {
+			account.Label = label
+		})
 
 		return nil
 	}

accounts/store_sql.go

@@ -51,6 +51,7 @@ type SQLQueries interface {
 	// UpdateAccountAliasForTests is a query intended only for testing
 	// purposes, to change the account alias.
 	UpdateAccountAliasForTests(ctx context.Context, arg sqlc.UpdateAccountAliasForTestsParams) (int64, error)
+	UpdateAccountLabel(ctx context.Context, arg sqlc.UpdateAccountLabelParams) (int64, error)
 	UpsertAccountPayment(ctx context.Context, arg sqlc.UpsertAccountPaymentParams) error
 	GetAccountInvoice(ctx context.Context, arg sqlc.GetAccountInvoiceParams) (sqlc.AccountInvoice, error)
 }
@@ -108,7 +109,6 @@ func (s *SQLStore) NewAccount(ctx context.Context, balance lnwire.MilliSatoshi,
 
 	var labelVal sql.NullString
 	if len(label) > 0 {
-
 		labelVal = sql.NullString{
 			String: label,
 			Valid:  true,
@@ -335,13 +335,14 @@ func (s *SQLStore) markAccountUpdated(ctx context.Context,
 	return err
 }
 
-// UpdateAccountBalanceAndExpiry updates the balance and/or expiry of an
-// account.
+// UpdateAccount updates the balance and/or expiration date of an existing
+// off-chain account.
 //
 // NOTE: This is part of the Store interface.
-func (s *SQLStore) UpdateAccountBalanceAndExpiry(ctx context.Context,
+func (s *SQLStore) UpdateAccount(ctx context.Context,
 	alias AccountID, newBalance fn.Option[int64],
-	newExpiry fn.Option[time.Time]) error {
+	newExpiry fn.Option[time.Time],
+	newLabel fn.Option[string]) error {
 
 	var writeTxOpts db.QueriesTxOptions
 	return s.db.ExecTx(ctx, &writeTxOpts, func(db SQLQueries) error {
@@ -374,6 +375,52 @@ func (s *SQLStore) UpdateAccountBalanceAndExpiry(ctx context.Context,
 			return err
 		}
 
+		newLabel.WhenSome(func(label string) {
+			// First, ensure that if a label is set, it can't be
+			// mistaken for a hex encoded account ID.
+			if err = checkLabel(label); err != nil {
+				return
+			}
+
+			var labelVal sql.NullString
+			if len(label) > 0 {
+				labelVal = sql.NullString{
+					String: label,
+					Valid:  true,
+				}
+
+				// Check label uniqueness.
+				dbAcct, getErr := db.GetAccountByLabel(
+					ctx, labelVal,
+				)
+				if getErr == nil {
+					// If the label
+					// is already set for another
+					// account, then we return an error.
+					if dbAcct.ID != id {
+						err = ErrLabelAlreadyExists
+						return
+					}
+				} else if !errors.Is(getErr, sql.ErrNoRows) {
+					err = getErr
+					return
+				}
+			}
+
+			_, err = db.UpdateAccountLabel(
+				ctx, sqlc.UpdateAccountLabelParams{
+					ID: id,
+					Label: sql.NullString{
+						String: label,
+						Valid:  label != "",
+					},
+				},
+			)
+		})
+		if err != nil {
+			return err
+		}
+
 		return s.markAccountUpdated(ctx, db, id)
 	})
 }

accounts/store_test.go

@@ -40,13 +40,19 @@ func TestAccountStore(t *testing.T) {
 	_, err = store.NewAccount(ctx, 123, time.Time{}, "0011223344556677")
 	require.ErrorContains(t, err, "is not allowed as it can be mistaken")
 
+	// Make sure we can create an account with an empty label.
+	acctEmpty, err := store.NewAccount(ctx, 0, time.Time{}, "")
+	require.NoError(t, err)
+	require.Empty(t, acctEmpty.Label)
+
 	now := clock.Now()
 
 	// Update all values of the account that we can modify.
 	//
 	// Update the balance and expiry.
-	err = store.UpdateAccountBalanceAndExpiry(
+	err = store.UpdateAccount(
 		ctx, acct1.ID, fn.Some(int64(-500)), fn.Some(now),
+		fn.None[string](),
 	)
 	require.NoError(t, err)
 
@@ -129,14 +135,14 @@ func TestAccountStore(t *testing.T) {
 	// Test listing and deleting accounts.
 	accounts, err := store.Accounts(ctx)
 	require.NoError(t, err)
-	require.Len(t, accounts, 1)
+	require.Len(t, accounts, 2)
 
 	err = store.RemoveAccount(ctx, acct1.ID)
 	require.NoError(t, err)
 
 	accounts, err = store.Accounts(ctx)
 	require.NoError(t, err)
-	require.Len(t, accounts, 0)
+	require.Len(t, accounts, 1)
 
 	_, err = store.Account(ctx, acct1.ID)
 	require.ErrorIs(t, err, ErrAccNotFound)
@@ -277,15 +283,15 @@ func TestAccountUpdateMethods(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
 
-	t.Run("UpdateAccountBalanceAndExpiry", func(t *testing.T) {
+	t.Run("UpdateAccount", func(t *testing.T) {
 		clock := clock.NewTestClock(time.Now())
 		store := NewTestDB(t, clock)
 
 		// Ensure that the function errors out if we try update an
 		// account that does not exist.
-		err := store.UpdateAccountBalanceAndExpiry(
+		err := store.UpdateAccount(
 			ctx, AccountID{}, fn.None[int64](),
-			fn.None[time.Time](),
+			fn.None[time.Time](), fn.None[string](),
 		)
 		require.ErrorIs(t, err, ErrAccNotFound)
 
@@ -309,16 +315,18 @@ func TestAccountUpdateMethods(t *testing.T) {
 
 		// Now, update just the balance of the account.
 		newBalance := int64(123)
-		err = store.UpdateAccountBalanceAndExpiry(
+		err = store.UpdateAccount(
 			ctx, acct.ID, fn.Some(newBalance), fn.None[time.Time](),
+			fn.None[string](),
 		)
 		require.NoError(t, err)
 		assertBalanceAndExpiry(newBalance, time.Time{})
 
 		// Now update just the expiry of the account.
 		newExpiry := clock.Now().Add(time.Hour)
-		err = store.UpdateAccountBalanceAndExpiry(
+		err = store.UpdateAccount(
 			ctx, acct.ID, fn.None[int64](), fn.Some(newExpiry),
+			fn.None[string](),
 		)
 		require.NoError(t, err)
 		assertBalanceAndExpiry(newBalance, newExpiry)
@@ -326,19 +334,107 @@ func TestAccountUpdateMethods(t *testing.T) {
 		// Update both the balance and expiry of the account.
 		newBalance = 456
 		newExpiry = clock.Now().Add(2 * time.Hour)
-		err = store.UpdateAccountBalanceAndExpiry(
+		err = store.UpdateAccount(
 			ctx, acct.ID, fn.Some(newBalance), fn.Some(newExpiry),
+			fn.None[string](),
 		)
 		require.NoError(t, err)
 		assertBalanceAndExpiry(newBalance, newExpiry)
 
 		// Finally, test an update that has no net changes to the
 		// balance or expiry.
-		err = store.UpdateAccountBalanceAndExpiry(
+		err = store.UpdateAccount(
 			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.None[string](),
 		)
 		require.NoError(t, err)
 		assertBalanceAndExpiry(newBalance, newExpiry)
+
+		// Test renaming the account.
+		newLabel := "bar"
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some(newLabel),
+		)
+		require.NoError(t, err)
+
+		dbAcct, err := store.Account(ctx, acct.ID)
+		require.NoError(t, err)
+		require.Equal(t, newLabel, dbAcct.Label)
+
+		// Test updating an account with its existing label doesn't fail
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some(newLabel),
+		)
+		require.NoError(t, err)
+		require.Equal(t, newLabel, dbAcct.Label)
+
+		// Try to rename to an existing label.
+		_, err = store.NewAccount(ctx, 0, time.Time{}, "existing")
+		require.NoError(t, err)
+
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some("existing"),
+		)
+		require.ErrorIs(t, err, ErrLabelAlreadyExists)
+
+		// Test that passing an empty Some("") label works and clears
+		// the label.
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some(""),
+		)
+		require.NoError(t, err)
+		dbAcct, err = store.Account(ctx, acct.ID)
+		require.NoError(t, err)
+		require.Empty(t, dbAcct.Label)
+
+		// Test that passing a None label doesn't change anything.
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some("new-label"),
+		)
+		require.NoError(t, err)
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.None[string](),
+		)
+		require.NoError(t, err)
+		dbAcct, err = store.Account(ctx, acct.ID)
+		require.NoError(t, err)
+		require.Equal(t, "new-label", dbAcct.Label)
+
+		// Test that we cannot update to a label that looks like an
+		// account ID.
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some("0011223344556677"),
+		)
+		require.ErrorContains(t, err, "is not allowed"+
+			" as it can be mistaken")
+
+		// Test that a hex string with a different length is allowed.
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some("00112233445566"),
+		)
+		require.NoError(t, err)
+
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some("001122334455667788"),
+		)
+		require.NoError(t, err)
+
+		// Test that a non-hex string with the same length as an account
+		// ID is allowed.
+		err = store.UpdateAccount(
+			ctx, acct.ID, fn.None[int64](), fn.None[time.Time](),
+			fn.Some("G011223344556677"),
+		)
+		require.NoError(t, err)
 	})
 
 	t.Run("AddAccountInvoice", func(t *testing.T) {