Fix optional type ID ambiguity and migrate types #3272
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 052db5e Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SupunS
reviewed
Apr 19, 2024
There was a problem hiding this comment.
Can you please also add a "Rehash test", similar to:
cadence/migrations/statictypes/intersection_type_migration_test.go
Lines 481 to 484 in 922e65c
The idea is to test, if there is an optional-typed TypeValue is being used as a key in a dictionary, then retrieving (using the old hash/typeID) and updating that key during the migration should work.
I think |
|
hmm, yes it does (didn't realize it earlier). However, that test is for testing entitlements rehashing, and the optional-type seems more like a coincident/side-effect? It would be good to have a separate test for optionals, so that any change to the entitlement test won't accidentally remove the use of optional type. |
|
So while I was writing these tests, I realized that I am not actually sure the migration itself is strictly necessary. We had to migrated restricted/intersection and reference static types because how they are encoded and decoded from storage changed, but with optional types even though the TypeID itself is changing how it's printed, the encoding of these values is not changing. So any optional static types that were created and encoded with the old IDs are just going to be loaded from storage the same way, and there is no migration that actually needs to be performed on the data. I realized this while trying to write the test @SupunS was talking about above because I couldn't actually get the migration to run on the test data, as when the data was loaded from storage to be migrated, it was already "migrated" so to speak, in that it was already converted to the new format by virtue of having been decoded. Am I misunderstanding something about how this works, or do we just not need to do anything here? |
|
@dsainati1 Do you need help here with the rehash test case? I can finish this up |
|
If you have time that would be super helpful! Otherwise I can get to it after the security stuff |
SupunS
reviewed
Apr 23, 2024
turbolent
reviewed
Apr 23, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3272 +/- ##
==========================================
+ Coverage 80.80% 80.81% +0.01%
==========================================
Files 380 381 +1
Lines 93643 93724 +81
==========================================
+ Hits 75666 75743 +77
- Misses 15276 15279 +3
- Partials 2701 2702 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@dsainati1 @SupunS Could you PTAL? |
turbolent
approved these changes
Apr 23, 2024
SupunS
approved these changes
Apr 23, 2024
turbolent
added
the
Storage Breaking Change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3248
masterbranchFiles changedin the Github PR explorer