Cadence Account Storage Map Migration #6761
Conversation
…omain-payloads-and-domain-storage-maps
…ntV2Migration contract
|
@turbolent Thanks for working on this part of the integration! Do we want to create a feature branch and target this PR to the feature branch? |
|
@onflow/flow-cadence-execution is there a way to call |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6761 +/- ##
===========================================
+ Coverage 39.71% 41.50% +1.78%
===========================================
Files 1332 111 -1221
Lines 123050 12400 -110650
===========================================
- Hits 48875 5146 -43729
+ Misses 69986 6868 -63118
+ Partials 4189 386 -3803
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@turbolent during bootstrapping you have access to the function When you want to run Let me know if that helps. |
|
@janezpodhostnik Thanks! It turned out that setting up the functions in the runtime won't work, because an environments chain ID might change (default config chain is MN, but then actual chain is e.g. Emulator). I refactored the code to be similar to the EVM setup, i.e. declaring the functions in the transaction invoker and in the script invoker. |
turbolent
changed the base branch from
master
to
auto-update-onflow-cadence-v1.3.0
|
@fxamacker I fixed a few things and added a test in 8b597cb which demonstrates that the migration of the service account works. Next step is probably to write an integration test which does this using the system chunk transaction, but I'm very new to this. Maybe someone from @onflow/flow-cadence-execution can take this on or at least point me in the right direction? |
|
@turbolent Thanks for adding the test and other updates! Would you like me to start reviewing this PR or should I wait until integration test is added and DRAFT status is removed? No rush, just checking before I begin something needed next week. |
|
@fxamacker Feel free to already start reviewing it. We'll probably want to have the integration tests before we merge, but feedback is already appreciated, thanks! |
|
As discussed in the last Execution Team meeting, I've added another We'll have to deploy this PR in an HCU, given the system chunk transaction change, but with this change, we can repurpose the |
turbolent
requested review from
janezpodhostnik and
zhangchiqing
as code owners
There was a problem hiding this comment.
I imagined that scheduleAccountV2Migration would be on the Migration contract and would just be called scheduleAccountMigration which would then dispatch this to the V2 contract.
I also though we could avoid using the V2 contract directly anywhere in the FVM (excluding tests of course).
| } | ||
| let evmHeartbeat = serviceAccount.storage | ||
| .borrow<&EVM.Heartbeat>(from: /storage/EVMHeartbeat) | ||
| evmHeartbeat?.heartbeat() |
There was a problem hiding this comment.
I spent way to long looking for a '. operator in the cadence language docs only to find out there is something wrong with the fonts:
There was a problem hiding this comment.
Where is this screenshot from? GitHub? VS Code? It's probably not a problem with the PR though?
There was a problem hiding this comment.
This is github on firefox. No not an issue in the PR. Just some sort of font issue somewhere.
| self.adminStoragePath = /storage/accountV2MigrationAdmin | ||
| self.nextAddressStartIndex = 1 | ||
| self.batchSize = 0 | ||
|
|
||
| self.account.storage.save( | ||
| <-create Admin(), | ||
| to: self.adminStoragePath | ||
| ) |
There was a problem hiding this comment.
This is really small, but the batching logic could probably live in the Migration contract instead of in the V2 one
There was a problem hiding this comment.
See #6761 (comment): The idea is that all logic related to the particular migration, the "account v2 migration", lives in AccountV2Migration, and Migration only delegates.
If we add the logic of the "account v2 migration" to the Migration contract, we won't be able to reuse it for other purposes, as the contract update rules e.g. don't allow removing enums, etc.
Sorry, I'm not quite following what's being suggested.
The idea for the If another migration needs to be added, we can reuse the Why should
Agreed, we should not use it directly, only through the Happy to have another sync to discuss this further |
The thing that threw me off was the reference to Would it be possible to have Yes maybe lets just quickly sync. |
|
@janezpodhostnik re: moving bootstrapping of the I guess we could deploy an "no-op" |
Depends on #6779
Start work on the migration logic for onflow/cadence#3584
AccountV2Migration, which is deployed to the service account during bootstrapping. ItsAdminresource is able to trigger the migration for the next batch of accounts to be migrated, by calling the injectedscheduleAccountV2Migrationfunctionfun scheduleAccountV2Migration(addressStartIndex: UInt64, count: UInt64): Boolto the new contractAccountV2Migrationin the service account. It generates the addresses in the given range, and schedules the migration to the new account storage format V2. It essentially just calls the internal Go functionruntime.Storage.ScheduleV2Migration()provided by Cadence.