VDB-318 Allow for packed storage slots #121

elizabethengelman · 2019-07-18T21:21:17Z

for storage values that are less than 32bytes the solidity compiler packs them into one storage slot, this PR starts to allow for that decoding

rmulhol

LGTM - nice catch! 🥇

rmulhol · 2019-07-19T15:59:19Z

libraries/shared/storage/utils/decoder.go

@@ -43,6 +47,11 @@ func decodeUint256(raw []byte) string {
 	return n.String()
 }

+func decodeUint128(raw []byte) string {


Wondering if it might make sense to just have one decodeInteger function as long as the implementation's the same for every type 🤔

I was thinking about that, and then also was wondering if it would make sense for each of these decode methods to do some sort of validation that the bytes passed in are the right length?

That seems like it would be cool, but do you know whether we can assume that input will be of a fixed length? Thinking that a uint128 could be 16 bytes if it's packed, or 32 if it's not. I suppose we could validate that the input is greater than the min required?

oh yeah, that's a great point. validating a minimum requirement seems doable, but i'm not sure how useful that may be.

for now, i'll update this to have just one decodeInteger method.

rmulhol · 2019-07-19T16:15:18Z

libraries/shared/storage/utils/value.go

+	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 {
 	return StorageValueMetadata{


This is probably unnecessary but I'm wondering if we might want to put a guard here to check that if t == PackedSlot then packedNames != nil and packedTypes != nil (and vice versa if t != PackedSlot)

rmulhol · 2019-07-19T16:16:53Z

libraries/shared/storage/utils/decoder.go

+}
+
+func getNumberOfBytes(valueType ValueType) int {
+	// 8 bits per byte


maybe worth replacing with a var?

rmulhol · 2019-07-19T16:18:41Z

libraries/shared/storage/utils/decoder.go

+	return decodedStorageSlotItems
+}
+
+func decodeIndividualItems(itemBytes []byte, valueType ValueType) string {


maybe this should be singular?

rmulhol · 2019-07-19T16:21:58Z

libraries/shared/storage/utils/decoder.go

+	decodedStorageSlotItems := map[int]string{}
+	numberOfTypes := len(packedTypes)
+
+	for position := 0; position < numberOfTypes; position++ {


could maybe replace this with for k, v := range packedTypes

Maps also don't have a guaranteed iteration order (golang actually purposefully randomizes the intersection order 😆: https://blog.golang.org/go-maps-in-action), so if we iterate over them with for k, v := range packedTypes they won't necessarily decode the raw data in the right order.

I think an alternative to the current implementation could be to sort the packedTypes keys, and then do something like this

var keys []int for k := range packedTypes { keys = append(keys, k) } sort.Ints(keys) for _, k := range keys { // similar loop }

Do you think that's more readable?

Good point! I'm fine to stick with the current implementation given that we need a guaranteed iteration order.

I am wondering how much it would muck things up to try an implementation that doesn't require a specific iteration order. Thinking that perhaps we could implement a function that returns the relevant slice for a given index provided the indexes and types of all values packed into the slot, but I'm also not sure whether or not that would be more readable 🤔

yeah, i need to think about this a bit more. 🤔 do you think it's worth adding a story in the back log to address this later?

aaizuss · 2019-07-19T17:14:59Z

libraries/shared/factories/storage/transformer_test.go

+			Expect(repository.PassedMetadata).To(Equal(fakeMetadata))
+			expectedPassedValue := make(map[int]string)
+			expectedPassedValue[0] = "10800"
+			expectedPassedValue[1] = "172800"


I know that 2a300 is "172800" in decimal and 2a30 is "10800", which is why you're setting these expectedPassedValues, but I don't understand how you know where to get the 2 slots from "000000000000000000000000000000000000000000000002a300000000002a30" for the expected values (or how the 0 padding works - is that in the solidity documentation?). Why is 2a30 the first slot and 2a300 the second? how do you know the second isn't 2a3000

edit: I commented on this before seeing the decodePackedSlot function, which looks like it might help me understand

aaizuss · 2019-07-19T17:20:08Z

libraries/shared/storage/utils/decoder.go

@@ -43,6 +47,11 @@ func decodeUint256(raw []byte) string {
 	return n.String()
 }

+func decodeUint128(raw []byte) string {


Why do we have three decodeUintxx functions that have the same implementation? Would it make sense to have a single decodeUint function instead?

Yeah, that's a good point that maybe having just one decodeUint function makes sense. The one thing I'm wondering is if having some sort of validation would make sense - like if we call decodeUint128 that we make sure that the passed in bytes are a uint128. 🤔

aaizuss · 2019-07-19T17:42:24Z

libraries/shared/storage/utils/decoder.go

+}
+
+func getNumberOfBytes(valueType ValueType) int {
+	// 8 bits per byte


Seems like we should have a constant for the number 8 that makes it clear a byte is 8 bits (ie ByteBits) - I imagine we use that a lot in the codebase. On the other hand that could be overkill and when I see computation/conversions with the number 8 I usually assume it's because it's the number of bits in a byte. 🤷‍♀

i like the idea of having a constant

aaizuss

I think this is good. I'm having a hard time internalizing how the storage slots work, so I think I need to read the solidity documentation again.

i-norden · 2019-07-19T18:31:43Z

libraries/shared/storage/utils/decoder.go

@@ -51,3 +60,52 @@ func decodeUint48(raw []byte) string {
 func decodeAddress(raw []byte) string {
 	return common.BytesToAddress(raw).Hex()
 }
+
+func decodePackedSlot(raw []byte, packedTypes map[int]ValueType) map[int]string {
+	storageSlotData := raw


I think this assignment is superfluous; could name the argument storageSlotData instead?

I assigned the raw argument to storageSlotData because then on line 84 we're updating what is in storageSlotData by popping off the current item. Does that make sense?

Oh yeah that makes perfect sense, sorry I missed that!

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

elizabethengelman added 2 commits July 18, 2019 13:30

First pass at decoded packed storage slots

b0fff9a

Add tests for decoding packed storage

ebfb496

elizabethengelman force-pushed the allow-for-packed-storage-slots branch from 19ef7f2 to ebfb496 Compare July 18, 2019 21:22

Pass storage values to repo.create as a map

51915f7

elizabethengelman force-pushed the allow-for-packed-storage-slots branch 3 times, most recently from 376988a to b8e51a7 Compare July 19, 2019 15:03

Update GetStorageValueMetadata

d30b4ea

elizabethengelman force-pushed the allow-for-packed-storage-slots branch 2 times, most recently from 83a6c76 to bf27858 Compare July 19, 2019 15:42

Apply go fmt changes and cleanup

69ad521

elizabethengelman force-pushed the allow-for-packed-storage-slots branch from bf27858 to 69ad521 Compare July 19, 2019 15:44

elizabethengelman requested review from rmulhol, i-norden, yaoandrew, Gslaughl and aaizuss and removed request for rmulhol and i-norden July 19, 2019 15:45

rmulhol approved these changes Jul 19, 2019

View reviewed changes

aaizuss reviewed Jul 19, 2019

View reviewed changes

aaizuss approved these changes Jul 19, 2019

View reviewed changes

i-norden approved these changes Jul 19, 2019

View reviewed changes

Address PR feedback

2c66afb

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

elizabethengelman force-pushed the allow-for-packed-storage-slots branch from 7c8337b to 2c66afb Compare July 23, 2019 18:37

elizabethengelman merged commit 85aba29 into staging Jul 23, 2019

elizabethengelman deleted the allow-for-packed-storage-slots branch July 23, 2019 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VDB-318 Allow for packed storage slots #121

VDB-318 Allow for packed storage slots #121

elizabethengelman commented Jul 18, 2019

rmulhol left a comment

rmulhol Jul 19, 2019

elizabethengelman Jul 23, 2019

rmulhol Jul 23, 2019

elizabethengelman Jul 23, 2019

rmulhol Jul 19, 2019

rmulhol Jul 19, 2019

rmulhol Jul 19, 2019

rmulhol Jul 19, 2019

elizabethengelman Jul 23, 2019

rmulhol Jul 23, 2019

elizabethengelman Jul 23, 2019

aaizuss Jul 19, 2019 •

edited

Loading

aaizuss Jul 19, 2019

elizabethengelman Jul 23, 2019

aaizuss Jul 19, 2019

elizabethengelman Jul 23, 2019

aaizuss left a comment

i-norden Jul 19, 2019

elizabethengelman Jul 23, 2019

i-norden Jul 23, 2019

VDB-318 Allow for packed storage slots #121

VDB-318 Allow for packed storage slots #121

Conversation

elizabethengelman commented Jul 18, 2019

rmulhol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaizuss Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaizuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaizuss Jul 19, 2019 •

edited

Loading