-
Notifications
You must be signed in to change notification settings - Fork 137
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
QA full Cadence 1.0 state migration in Emulator #3098
Comments
I have already done this testing, so guess we can mark this as done. Maybe one last thing to test is the final CLI build (for the milestone release), once it is out. |
Nice work doing some initial testing Supun! Eventually, it would be good to do a final test before the release. There are still some improvements to the state migration outstanding: onflow/flow-go#5479 and we should re-test once those landed in the Emulator. It would also be great to test the state migration on a "larger" / "real" project. I had asked Dev Ex on Discord here: https://discord.com/channels/613813861610684416/1123314820763111465/1210727371905171496 |
ah nice! |
Forgot to mention / just to be clear: Ideally this isn't done by "us" (Cadence team), but maybe by someone familiar with such an example project. cc @j1010001 to maybe connect with Dev Ex / the community |
I tried testing from CLI branch using
Seemed to me they are errors regarding upgrading base contracts. The original
But now I'm getting:
Is there something I'm missing? I was following this guide. |
@nvdtf Can you try adding the following, to the "deployments": {
"emulator": {
"emulator-account": [
"ExampleNFT"
]
}
} If that was the issue, I'll also add this to the migration guide. |
@SupunS Yeah that fixed the
The produced reports are empty. Not sure what I'm doing wrong |
What build of the CLI are you using? It's likely that the latest release has problems. @SupunS I'll update the Emulator once onflow/flow-go#5533 and onflow/flow-go#5540 are merged. Would be great to get this updated all the way down to the CLI. |
The latest release doesn't support |
So what is failing is the core-contracts migration. Seems the expected core/system contracts cannot be found in the emulator state. Did the contract addresses change at some point in the emulator? (not sure, but I think I saw somewhere something like that).
Do you know what's the version of the CLI used to create the old state? Trying to see if it's a version before the |
Yeah, that's likely. Maybe we need to make the update for this contract conditional. Alternatively we should recommend updating the pre-1.0 CLI to the latest version when creating the snapshot |
That could be likely. My old CLI is v1.4.2 so its a few versions behind. I don't know if it matters, but I run the old one with |
Updated the guide with these points |
@nvdtf Can you please re-try with the latest pre-1.0 CLI to create the snapshot, and with the 1.0 feature branch: https://github.com/onflow/flow-cli/tree/feature/stable-cadence – it has the migrate command now, and improved migration code. There are a couple known issues, but I'm also currently updating the CLI once again to the latest migration code with fixes for the known issues. |
@nvdtf I finished the update, can you please retry with the latest CLI preview release: https://github.com/onflow/flow-cli/releases/tag/v1.12.0-cadence-v1.0.0-preview.9 ? |
Thanks for the updates! Looks like the previous errors are all solved. I'm using the latest released
|
Looking into it, could be a bug in the migration. Think I know what's happening |
I think the problem is that developers will likely use string-location imports, e.g. Do we have that import-rewrite logic integrated in the flow migrate command? cc @jribbink |
@nvdtf Does the contracts section in {
"contracts": {
"ExampleNFT": "updated_ExampleNFT.cdc"
},
} |
@SupunS Yes it points to the updated version. After I saw the error I double checked to make sure. Wasn't sure why its complaining about all |
So there were two issues:
Once both of these fixes are merged, I'll do another CLI build. |
@nvdtf A new CLI release with the fix is now available: https://github.com/onflow/flow-cli/releases/tag/v1.15.0-cadence-v1.0.0-preview.12 Can you please try with the new release and see if that works fine? |
Thanks for the new releases! We're one step closer to getting a successful run now. We found out the new NFTv2 changes are flagged as invalid contract updates in the migration function: Some of the contract updates are not supported out-of-the-box with migration. |
I think it makes sense to update the It also makes sense that it would complain about the mismatching
and now it is
which would be an invalid upgrade. I didn't realize we were actually going to be migrating I can review a PR to the flow-nft repo if y'all make one this week and I can help get the dependencies updated in all the contracts repos so we can get flow-go and the CLI updated also. Shouldn't take too long once the |
Thanks Josh for having a look!
We are not actually doing any migrations to the This did uncover some of the rules that need to be / could be relaxed in the contract-update-validator. And for others, like the new fields, I believe developers could work around by adjusting their code. |
🎉 I ran a successful migration with
|
That's awesome! Once recently identified bugs (see https://github.com/onflow/cadence/milestone/27) have been fixed, we should re-run this test case |
@nvdtf Could you please re-test using the latest CLI preview release? Once it's been verified that everything works as expected, could you please close the issue? |
Executed the above test again. No errors and got expected output. Closing issue. |
Goal: Someone (in 4D?) can take a test contract, take snapshot based on v0.42 and run migration on emulator and validate the upgraded contract works correctly.
depends on: onflow/flow-cli#1433
The text was updated successfully, but these errors were encountered: