-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reintroduce and fix stringAtreeValue #1174
Conversation
value, err := storable.StoredValue(interpreter.Storage) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
if _, ok := value.(stringAtreeValue); ok { | ||
equal, err := stringAtreeComparator(interpreter.Storage, value, otherStorable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the LHS storable and the RHS storable (inside stringAtreeComparator
) are now correctly loaded as values, handling the case where either of them is a storage ID storable
} | ||
|
||
func stringAtreeComparator(storage atree.SlabStorage, value atree.Value, otherStorable atree.Storable) (bool, error) { | ||
otherValue, err := otherStorable.StoredValue(storage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We correctly load the value for the other storable here and do not incorrectly cast it directly to stringAtreeValue
key := MustConvertStoredValue(atreeKey) | ||
keyCopy := interpreter.CopyValue(getLocationRange, key, address) | ||
// NOTE: key is stringAtreeValue | ||
// and does not need to be converted or copied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but do we need to copy key here? Just double-checking. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key is not a Cadence interpreter.Value
, so there is nothing to deep copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if it would be, copying it is literally just using it, as it is effectively a Go string (value type, immutable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Reverts and fixes 17eec97, i.e. reintroduce
stringAtreeValue
as theatree.Value
used for composite value field names (atree ordered map keys).This restores the behaviour, so we do not need a migration for Testnet, and the PR also fixes the problem identified by @fxamacker earlier that the old
stringAtreeValue
code was not handling the case properly where the value's size exceeds the max inline limit, and storables where casted tostringAtreeValue
, but may beStorageIDStorable
in that case