Three code review suggestions (#4681)feature/avm-box

6 files changed, 73 insertions, 85 deletions

diff --git a/daemon/algod/api/server/v2/handlers.go b/daemon/algod/api/server/v2/handlers.go
index 04d268a02..067b213b9 100644
--- a/daemon/algod/api/server/v2/handlers.go
+++ b/daemon/algod/api/server/v2/handlers.go
@@ -1199,7 +1199,7 @@ func (v2 *Handlers) GetApplicationByID(ctx echo.Context, applicationID uint64) e
 func applicationBoxesMaxKeys(requestedMax uint64, algodMax uint64) uint64 {
 	if requestedMax == 0 {
 		if algodMax == 0 {
-			return 0 // unlimited results when both requested and algod max are 0
+			return math.MaxUint64 // unlimited results when both requested and algod max are 0
 		}
 		return algodMax + 1 // API limit dominates.  Increments by 1 to test if more than max supported results exist.
 	}
diff --git a/daemon/algod/api/server/v2/handlers_test.go b/daemon/algod/api/server/v2/handlers_test.go
index f68005fe4..98635897d 100644
--- a/daemon/algod/api/server/v2/handlers_test.go
+++ b/daemon/algod/api/server/v2/handlers_test.go
@@ -17,9 +17,11 @@
 package v2
 
 import (
+	"math"
+	"testing"
+
 	"github.com/algorand/go-algorand/test/partitiontest"
 	"github.com/stretchr/testify/require"
-	"testing"
 )
 
 func TestApplicationBoxesMaxKeys(t *testing.T) {
@@ -35,5 +37,5 @@ func TestApplicationBoxesMaxKeys(t *testing.T) {
 	require.Equal(t, uint64(2), applicationBoxesMaxKeys(0, 1))
 
 	// Response size _not_ limited
-	require.Equal(t, uint64(0), applicationBoxesMaxKeys(0, 0))
+	require.Equal(t, uint64(math.MaxUint64), applicationBoxesMaxKeys(0, 0))
 }
diff --git a/data/txntest/txn.go b/data/txntest/txn.go
index 0eb551b92..cab0ded5c 100644
--- a/data/txntest/txn.go
+++ b/data/txntest/txn.go
@@ -107,15 +107,44 @@ type Txn struct {
 	StateProofMsg  stateproofmsg.Message
 }
 
+// internalCopy "finishes" a shallow copy done by a simple Go assignment by
+// copying all of the slice fields
+func (tx *Txn) internalCopy() {
+	tx.Note = append([]byte(nil), tx.Note...)
+	if tx.ApplicationArgs != nil {
+		tx.ApplicationArgs = append([][]byte(nil), tx.ApplicationArgs...)
+		for i := range tx.ApplicationArgs {
+			tx.ApplicationArgs[i] = append([]byte(nil), tx.ApplicationArgs[i]...)
+		}
+	}
+	tx.Accounts = append([]basics.Address(nil), tx.Accounts...)
+	tx.ForeignApps = append([]basics.AppIndex(nil), tx.ForeignApps...)
+	tx.ForeignAssets = append([]basics.AssetIndex(nil), tx.ForeignAssets...)
+	tx.Boxes = append([]transactions.BoxRef(nil), tx.Boxes...)
+	for i := 0; i < len(tx.Boxes); i++ {
+		tx.Boxes[i].Name = append([]byte(nil), tx.Boxes[i].Name...)
+	}
+
+	// Programs may or may not actually be byte slices.  The other
+	// possibilitiues don't require copies.
+	if program, ok := tx.ApprovalProgram.([]byte); ok {
+		tx.ApprovalProgram = append([]byte(nil), program...)
+	}
+	if program, ok := tx.ClearStateProgram.([]byte); ok {
+		tx.ClearStateProgram = append([]byte(nil), program...)
+	}
+}
+
 // Noted returns a new Txn with the given note field.
-func (tx *Txn) Noted(note string) *Txn {
-	copy := *tx
-	copy.Note = []byte(note)
-	return &copy
+func (tx Txn) Noted(note string) *Txn {
+	tx.internalCopy()
+	tx.Note = []byte(note)
+	return &tx
 }
 
 // Args returns a new Txn with the given strings as app args
 func (tx Txn) Args(strings ...string) *Txn {
+	tx.internalCopy()
 	bytes := make([][]byte, len(strings))
 	for i, s := range strings {
 		bytes[i] = []byte(s)
diff --git a/ledger/accountdb.go b/ledger/accountdb.go
index 9018162e3..43d071106 100644
--- a/ledger/accountdb.go
+++ b/ledger/accountdb.go
@@ -46,14 +46,13 @@ import (
 // accountsDbQueries is used to cache a prepared SQL statement to look up
 // the state of a single account.
 type accountsDbQueries struct {
-	listCreatablesStmt                *sql.Stmt
-	lookupStmt                        *sql.Stmt
-	lookupResourcesStmt               *sql.Stmt
-	lookupAllResourcesStmt            *sql.Stmt
-	lookupKvPairStmt                  *sql.Stmt
-	lookupKeysByPrefixTypicalStmt     *sql.Stmt
-	lookupKeysByPrefixEmptyOrAllFStmt *sql.Stmt
-	lookupCreatorStmt                 *sql.Stmt
+	listCreatablesStmt     *sql.Stmt
+	lookupStmt             *sql.Stmt
+	lookupResourcesStmt    *sql.Stmt
+	lookupAllResourcesStmt *sql.Stmt
+	lookupKvPairStmt       *sql.Stmt
+	lookupKeysByRangeStmt  *sql.Stmt
+	lookupCreatorStmt      *sql.Stmt
 }
 
 type onlineAccountsDbQueries struct {
@@ -2605,12 +2604,7 @@ func accountsInitDbQueries(q db.Queryable) (*accountsDbQueries, error) {
 		return nil, err
 	}
 
-	qs.lookupKeysByPrefixTypicalStmt, err = q.Prepare("SELECT acctrounds.rnd, kvstore.key FROM acctrounds LEFT JOIN kvstore ON kvstore.key >= ? AND kvstore.key < ? WHERE id='acctbase'")
-	if err != nil {
-		return nil, err
-	}
-
-	qs.lookupKeysByPrefixEmptyOrAllFStmt, err = q.Prepare("SELECT acctrounds.rnd, kvstore.key FROM acctrounds LEFT JOIN kvstore ON kvstore.key >= ? WHERE id='acctbase'")
+	qs.lookupKeysByRangeStmt, err = q.Prepare("SELECT acctrounds.rnd, kvstore.key FROM acctrounds LEFT JOIN kvstore ON kvstore.key >= ? AND kvstore.key < ? WHERE id='acctbase'")
 	if err != nil {
 		return nil, err
 	}
@@ -2728,14 +2722,15 @@ func keyPrefixIntervalPreprocessing(prefix []byte) ([]byte, []byte) {
 }
 
 func (qs *accountsDbQueries) lookupKeysByPrefix(prefix string, maxKeyNum uint64, results map[string]bool, resultCount uint64) (round basics.Round, err error) {
+	start, end := keyPrefixIntervalPreprocessing([]byte(prefix))
+	if end == nil {
+		// Not an expected use case, it's asking for all keys, or all keys
+		// prefixed by some number of 0xFF bytes.
+		return 0, fmt.Errorf("Lookup by strange prefix %#v", prefix)
+	}
 	err = db.Retry(func() error {
-		prefixBytes, prefixIncrBytes := keyPrefixIntervalPreprocessing([]byte(prefix))
 		var rows *sql.Rows
-		if prefixIncrBytes != nil {
-			rows, err = qs.lookupKeysByPrefixTypicalStmt.Query(prefixBytes, prefixIncrBytes)
-		} else {
-			rows, err = qs.lookupKeysByPrefixEmptyOrAllFStmt.Query(prefixBytes)
-		}
+		rows, err = qs.lookupKeysByRangeStmt.Query(start, end)
 		if err != nil {
 			return err
 		}
@@ -2744,7 +2739,7 @@ func (qs *accountsDbQueries) lookupKeysByPrefix(prefix string, maxKeyNum uint64,
 		var v sql.NullString
 
 		for rows.Next() {
-			if maxKeyNum > 0 && resultCount == maxKeyNum {
+			if resultCount == maxKeyNum {
 				return nil
 			}
 			err = rows.Scan(&round, &v)
@@ -3096,8 +3091,7 @@ func (qs *accountsDbQueries) close() {
 		&qs.lookupResourcesStmt,
 		&qs.lookupAllResourcesStmt,
 		&qs.lookupKvPairStmt,
-		&qs.lookupKeysByPrefixTypicalStmt,
-		&qs.lookupKeysByPrefixEmptyOrAllFStmt,
+		&qs.lookupKeysByRangeStmt,
 		&qs.lookupCreatorStmt,
 	}
 	for _, preparedQuery := range preparedQueries {
diff --git a/ledger/accountdb_test.go b/ledger/accountdb_test.go
index 7789898bf..0f9fd9413 100644
--- a/ledger/accountdb_test.go
+++ b/ledger/accountdb_test.go
@@ -1170,19 +1170,11 @@ func TestLookupKeysByPrefix(t *testing.T) {
 	testCases := []struct {
 		prefix        []byte
 		expectedNames [][]byte
+		err           string
 	}{
 		{
 			prefix: []byte{0xFF},
-			expectedNames: [][]byte{
-				{0xFF, 0x12, 0x34, 0x56, 0x78},
-				{0xFF, 0xFF, 0x34, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-				{0xFF, 0xFE, 0xFF},
-				{0xFF, 0xFF},
-				{0xFF, 0xFF, 0x00, 0xFF, 0xFF},
-			},
+			err:    "strange prefix",
 		},
 		{
 			prefix: []byte{0xFF, 0xFE},
@@ -1198,35 +1190,19 @@ func TestLookupKeysByPrefix(t *testing.T) {
 		},
 		{
 			prefix: []byte{0xFF, 0xFF},
-			expectedNames: [][]byte{
-				{0xFF, 0xFF, 0x34, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-				{0xFF, 0xFF},
-				{0xFF, 0xFF, 0x00, 0xFF, 0xFF},
-			},
+			err:    "strange prefix",
 		},
 		{
 			prefix: []byte{0xFF, 0xFF, 0xFF},
-			expectedNames: [][]byte{
-				{0xFF, 0xFF, 0xFF, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-			},
+			err:    "strange prefix",
 		},
 		{
 			prefix: []byte{0xFF, 0xFF, 0xFF, 0xFF},
-			expectedNames: [][]byte{
-				{0xFF, 0xFF, 0xFF, 0xFF, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-			},
+			err:    "strange prefix",
 		},
 		{
 			prefix: []byte{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-			expectedNames: [][]byte{
-				{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-			},
+			err:    "strange prefix",
 		},
 		{
 			prefix: []byte{0xBA, 0xDD, 0xAD, 0xFF},
@@ -1294,25 +1270,7 @@ func TestLookupKeysByPrefix(t *testing.T) {
 		},
 		{
 			prefix: []byte{},
-			expectedNames: [][]byte{
-				{0xFF, 0x12, 0x34, 0x56, 0x78},
-				{0xFF, 0xFF, 0x34, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0x56, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0x78},
-				{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
-				{0xFF, 0xFE, 0xFF},
-				{0xFF, 0xFF},
-				{0xFF, 0xFF, 0x00, 0xFF, 0xFF},
-				{0xBA, 0xDD, 0xAD, 0xFF, 0xFF},
-				{0xBA, 0xDD, 0xAE, 0x00},
-				{0xBA, 0xDD, 0xAE},
-				[]byte("TACOCAT"),
-				[]byte("TACOBELL"),
-				[]byte("DingHo-SmallPack"),
-				[]byte("DingHo-StandardPack"),
-				[]byte("BostonKitchen-CheeseSlice"),
-				[]byte(`™£´´∂ƒ∂ƒßƒ©∑®ƒß∂†¬∆`),
-			},
+			err:    "strange prefix",
 		},
 	}
 
@@ -1320,12 +1278,17 @@ func TestLookupKeysByPrefix(t *testing.T) {
 		t.Run("lookupKVByPrefix-testcase-"+strconv.Itoa(index), func(t *testing.T) {
 			actual := make(map[string]bool)
 			_, err := qs.lookupKeysByPrefix(string(testCase.prefix), uint64(len(kvPairDBPrepareSet)), actual, 0)
-			require.NoError(t, err)
-			expected := make(map[string]bool)
-			for _, name := range testCase.expectedNames {
-				expected[string(name)] = true
+			if err != nil {
+				require.NotEmpty(t, testCase.err, testCase.prefix)
+				require.Contains(t, err.Error(), testCase.err)
+			} else {
+				require.Empty(t, testCase.err)
+				expected := make(map[string]bool)
+				for _, name := range testCase.expectedNames {
+					expected[string(name)] = true
+				}
+				require.Equal(t, actual, expected)
 			}
-			require.Equal(t, actual, expected)
 		})
 	}
 }
diff --git a/ledger/acctupdates.go b/ledger/acctupdates.go
index 5ed4df31e..116eaf9fd 100644
--- a/ledger/acctupdates.go
+++ b/ledger/acctupdates.go
@@ -518,7 +518,7 @@ func (au *accountUpdates) lookupKeysByPrefix(round basics.Round, keyPrefix strin
 					resultCount++
 					// check if the size of `results` reaches `maxKeyNum`
 					// if so just return the list of keys
-					if maxKeyNum > 0 && resultCount == maxKeyNum {
+					if resultCount == maxKeyNum {
 						return
 					}
 				}