Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add cadence values migration #5192

Merged
merged 31 commits into from
Jan 26, 2024
Merged

Add cadence values migration #5192

merged 31 commits into from
Jan 26, 2024

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Jan 2, 2024 •
edited
Loading

Closes onflow/cadence#2990

Depends on #5290

@SupunS SupunS self-assigned this Jan 2, 2024
@SupunS SupunS force-pushed the supun/cadence-migrations branch from b7d256f to baf218f Compare January 3, 2024 19:28
@SupunS SupunS marked this pull request as ready for review January 4, 2024 19:37
@SupunS SupunS requested a review from yhassanzadeh13 as a code owner January 4, 2024 19:37
turbolent
turbolent reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

cmd/util/ledger/migrations/cadence_values_migration.go Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration_test.go Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration_test.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/cadence-migrations branch from 361727d to 9eb4edb Compare January 8, 2024 23:33
@SupunS SupunS mentioned this pull request Jan 9, 2024
Merged
6 tasks
@SupunS SupunS force-pushed the supun/cadence-migrations branch from a18c787 to 177c5c4 Compare January 9, 2024 20:40
@turbolent
Copy link
Member

Could you please add a simple test case for the links/cap con migration? (also happy to add it)

Basically, extend the transaction used for the snapshot to link a capability and save it to storage, then assert that after the migration the link became a cap con, and the capability can be still borrowed.

turbolent
turbolent reviewed Jan 11, 2024
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

cmd/util/ledger/migrations/cadence_values_migration.go Show resolved Hide resolved
cmd/util/ledger/migrations/test-data/cadence_values_migration/README.md Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/test-data/cadence_values_migration/README.md Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/cadence-migrations branch from 31b485d to e947745 Compare January 11, 2024 03:13
@SupunS SupunS requested a review from dsainati1 January 18, 2024 21:02
turbolent
turbolent reviewed Jan 18, 2024
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Just some last minor questions/concerns

cmd/util/cmd/execution-state-extract/execution_state_extract.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/migrator_runtime.go Outdated Show resolved Hide resolved
turbolent
turbolent approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@turbolent
Copy link
Member

re: onflow/cadence#3007: Do we need NewStaticTypeMigration here? We can maybe get this merged as-is, and add/change that in a follow up PR?

@SupunS
Copy link
Member Author

SupunS commented Jan 23, 2024 •
edited
Loading

@turbolent Yeah, this PR was before the refactoring and also doesn't include changes introduced in onflow/cadence#3036. We would need to update the fvm side to match those API changes, when the Cadence version in bumped

@turbolent
Copy link
Member

OK, I'll open a follow-up PR then

@turbolent turbolent mentioned this pull request Jan 24, 2024
Merged
@SupunS SupunS changed the base branch from supun/atree-migration-cadence-1.0 to bastian/stable-cadence-update January 25, 2024 22:46
Base automatically changed from bastian/stable-cadence-update to feature/stable-cadence January 25, 2024 23:13
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a6e8ebc) 55.63% compared to head (039970c) 50.47%.

Additional details and impacted files 
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5192      +/-   ##
==========================================================
- Coverage                   55.63%   50.47%   -5.17%     
==========================================================
  Files                        1002      196     -806     
  Lines                       96497    17433   -79064     
==========================================================
- Hits                        53688     8799   -44889     
+ Misses                      38770     8026   -30744     
+ Partials                     4039      608    -3431
Flag Coverage Δ
unittests 50.47% <ø> (-5.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@turbolent
Copy link
Member

Linter is reporting some issues: https://github.com/onflow/flow-go/actions/runs/7661655020/job/20883020316?pr=5192#step:5:27

@SupunS SupunS force-pushed the supun/cadence-migrations branch from 1818f2e to 039970c Compare January 26, 2024 00:52
@turbolent turbolent merged commit a89fb51 into feature/stable-cadence Jan 26, 2024
50 of 51 checks passed
@turbolent turbolent deleted the supun/cadence-migrations branch January 26, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janezpodhostnik janezpodhostnik janezpodhostnik approved these changes

@turbolent turbolent turbolent approved these changes

@yhassanzadeh13 yhassanzadeh13 Awaiting requested review from yhassanzadeh13

@dsainati1 dsainati1 Awaiting requested review from dsainati1

@peterargue peterargue Awaiting requested review from peterargue

@zhangchiqing zhangchiqing Awaiting requested review from zhangchiqing

@tarakby tarakby Awaiting requested review from tarakby

@ramtinms ramtinms Awaiting requested review from ramtinms

@AlexHentschel AlexHentschel Awaiting requested review from AlexHentschel

@durkmurder durkmurder Awaiting requested review from durkmurder

@jordanschalm jordanschalm Awaiting requested review from jordanschalm

Assignees

@SupunS SupunS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SupunS @turbolent @codecov-commenter @janezpodhostnik