Address PR feedback

- factor out bitsPerByte constant
- panic if the necessary args aren't passed to GetStorageValueMetadataForPackedSlot
- only have one decode integer method in the deocoder file

libraries/shared/storage/utils/decoder.go

@@ -23,14 +23,18 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 )
 
+const (
+	bitsPerByte = 8
+)
+
 func Decode(row StorageDiffRow, metadata StorageValueMetadata) (interface{}, error) {
 	switch metadata.Type {
 	case Uint256:
-		return decodeUint256(row.StorageValue.Bytes()), nil
+		return decodeInteger(row.StorageValue.Bytes()), nil
 	case Uint48:
-		return decodeUint48(row.StorageValue.Bytes()), nil
+		return decodeInteger(row.StorageValue.Bytes()), nil
 	case Uint128:
-		return decodeUint128(row.StorageValue.Bytes()), nil
+		return decodeInteger(row.StorageValue.Bytes()), nil
 	case Address:
 		return decodeAddress(row.StorageValue.Bytes()), nil
 	case Bytes32:
@@ -42,17 +46,7 @@ func Decode(row StorageDiffRow, metadata StorageValueMetadata) (interface{}, err
 	}
 }
 
-func decodeUint256(raw []byte) string {
+func decodeInteger(raw []byte) string {
-	n := big.NewInt(0).SetBytes(raw)
-	return n.String()
-}
-
-func decodeUint128(raw []byte) string {
-	n := big.NewInt(0).SetBytes(raw)
-	return n.String()
-}
-
-func decodeUint48(raw []byte) string {
 	n := big.NewInt(0).SetBytes(raw)
 	return n.String()
 }
@@ -90,21 +84,20 @@ func decodePackedSlot(raw []byte, packedTypes map[int]ValueType) map[int]string
 func decodeIndividualItems(itemBytes []byte, valueType ValueType) string {
 	switch valueType {
 	case Uint48:
-		return decodeUint48(itemBytes)
+		return decodeInteger(itemBytes)
 	case Uint128:
-		return decodeUint128(itemBytes)
+		return decodeInteger(itemBytes)
 	default:
 		panic(fmt.Sprintf("can't decode unknown type: %d", valueType))
 	}
 }
 
 func getNumberOfBytes(valueType ValueType) int {
-	// 8 bits per byte
 	switch valueType {
 	case Uint48:
-		return 48 / 8
+		return 48 / bitsPerByte
 	case Uint128:
-		return 128 / 8
+		return 128 / bitsPerByte
 	default:
 		panic(fmt.Sprintf("ValueType %d not recognized", valueType))
 	}

libraries/shared/storage/utils/value.go

@@ -16,6 +16,8 @@
 
 package utils
 
+import "fmt"
+
 type ValueType int
 
 const (
@@ -33,8 +35,8 @@ type StorageValueMetadata struct {
 	Name        string
 	Keys        map[Key]string
 	Type        ValueType
-	PackedNames map[int]string    //position in map (zero indexed) => name of packed item
+	PackedNames map[int]string    //zero indexed position in map => name of packed item
-	PackedTypes map[int]ValueType //position in map (zero indexed)=> type of packed item
+	PackedTypes map[int]ValueType //zero indexed position in map => type of packed item
 }
 
 func GetStorageValueMetadata(name string, keys map[Key]string, valueType ValueType) StorageValueMetadata {
@@ -45,12 +47,23 @@ func GetStorageValueMetadataForPackedSlot(name string, keys map[Key]string, valu
 	return getMetadata(name, keys, valueType, packedNames, packedTypes)
 }
 
-func getMetadata(name string, keys map[Key]string, t ValueType, packedNames map[int]string, packedTypes map[int]ValueType) StorageValueMetadata {
+func getMetadata(name string, keys map[Key]string, valueType ValueType, packedNames map[int]string, packedTypes map[int]ValueType) StorageValueMetadata {
+	assertPackedSlotArgs(valueType, packedNames, packedTypes)
+
 	return StorageValueMetadata{
 		Name:        name,
 		Keys:        keys,
-		Type:        t,
+		Type:        valueType,
 		PackedNames: packedNames,
 		PackedTypes: packedTypes,
 	}
 }
+
+func assertPackedSlotArgs(valueType ValueType, packedNames map[int]string, packedTypes map[int]ValueType) {
+	if valueType == PackedSlot && (packedTypes == nil || packedNames == nil) {
+		panic(fmt.Sprintf("ValueType is PackedSlot. Expected PackedNames and PackedTypes to not be nil, but got PackedNames = %v and PackedTypes = %v", packedNames, packedTypes))
+	} else if (packedNames != nil && packedTypes != nil) && valueType != PackedSlot {
+		panic(fmt.Sprintf("PackedNames and PackedTypes passed in. Expected ValueType to equal PackedSlot (%v), but got %v.", PackedSlot, valueType))
+	}
+
+}

libraries/shared/storage/utils/value_test.go

@@ -20,20 +20,59 @@ var _ = Describe("Storage value metadata getter", func() {
 		Expect(utils.GetStorageValueMetadata(metadataName, metadataKeys, metadataType)).To(Equal(expectedMetadata))
 	})
 
-	It("returns metadata for a packed storage slot variables", func() {
+	Describe("metadata for a packed storaged slot", func() {
-		metadataName := "fake_name"
+		It("returns metadata for multiple storage variables", func() {
-		metadataKeys := map[utils.Key]string{"key": "value"}
+			metadataName := "fake_name"
-		metadataType := utils.Uint256
+			metadataKeys := map[utils.Key]string{"key": "value"}
-		metadataPackedNames := map[int]string{0: "name"}
+			metadataType := utils.PackedSlot
-		metadataPackedTypes := map[int]utils.ValueType{0: utils.Uint48}
+			metadataPackedNames := map[int]string{0: "name"}
+			metadataPackedTypes := map[int]utils.ValueType{0: utils.Uint48}
 
-		expectedMetadata := utils.StorageValueMetadata{
+			expectedMetadata := utils.StorageValueMetadata{
-			Name:        metadataName,
+				Name:        metadataName,
-			Keys:        metadataKeys,
+				Keys:        metadataKeys,
-			Type:        metadataType,
+				Type:        metadataType,
-			PackedTypes: metadataPackedTypes,
+				PackedTypes: metadataPackedTypes,
-			PackedNames: metadataPackedNames,
+				PackedNames: metadataPackedNames,
-		}
+			}
-		Expect(utils.GetStorageValueMetadataForPackedSlot(metadataName, metadataKeys, metadataType, metadataPackedNames, metadataPackedTypes)).To(Equal(expectedMetadata))
+			Expect(utils.GetStorageValueMetadataForPackedSlot(metadataName, metadataKeys, metadataType, metadataPackedNames, metadataPackedTypes)).To(Equal(expectedMetadata))
+		})
+
+		It("panics if PackedTypes are nil when the type is PackedSlot", func() {
+			metadataName := "fake_name"
+			metadataKeys := map[utils.Key]string{"key": "value"}
+			metadataType := utils.PackedSlot
+			metadataPackedNames := map[int]string{0: "name"}
+
+			getMetadata := func() {
+				utils.GetStorageValueMetadataForPackedSlot(metadataName, metadataKeys, metadataType, metadataPackedNames, nil)
+			}
+			Expect(getMetadata).To(Panic())
+		})
+
+		It("panics if PackedNames are nil when the type is PackedSlot", func() {
+			metadataName := "fake_name"
+			metadataKeys := map[utils.Key]string{"key": "value"}
+			metadataType := utils.PackedSlot
+			metadataPackedTypes := map[int]utils.ValueType{0: utils.Uint48}
+
+			getMetadata := func() {
+				utils.GetStorageValueMetadataForPackedSlot(metadataName, metadataKeys, metadataType, nil, metadataPackedTypes)
+			}
+			Expect(getMetadata).To(Panic())
+		})
+
+		It("panics if valueType is not PackedSlot if PackedNames is populated", func() {
+			metadataName := "fake_name"
+			metadataKeys := map[utils.Key]string{"key": "value"}
+			metadataType := utils.Uint48
+			metadataPackedNames := map[int]string{0: "name"}
+			metadataPackedTypes := map[int]utils.ValueType{0: utils.Uint48}
+
+			getMetadata := func() {
+				utils.GetStorageValueMetadataForPackedSlot(metadataName, metadataKeys, metadataType, metadataPackedNames, metadataPackedTypes)
+			}
+			Expect(getMetadata).To(Panic())
+		})
 	})
 })