fix: migrations for v2.3.0 #627
Conversation
DylanVerstraete
requested review from
brandonpille,
renauter,
robvanmieghem and
LeeSmet
as code owners
| @@ -1,4 +1,5 @@ | |||
| use crate::*; | |||
| use alloc::collections::BTreeMap; | |||
There was a problem hiding this comment.
why was this added?
There was a problem hiding this comment.
I first added the code in V15. That's why I had to import it here but I'll remove it. Do we still need v15 and v8? Or can we remove it as it is in runtime now.
substrate-node/runtime/src/lib.rs
Outdated
| pallet_tfgrid::migrations::v15::MigrateTwinsV15<Runtime>, | ||
| pallet_smart_contract::migrations::v8::FixTwinLockedBalances<Runtime>, | ||
| migrations::tfgrid_v15_smart_contract_v8::Migrate<Runtime>, | ||
| //pallet_tfgrid::migrations::v15::MigrateTwinsV15<Runtime>, |
There was a problem hiding this comment.
no need to comment, only keep the required one
| } else { | ||
| info!(" >>> Unused TFGrid pallet V15 migration"); | ||
| } | ||
| if pallet_smart_contract::PalletVersion::<T>::get() |
There was a problem hiding this comment.
I guess pallet_tfgrid::PalletVersion::<T>::get() == pallet_tfgrid::types::StorageVersion::V14Struct is also required here, so combine the if?
| } | ||
| // Update pallet storage version | ||
| pallet_tfgrid::PalletVersion::<T>::set(pallet_tfgrid::types::StorageVersion::V15Struct); | ||
| info!(" <<< Twin migration success, storage version upgraded ({:?} twins)", pallet_tfgrid::Twins::<T>::iter().count()); |
There was a problem hiding this comment.
remove count in the info log, this log will always run and impact the migration execution duration
| // If the twin needs to have some locked balance on his account because of running contracts | ||
| // Check how much we can actually lock based on his current total balance | ||
| // this will make sure the locked balance will not exceed the total balance on the twin's account | ||
| let should_lock = twin_contract_locked_balances.get(&twin_id).map(|b| { |
There was a problem hiding this comment.
Maybe be we should not consider the twins without locked balance before doing this?
I mean in case of twin_contract_locked_balances.get(&twin_id) == None
Not sure if all twins have contracts and locked balances...
There was a problem hiding this comment.
We fill the twin_contract_locked_balances map before this step and we only add an element if the twin exists. So the None case will not happen.
There was a problem hiding this comment.
Yes, I get this
But in the loop for (twin_id, twin) in twins { ... all the twins are considered right?
There was a problem hiding this comment.
Was expecting something like for twin_id in twin_contract_locked_balances.keys() { ... instead
But maybe I miss something
There was a problem hiding this comment.
Yes but we do a get, if the get doesn't find the key it will return None directly and then we don't do anything with it (see the if let some(should_lock)). The map part after the get is only executed if the get returned something.
There was a problem hiding this comment.
Was expecting something like
for twin_id in twin_contract_locked_balances.keys() { ...instead But maybe I miss something
Ah like that. I don't know if this part should happen for all twins or only for the twins that have a contract. @DylanVerstraete?
There was a problem hiding this comment.
Great, I see now
Thanks for the explanation of the return None directly part ;)
| }); | ||
|
|
||
| // Unlock all balance for the twin | ||
| <T as Config>::Currency::remove_lock(GRID_LOCK_ID, &twin.account_id); |
There was a problem hiding this comment.
Same idea here
So all twins have a lock?
There was a problem hiding this comment.
I'm not sure here. We do add the lock whenever we bill something but in case nothing was ever billed I don't know. Does it even matter? Maybe @DylanVerstraete knows.
There was a problem hiding this comment.
Maybe putting this part inside the if let Some(should_lock) = should_lock { block ?
remove lock only when we want to set lock again
There was a problem hiding this comment.
could do that yes
* feat: rework documentation (#571) * doc: service consumer contract flow (#567) * fix: farming policies map ids (#591) * chore: reset runtime migrations * feat: delete twin is not allowed (#586) * fix: typos (#595) * fix: locked balances (#590) * feat: allow twin to connect to a relay (#570) * chore: bump to version 2.3.0-rc1 (specVersion: 124) * feat: add node power (#592) * chore: update ADR docs * chore: bump to version 2.3.0-rc2 (specVersion: 125) * chore: rework logging pallet smart contract * fix: allow numbers in relay addresses * chore: bump to version 2.3.0-rc3 (specVersion: 125) * fix: migration V15 for devnet #598 * chore: bump to version 2.3.0-rc4 (specVersion: 127) * chore: restore V15 migration * chore: update dev chain spec * fix: soften serial number validation (#603) * chore: set max length of serial number to 128 bytes * chore: align tft bridge dependency * chore: update default dev chainspec * chore: revert default dev bridge validators chainspec * Drop unused structopt dependency This also removes some unused child dependencies Signed-off-by: Lee Smet <lee.smet@hotmail.com> * Set the substrate node default chart image to ghcr.io * fix: rework balances migrations * chore: refactor * feat: also remove contract and lock if twin is deleted * chore: add writes * chore: resolve comment * chore: fix ci * chore: resolve pr comments * chore: add log * Update build_test.yaml to use the new runner (#617) * chore: remove serial valdiation (#616) * chore: bump to version 2.3.0-rc6 (specVersion: 129) * chore: rework chart #620 * fix: discount calculation handle variable billing frequency (#626) * fix: migrations for v2.3.0 (#627) * chore: bump to version 2.3.0-rc7 (specVersion: 130) * fix: soften node city/country validation * chore: bump to version 2.3.0-rc8 (specVersion: 131) * chore: update cargo lock * chore: bump version to 2.3.0 --------- Signed-off-by: Lee Smet <lee.smet@hotmail.com> Co-authored-by: Erwan Renaut <73958772+renauter@users.noreply.github.com> Co-authored-by: omahs <73983677+omahs@users.noreply.github.com> Co-authored-by: Lee Smet <lee.smet@hotmail.com> Co-authored-by: Rob Van Mieghem <robvanmieghem@gmail.com> Co-authored-by: Brandon <brandon@threefold.tech>
No description provided.