Address review feedback

cmd/soroban-rpc/internal/ledgerentry_storage/db.go

@@ -143,12 +143,11 @@ func (s *sqlDB) GetLatestLedgerSequence() (uint32, error) {
 	if err != nil {
 		return 0, err
 	}
+	// Since it's a read-only transaction, we don't
+	// care whether we commit it or roll it back as long as we close it
+	defer tx.Rollback()
 	ret, err := getLatestLedgerSequence(tx)
 	if err != nil {
-		_ = tx.Rollback()
-		return 0, err
-	}
-	if err := tx.Commit(); err != nil {
 		return 0, err
 	}
 	return ret, nil
@@ -162,24 +161,19 @@ func (s *sqlDB) GetLedgerEntry(key xdr.LedgerKey) (xdr.LedgerEntry, bool, uint32
 	if err != nil {
 		return xdr.LedgerEntry{}, false, 0, err
 	}
+	// Since it's a read-only transaction, we don't
+	// care whether we commit it or roll it back as long as we close it
+	defer tx.Rollback()
 	seq, err := getLatestLedgerSequence(tx)
 	if err != nil {
-		_ = tx.Rollback()
 		return xdr.LedgerEntry{}, false, 0, err
 	}
 	buffer := xdr.NewEncodingBuffer()
 	entry, err := getLedgerEntry(tx, buffer, key)
 	if err == sql.ErrNoRows {
-		if err = tx.Commit(); err != nil {
-			return xdr.LedgerEntry{}, false, 0, err
-		}
 		return xdr.LedgerEntry{}, false, seq, nil
 	}
 	if err != nil {
-		_ = tx.Rollback()
-		return xdr.LedgerEntry{}, false, seq, err
-	}
-	if err := tx.Commit(); err != nil {
 		return xdr.LedgerEntry{}, false, seq, err
 	}
 	return entry, true, seq, nil
@@ -255,6 +249,7 @@ func (l *ledgerUpdaterTx) UpsertLedgerEntry(key xdr.LedgerKey, entry xdr.LedgerE
 	// safe since we cast to string right away
 	encodedEntry, err := l.buffer.UnsafeMarshalBinary(&entry)
 	if err != nil {
+		_ = l.tx.Rollback()
 		return err
 	}
 	encodedEntryStr := string(encodedEntry)
@@ -273,6 +268,7 @@ func (l *ledgerUpdaterTx) UpsertLedgerEntry(key xdr.LedgerKey, entry xdr.LedgerE
 func (l *ledgerUpdaterTx) DeleteLedgerEntry(key xdr.LedgerKey) error {
 	encodedKey, err := encodeLedgerKey(l.buffer, key)
 	if err != nil {
+		_ = l.tx.Rollback()
 		return err
 	}
 	l.keyToEntryBatch[encodedKey] = nil
@@ -299,7 +295,7 @@ func (l *ledgerUpdaterTx) Done() error {
 	if err := l.tx.Commit(); err != nil {
 		return err
 	}
-	if l.postWriteCommitHook() != nil {
+	if l.postWriteCommitHook != nil {
 		if err := l.postWriteCommitHook(); err != nil {
 			return err
 		}

cmd/soroban-rpc/internal/ledgerentry_storage/db_test.go

@@ -5,7 +5,6 @@ import (
 	"database/sql"
 	"fmt"
 	"math/rand"
-	"os"
 	"path"
 	"sync"
 	"testing"
@@ -17,10 +16,9 @@ import (
 )
 
 func TestGoldenPath(t *testing.T) {
-	db, dbPath := NewTestDB()
+	db := NewTestDB(t)
 	defer func() {
 		assert.NoError(t, db.Close())
-		assert.NoError(t, os.Remove(dbPath))
 	}()
 
 	// Check that we get an empty DB error
@@ -101,10 +99,9 @@ func TestGoldenPath(t *testing.T) {
 }
 
 func TestDeleteNonExistentLedgerEmpty(t *testing.T) {
-	db, dbPath := NewTestDB()
+	db := NewTestDB(t)
 	defer func() {
 		assert.NoError(t, db.Close())
-		assert.NoError(t, os.Remove(dbPath))
 	}()
 
 	// Simulate a ledger which creates and deletes a ledger entry
@@ -166,10 +163,9 @@ func getContractDataLedgerEntry(data xdr.ContractDataEntry) (xdr.LedgerKey, xdr.
 // Make sure that (multiple, simultaneous) read transactions can happen while a write-transaction is ongoing,
 // and write is only visible once the transaction is committed
 func TestReadTxsDuringWriteTx(t *testing.T) {
-	db, dbPath := NewTestDB()
+	db := NewTestDB(t)
 	defer func() {
 		assert.NoError(t, db.Close())
-		assert.NoError(t, os.Remove(dbPath))
 	}()
 
 	// Check that we get an empty DB error
@@ -237,10 +233,9 @@ func TestReadTxsDuringWriteTx(t *testing.T) {
 // Make sure that a write transaction can happen while multiple read transactions are ongoing,
 // and write is only visible once the transaction is committed
 func TestWriteTxsDuringReadTxs(t *testing.T) {
-	db, dbPath := NewTestDB()
+	db := NewTestDB(t)
 	defer func() {
 		assert.NoError(t, db.Close())
-		assert.NoError(t, os.Remove(dbPath))
 	}()
 
 	// Check that we get an empty DB error
@@ -320,16 +315,9 @@ func TestWriteTxsDuringReadTxs(t *testing.T) {
 
 // Check that we can have coexisting reader and writer goroutines without deadlocks or errors
 func TestConcurrentReadersAndWriter(t *testing.T) {
-	db, dbPath := NewTestDB()
+	db := NewTestDB(t)
 	defer func() {
-		err := db.Close()
-		if err != nil {
-			panic(err)
-		}
-		err = os.Remove(dbPath)
-		if err != nil {
-			panic(err)
-		}
+		assert.NoError(t, db.Close())
 	}()
 	contractID := xdr.Hash{0xca, 0xfe}
 	done := make(chan struct{})
@@ -410,16 +398,9 @@ func TestConcurrentReadersAndWriter(t *testing.T) {
 }
 
 func BenchmarkLedgerUpdate(b *testing.B) {
-	db, dbPath := NewTestDB()
+	db := NewTestDB(b)
 	defer func() {
-		err := db.Close()
-		if err != nil {
-			panic(err)
-		}
-		err = os.Remove(dbPath)
-		if err != nil {
-			panic(err)
-		}
+		assert.NoError(b, db.Close())
 	}()
 	keyUint32 := xdr.Uint32(0)
 	data := xdr.ContractDataEntry{
@@ -454,11 +435,12 @@ func BenchmarkLedgerUpdate(b *testing.B) {
 	b.StopTimer()
 }
 
-func NewTestDB() (DB, string) {
-	dbPath := path.Join(os.TempDir(), fmt.Sprintf("%08x.sqlite", rand.Int63()))
+func NewTestDB(tb testing.TB) DB {
+	tmp := tb.TempDir()
+	dbPath := path.Join(tmp, "db.sqlite")
 	db, err := OpenSQLiteDB(dbPath)
 	if err != nil {
 		panic(err)
 	}
-	return db, dbPath
+	return db
 }