Only encode reference static type in legacy format if it was decoded as such #3199
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 1a59183 Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3199 +/- ##
=======================================
Coverage 80.68% 80.68%
=======================================
Files 380 380
Lines 93164 93175 +11
=======================================
+ Hits 75166 75177 +11
Misses 15294 15294
Partials 2704 2704
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dsainati1
approved these changes
Mar 27, 2024
SupunS
reviewed
Mar 27, 2024
There was a problem hiding this comment.
Good catch! I'm wondering whether we need to solve this generically. As in, when removing the key, should we maybe first try removing with the existing key, and if it fails, then fallback and try remove with the legacy key. That will of course add some overhead, but it will also allow running the migration multiple times in future, I guess?
|
I think the solution already solves this "generically" / supports running the migration multiple times: Originally, when the reference type is decoded for the "first" time, and it has a legacy authorization flag, then the legacy key wrapper will encode it as such again. A migration may then replace it with a reference type that has a new authorization. When the migration is run again, it is decoded with the new authorization, and the legacy key wrapper will use the new encoding. But maybe I'm missing something. It's a bit hard to test |
SupunS
approved these changes
Mar 27, 2024
|
@SupunS please let me know if you still think there's something we need to handle here / there's a flaw in my reasoning. My plan was to merge this and see how it works. We might still need to refine this further. Also, I had a look at the other "legacy wrappers", but AFAICS they do not have a similar problem. Again, I might be missing something |
|
ah no, that makes sense. My concern was mostly with the rest of the legacy wrappers, but I had a look at it them again, and I also felt that they would seamlessly work. We can revisit only if we run into something similar, otherwise no need to try and fix something that is not broken 😄 |
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 #3192
Description
When removing an existing key from a dictionary, a "legacy key" is used, which encodes the key in its original form.
However, only do so if the key was actually decoded in legacy form.
In particular, reference static types may have been already migrated, e.g. the entitlements migration adds authorization with an entitlement set, which then should not be encoded in legacy form, i.e. with an authorization flag.
masterbranchFiles changedin the Github PR explorer