From 8702f7d8fbe6d2e19d69a7bba718d23c14d791dc Mon Sep 17 00:00:00 2001
From: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
Date: Tue, 27 Jun 2023 12:28:11 -0500
Subject: [PATCH] Change OrderedMap.Get() to return Value

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

Currently, OrderedMap.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.
---
 cmd/stress/map.go |  6 +-----
 map.go            | 20 ++++++++++++++++++--
 map_test.go       |  5 +----
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/cmd/stress/map.go b/cmd/stress/map.go
index c0edbb74..e2eacaad 100644
--- a/cmd/stress/map.go
+++ b/cmd/stress/map.go
@@ -390,14 +390,10 @@ func checkMapDataLoss(m *atree.OrderedMap, elements map[atree.Value]atree.Value)
 
 	// Check every element
 	for k, v := range elements {
-		storable, err := m.Get(compare, hashInputProvider, k)
+		convertedValue, err := m.Get(compare, hashInputProvider, k)
 		if err != nil {
 			return fmt.Errorf("failed to get element with key %s: %w", k, err)
 		}
-		convertedValue, err := storable.StoredValue(m.Storage)
-		if err != nil {
-			return fmt.Errorf("failed to convert storable to value with key %s: %w", k, err)
-		}
 		err = valueEqual(v, convertedValue)
 		if err != nil {
 			return fmt.Errorf("failed to compare %s and %s: %w", v, convertedValue, err)
diff --git a/map.go b/map.go
index b65b798f..7bb9e3fc 100644
--- a/map.go
+++ b/map.go
@@ -3606,7 +3606,7 @@ func NewMapWithRootID(storage SlabStorage, rootID StorageID, digestBuilder Diges
 }
 
 func (m *OrderedMap) Has(comparator ValueComparator, hip HashInputProvider, key Value) (bool, error) {
-	_, err := m.Get(comparator, hip, key)
+	_, err := m.get(comparator, hip, key)
 	if err != nil {
 		var knf *KeyNotFoundError
 		if errors.As(err, &knf) {
@@ -3618,7 +3618,23 @@ func (m *OrderedMap) Has(comparator ValueComparator, hip HashInputProvider, key
 	return true, nil
 }
 
-func (m *OrderedMap) Get(comparator ValueComparator, hip HashInputProvider, key Value) (Storable, error) {
+func (m *OrderedMap) Get(comparator ValueComparator, hip HashInputProvider, key Value) (Value, error) {
+
+	storable, err := m.get(comparator, hip, key)
+	if err != nil {
+		// Don't need to wrap error as external error because err is already categorized by MapSlab.Get().
+		return nil, err
+	}
+
+	v, err := storable.StoredValue(m.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 (m *OrderedMap) get(comparator ValueComparator, hip HashInputProvider, key Value) (Storable, error) {
 
 	keyDigest, err := m.digesterBuilder.Digest(hip, key)
 	if err != nil {
diff --git a/map_test.go b/map_test.go
index 3b3ced33..f983835e 100644
--- a/map_test.go
+++ b/map_test.go
@@ -119,10 +119,7 @@ func verifyMap(
 
 	// Verify map elements
 	for k, v := range keyValues {
-		s, err := m.Get(compare, hashInputProvider, k)
-		require.NoError(t, err)
-
-		e, err := s.StoredValue(m.Storage)
+		e, err := m.Get(compare, hashInputProvider, k)
 		require.NoError(t, err)
 
 		valueEqual(t, typeInfoComparator, v, e)