Improve migrations #3144
Merged
Improve migrations #3144
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
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 2874bef Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…ue before reinserting
remove keys/values from old dictionar and insert into new dictionary without transfer out/in
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3144 +/- ##
==========================================
+ Coverage 80.53% 80.54% +0.01%
==========================================
Files 381 381
Lines 92625 92727 +102
==========================================
+ Hits 74594 74691 +97
+ Misses 15327 15326 -1
- Partials 2704 2710 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6 tasks
SupunS
approved these changes
Mar 6, 2024
6 tasks
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.
Work towards #3096 and #3149
Description
Great suggestion by @fxamacker: Perform a atree storage health check after each migration, to ensure individual migrations produce valid outcomes.
This currently fails for some tests:
statictypes.TestMigratingNestedContainers/nested_dictionary:entitlements.TestMigrateSimpleContract:Fixes, improvements, and additions to tests
Fixes for migration
The health checks identified several issues, such as missing data or unreferenced data (garbage).
Fix this in general by changing the migrations so that they properly remove old child values from old containers, without transferring them to the stack, and inserting them into the (potentially new) target container, again without transferring them.
The assumption of the container operation functions, e.g.
DictionaryValue.InsertorArrayValue.Remove, is that the input is on the stack, and the result (if any) is also returned on the stack, even if the container itself is in a non-zero address account.During the migration, all values are always in the account that is currently migrated, and values are never on the stack.
To properly move child values between or inside containers, add new function
*WithoutTransferfunctions toDictionaryValue,ArrayValue, andCompositeValue, e.g.DictionaryValue.InsertWithoutTransfer, and use them in the migration. The existing functions for performing operations on containers, likeDictionaryValue.Insertstill perform the same: they transfer the value from the stack to the container and call into the non-transfer variant of the function (e.g. for insertion), or call into the non-transfer function and transfer the result from the account to the stack.Finally, introduce
NewWithTypefunctions for dictionaries and arrays, and use them in the static type and entitlements migration. They create a new container, with the given type, and move all children to the new container. This is currently needed for these migrations, as it is not (yet) possible to change the type of containers.Once atree exposes functions to change the type of containers directly, we can replace these calls and remove the
NewWithTypeagain.Best viewed without whitespace changes.
masterbranchFiles changedin the Github PR explorer