From 044db0141ee6cdd273e224faca32d625ea168826 Mon Sep 17 00:00:00 2001
From: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
Date: Tue, 27 Jun 2023 10:28:28 -0500
Subject: [PATCH] Change Array.Get() to return Value instead of Storable

This commit changes Array.Get() to return Value instead of Storable.

Currently, Array.Get() returns Storable and client converts
returned Storable to Value.  However, it only makes sense to
return Storable if client needs to remove register (slab) by
StorageID (StorageIDStorable).

Get() should only provide value without possibility of client
manipulating the underlying storable.
---
 array.go            | 15 ++++++++++++---
 array_bench_test.go | 13 +++++++------
 array_test.go       |  9 +++------
 cmd/stress/array.go |  6 +-----
 4 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/array.go b/array.go
index a28446a0..98213063 100644
--- a/array.go
+++ b/array.go
@@ -1995,9 +1995,18 @@ func NewArrayWithRootID(storage SlabStorage, rootID StorageID) (*Array, error) {
 	}, nil
 }
 
-func (a *Array) Get(i uint64) (Storable, error) {
-	// Don't need to wrap error as external error because err is already categorized by ArraySlab.Get().
-	return a.root.Get(a.Storage, i)
+func (a *Array) Get(i uint64) (Value, error) {
+	storable, err := a.root.Get(a.Storage, i)
+	if err != nil {
+		// Don't need to wrap error as external error because err is already categorized by ArraySlab.Get().
+		return nil, err
+	}
+	v, err := storable.StoredValue(a.Storage)
+	if err != nil {
+		// Wrap err as external error (if needed) because err is returned by Storable interface.
+		return nil, wrapErrorfAsExternalErrorIfNeeded(err, "failed to get storable's stored value")
+	}
+	return v, nil
 }
 
 func (a *Array) Set(index uint64, value Value) (Storable, error) {
diff --git a/array_bench_test.go b/array_bench_test.go
index a83b9bd4..04578845 100644
--- a/array_bench_test.go
+++ b/array_bench_test.go
@@ -25,7 +25,8 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var noop Storable
+var noopValue Value
+var noopStorable Storable
 
 func BenchmarkArrayGet100x(b *testing.B) {
 	benchmarks := []struct {
@@ -223,18 +224,18 @@ func benchmarkArrayGet(b *testing.B, initialArraySize, numberOfOps int) {
 
 	array := setupArray(b, r, storage, initialArraySize)
 
-	var storable Storable
+	var value Value
 
 	b.StartTimer()
 
 	for i := 0; i < b.N; i++ {
 		for i := 0; i < numberOfOps; i++ {
 			index := r.Intn(int(array.Count()))
-			storable, _ = array.Get(uint64(index))
+			value, _ = array.Get(uint64(index))
 		}
 	}
 
-	noop = storable
+	noopValue = value
 }
 
 func benchmarkArrayInsert(b *testing.B, initialArraySize, numberOfOps int) {
@@ -307,7 +308,7 @@ func benchmarkArrayRemoveAll(b *testing.B, initialArraySize int) {
 		}
 	}
 
-	noop = storable
+	noopStorable = storable
 }
 
 func benchmarkArrayPopIterate(b *testing.B, initialArraySize int) {
@@ -336,7 +337,7 @@ func benchmarkArrayPopIterate(b *testing.B, initialArraySize int) {
 		}
 	}
 
-	noop = storable
+	noopStorable = storable
 }
 
 func benchmarkNewArrayFromAppend(b *testing.B, initialArraySize int) {
diff --git a/array_test.go b/array_test.go
index 8ce86fc8..3bf7b1d4 100644
--- a/array_test.go
+++ b/array_test.go
@@ -57,10 +57,7 @@ func verifyArray(
 
 	// Verify array elements
 	for i, v := range values {
-		s, err := array.Get(uint64(i))
-		require.NoError(t, err)
-
-		e, err := s.StoredValue(array.Storage)
+		e, err := array.Get(uint64(i))
 		require.NoError(t, err)
 
 		valueEqual(t, typeInfoComparator, v, e)
@@ -153,8 +150,8 @@ func TestArrayAppendAndGet(t *testing.T) {
 		require.NoError(t, err)
 	}
 
-	storable, err := array.Get(array.Count())
-	require.Nil(t, storable)
+	e, err := array.Get(array.Count())
+	require.Nil(t, e)
 	require.Equal(t, 1, errorCategorizationCount(err))
 
 	var userError *UserError
diff --git a/cmd/stress/array.go b/cmd/stress/array.go
index 305d296a..68d7f59d 100644
--- a/cmd/stress/array.go
+++ b/cmd/stress/array.go
@@ -417,14 +417,10 @@ func checkArrayDataLoss(array *atree.Array, values []atree.Value) error {
 
 	// Check every element
 	for i, v := range values {
-		storable, err := array.Get(uint64(i))
+		convertedValue, err := array.Get(uint64(i))
 		if err != nil {
 			return fmt.Errorf("failed to get element at %d: %w", i, err)
 		}
-		convertedValue, err := storable.StoredValue(array.Storage)
-		if err != nil {
-			return fmt.Errorf("failed to convert storable to value at %d: %w", i, err)
-		}
 		err = valueEqual(v, convertedValue)
 		if err != nil {
 			return fmt.Errorf("failed to compare %s and %s: %w", v, convertedValue, err)