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

Add staged contract migration #5300

Merged
merged 10 commits into from
Jan 26, 2024

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Jan 25, 2024

@SupunS SupunS self-assigned this Jan 25, 2024
@SupunS SupunS added the Feature label Jan 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (8af35c0) 58.45% compared to head (838a812) 55.64%.

Files Patch % Lines
...il/ledger/migrations/staged_contracts_migration.go 87.80% 11 Missing and 4 partials ⚠️
...execution-state-extract/execution_state_extract.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5300      +/-   ##
==========================================================
- Coverage                   58.45%   55.64%   -2.81%     
==========================================================
  Files                         911      998      +87     
  Lines                       86472    96256    +9784     
==========================================================
+ Hits                        50547    53563    +3016     
- Misses                      32208    38673    +6465     
- Partials                     3717     4020     +303     
Flag Coverage Δ
unittests 55.64% <81.81%> (-2.81%) ⬇️

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.

@SupunS SupunS force-pushed the supun/staged-contract-migration branch from 01917b2 to a5b8e04 Compare January 25, 2024 21:01
@SupunS SupunS changed the base branch from feature/stable-cadence to bastian/stable-cadence-update January 25, 2024 22:54
@SupunS SupunS force-pushed the supun/staged-contract-migration branch from 443e492 to 9e2f6a0 Compare January 25, 2024 22:56
go.mod Outdated Show resolved Hide resolved
@SupunS SupunS marked this pull request as ready for review January 25, 2024 22:59
@SupunS SupunS requested review from turbolent and dsainati1 January 25, 2024 23:00
@SupunS SupunS force-pushed the supun/staged-contract-migration branch from 9e2f6a0 to 02f8cc5 Compare January 25, 2024 23:02
Base automatically changed from bastian/stable-cadence-update to feature/stable-cadence January 25, 2024 23:13
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!

@SupunS SupunS force-pushed the supun/staged-contract-migration branch from ac81bbc to f1c61a9 Compare January 26, 2024 04:11
@SupunS SupunS force-pushed the supun/staged-contract-migration branch from f1c61a9 to e7c7d41 Compare January 26, 2024 16:22
Comment on lines +97 to +104
// Contracts must be migrated first
migrators.CreateAccountBasedMigration(
log,
nWorker,
[]migrators.AccountBasedMigration{
migrators.NewStagedContractsMigration(migrators.GetStagedContracts),
},
),
Copy link
Member Author

@SupunS SupunS Jan 26, 2024

Choose a reason for hiding this comment

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

Not sure if this is the proper way to do this: We need to run contract migration for all accounts first, before running other migrations for any account.

@SupunS SupunS requested a review from turbolent January 26, 2024 16:27
@turbolent
Copy link
Member

TestCadenceValuesMigration is failing, not quite sure why

@turbolent
Copy link
Member

Sorry, that was my bad, I had changed the format of the error message!

@SupunS
Copy link
Member Author

SupunS commented Jan 26, 2024

ah no, that's totally fine! It's a bit fragile/not really a good practice to assert on the error messages anyway. But don't really have a better alternative in this case really. In that test I want to make sure the 'correct' error is on the correct account, path, and the key (which is pretty much the entire error message)

@turbolent turbolent merged commit 97279f9 into feature/stable-cadence Jan 26, 2024
50 of 51 checks passed
@turbolent turbolent deleted the supun/staged-contract-migration branch January 26, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants