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.

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

Unify system-contracts migration and staged-contracts migration #5405

Merged
merged 7 commits into from
Feb 23, 2024

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Feb 16, 2024

Work towards onflow/cadence#2865

Depends on onflow/cadence#3117

Validator is disabled by default.

I also realized the core-contract migration and the staged contract migration contains a lot of duplicate logic.
Hence refactored the core-contracts migration (ChangeContractCodeMigration) to also use the StagedContractMigration underneath. Core contract changes are also in a way, a set of staged contract changes.

@SupunS SupunS self-assigned this Feb 16, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 57.74%. Comparing base (46d7563) to head (d48ca2b).

Files Patch % Lines
...edger/migrations/change_contract_code_migration.go 87.87% 8 Missing ⚠️
...md/util/ledger/util/migration_runtime_interface.go 0.00% 6 Missing ⚠️
...il/ledger/migrations/staged_contracts_migration.go 94.54% 2 Missing and 1 partial ⚠️
cmd/util/ledger/migrations/migrator_runtime.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5405      +/-   ##
==========================================================
+ Coverage                   56.37%   57.74%   +1.37%     
==========================================================
  Files                        1030      839     -191     
  Lines                      100057    82907   -17150     
==========================================================
- Hits                        56403    47873    -8530     
+ Misses                      39394    31373    -8021     
+ Partials                     4260     3661     -599     
Flag Coverage Δ
unittests 57.74% <85.71%> (+1.37%) ⬆️

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.

Comment on lines +26 to +29
const contractA = `
access(all) contract A {
access(all) fun foo() {}
}`
Copy link
Member Author

@SupunS SupunS Feb 16, 2024

Choose a reason for hiding this comment

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

Tests below would now have to use valid contract codes, once/if the update validator is enabled.
So I replaced the dummy codes with valid cadence codes.

@SupunS SupunS marked this pull request as ready for review February 16, 2024 08:34
@SupunS SupunS changed the title Run contract update validator for staged contracts Run contract update validator for system contracts migration Feb 16, 2024
@SupunS SupunS force-pushed the supun/contract-update-validator branch from eff3173 to 4d7d75d Compare February 16, 2024 11:15
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.

Thank you for refactoring the duplicated code!

Does this mean system contract updates are now going to be checked by the updated contract update checker, like other staged contract updates, and forced to pass?

go.mod Outdated Show resolved Hide resolved
@SupunS SupunS changed the title Run contract update validator for system contracts migration Unify system-contracts migration and staged-contracts migration Feb 17, 2024
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 refactoring and cleaning this up!

@turbolent turbolent merged commit 9bcc862 into feature/stable-cadence Feb 23, 2024
50 of 51 checks passed
@turbolent turbolent deleted the supun/contract-update-validator branch February 23, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants