Improve deposits migration #1636

AurevoirXavier · 2024-11-29T14:12:23Z

Closes #1635.

Signed-off-by: Xavier Lau <[email protected]>

hackfisher · 2024-12-02T03:08:07Z

runtime/darwinia/src/migration.rs

@@ -86,5 +87,11 @@ fn migrate() -> frame_support::weights::Weight {
 		]),
 	);

-	<Runtime as frame_system::Config>::DbWeight::get().reads_writes(r, w + 5)
+	let _ = Balances::transfer_all(
+		RuntimeOrigin::signed(PalletId(*b"dar/depo").into_account_truncating()),


The is still some CRAB left in deposit pallet account on Crab chain.

https://crab-scan.darwinia.network/address/0x6d6F646C6461722F6465706F0000000000000000

We also need to do the transfer migration for CRAB to clean it.

I haven't noticed anything from a few days back. This might be due to the account migration.

pallet/deposit/src/lib.rs

hackfisher · 2024-12-02T03:26:50Z

@AurevoirXavier CI failed

pallet/deposit/src/lib.rs

Signed-off-by: Xavier Lau <[email protected]>

hackfisher · 2024-12-02T03:42:50Z

pallet/deposit/src/lib.rs

@@ -216,14 +213,13 @@ pub mod pallet {
 			}

 			if to_claim.0 != 0 {
-				T::Ring::transfer(&account_id(), who, to_claim.0, AllowDeath)?;
+				T::Ring::transfer(&T::Treasury::get(), who, to_claim.0, AllowDeath)?;


Also emit failure event here if error.

hackfisher · 2024-12-02T03:46:37Z

runtime/crab/src/migration.rs

@@ -52,6 +52,11 @@ fn migrate() -> frame_support::weights::Weight {
 	}



Please remove previous migration which have been done in last upgrade.

Signed-off-by: Xavier Lau <[email protected]>

hackfisher · 2024-12-02T05:35:06Z

pallet/deposit/src/lib.rs

+				{
+					Self::deposit_event(Event::MigrationFailedOnRefund);
+
+					Err(e)?;


Do not throw errors, otherwise there are still side effects l.

hackfisher · 2024-12-02T05:36:04Z

pallet/deposit/src/lib.rs

 				Self::deposit_event(Event::DepositsClaimed {
 					owner: who.clone(),
 					deposits: to_claim.1,
 				});
 			}
 			if to_migrate.0 != 0 {
-				T::Ring::transfer(&account_id(), &T::Treasury::get(), to_migrate.0, AllowDeath)?;
 				T::DepositMigrator::migrate(who.clone(), to_migrate.0, to_migrate.2)?;


Should not throw errors here right?

In FP, we treat any I/O operation as the side effect.

And here, especially we interact with the DB, take has already introduced the side effects. We should keep the error and utilize it to help rollback the state.

You didn't get what I mean.

If keep the error, this batch (10 items) will be inserted again, but some of them might have been migrated, then they will be keep migrated again and again. You didn't do the rollback entirely outside (re-insert is not rollback), so just giveup rollup and record it here.

(It OK as we can always adopt migrate by script if there is.)

Then we can remove the event and insert the failure items into a different storage, and process that storage manually later. Using storage is easier to track and process compare to event. (Event is also a storage, same cost)

Considering that you are on vacation and the likelihood of errors is very low, I suggest we keep things simple. At the same time, we must absolutely avoid any ledger inaccuracies or double-spending in case an error does occur.

Then we can remove the event and insert the failure items into a different storage, and process that storage manually later. Using storage is easier to track and process compare to event.

If this looks simpler for you, It works for me. (Just need me take another several minutes to review the difference between them, good so far.)

Signed-off-by: Xavier Lau <[email protected]>

github-actions · 2024-12-03T06:39:37Z

Check 6a6eaf9 crab-dev

Check runtime version

Check runtime storage

github-actions · 2024-12-03T06:39:37Z

Check 6a6eaf9 darwinia-dev

Check runtime version

Check runtime storage

github-actions · 2024-12-03T06:39:42Z

Check 6a6eaf9 koi-dev

Check runtime version

Check runtime storage

Pallet DarwiniaStaking
+ Entry: StorageEntryMetadata { name: "UnissuedReward", modifier: Default, ty: Plain(UntrackedSymbol { id: 6, marker: PhantomData<fn() -> core::any::TypeId> }), default: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], docs: [" Unissued reward to Treasury."] }

Pallet Deposit
+ Entry: StorageEntryMetadata { name: "MigrationFailures", modifier: Optional, ty: Map { hashers: [Blake2_128Concat], key: UntrackedSymbol { id: 0, marker: PhantomData<fn() -> core::any::TypeId> }, value: UntrackedSymbol { id: 368, marker: PhantomData<fn() -> core::any::TypeId> } }, default: [0], docs: [" Failures of migration.", "", " The first value is the deposits that failed to migrate,", " the second value is the type of failure."] }

AurevoirXavier added C-Pallet [Component] Something about pallet C-Runtime [Component] Something about runtime labels Nov 29, 2024

github-actions bot assigned AurevoirXavier Nov 29, 2024

Improve deposits migration

7a91c09

Signed-off-by: Xavier Lau <[email protected]>

AurevoirXavier force-pushed the x/fix branch from f410124 to 7a91c09 Compare November 29, 2024 14:14

AurevoirXavier added 2 commits November 30, 2024 10:36

Fix

9469224

Fix

df9ebc8

This comment has been minimized.

Sign in to view

hackfisher reviewed Dec 2, 2024

View reviewed changes

pallet/deposit/src/lib.rs Show resolved Hide resolved

hackfisher reviewed Dec 2, 2024

View reviewed changes

pallet/deposit/src/lib.rs Outdated Show resolved Hide resolved

Transfer all

754e22b

Signed-off-by: Xavier Lau <[email protected]>

AurevoirXavier force-pushed the x/fix branch from ae93ef0 to 754e22b Compare December 2, 2024 03:36

Deposit exit reason event

0c8e69a

hackfisher reviewed Dec 2, 2024

View reviewed changes

Add event for migration failure on refund

ac3f0a3

Signed-off-by: Xavier Lau <[email protected]>

AurevoirXavier force-pushed the x/fix branch from dba0972 to ac3f0a3 Compare December 2, 2024 03:48

Remove old migration

4bdbf50

Signed-off-by: Xavier Lau <[email protected]>

hackfisher reviewed Dec 2, 2024

View reviewed changes

Add missing items

9349685

This comment has been minimized.

Sign in to view

Record failures on automation tasks

6a6eaf9

Signed-off-by: Xavier Lau <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve deposits migration #1636

Improve deposits migration #1636

AurevoirXavier commented Nov 29, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hackfisher Dec 2, 2024

AurevoirXavier Dec 2, 2024

hackfisher commented Dec 2, 2024

hackfisher Dec 2, 2024

hackfisher Dec 2, 2024

hackfisher Dec 2, 2024

hackfisher Dec 2, 2024

AurevoirXavier Dec 2, 2024 •

edited

Loading

hackfisher Dec 2, 2024 •

edited

Loading

AurevoirXavier Dec 2, 2024

hackfisher Dec 2, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 3, 2024

		@@ -52,6 +52,11 @@ fn migrate() -> frame_support::weights::Weight {
		}

Improve deposits migration #1636

Are you sure you want to change the base?

Improve deposits migration #1636

Conversation

AurevoirXavier commented Nov 29, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackfisher commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurevoirXavier Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

hackfisher Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 3, 2024

AurevoirXavier commented Nov 29, 2024 •

edited

Loading

AurevoirXavier Dec 2, 2024 •

edited

Loading

hackfisher Dec 2, 2024 •

edited

Loading