Fix: LP accounting #1657

mustermeiszer · 2023-12-20T08:41:09Z

Description

Currently, when tokens are transfered to another domain we are storing them in the domain specific account. That is correct for tranche tokens, as Centrifuge Chain is the reserve for these kind of tokens. But all other tokens that are LP transferable are tokens where the domain is the reserve, hence we need to burn them instead.

This PR fixes the code to burn instead and furthermore provides a migration for Centrifuge Chain to burn existing tokens.

NOTE: We must burn all LP tokens from this account, as we are now also deployed on Celo, Arbitrum and Base we must be cautious whether we need to extend the burn-migration to USDC from these domains.

Changes and Descriptions

use burn_from instead of transfer
migration for burning from Domain account

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

mustermeiszer · 2023-12-20T08:55:15Z

The migration itself works, but I still get a panic. @wischli do you have a clue what could be the issue here:

./target/debug/centrifuge-chain try-runtime --runtime target/debug/wbuild/centrifuge-runtime/centrifuge_runtime.wasm --chain centrifuge on-runtime-upgrade live --uri wss://fullnode.centrifuge.io:443
2023-12-20 09:53:24 replacing wss:// in uri with https://: "https://fullnode.centrifuge.io:443" (ws is currently unstable for fetching remote storage, for more see https://github.com/paritytech/jsonrpsee/issues/1086)    
2023-12-20 09:53:25 since no at is provided, setting it to latest finalized head, 0xd01de95547bc8a8f7b3d12ea3c79baea47032e0b41291da58cefbb2beb29a3cf    
2023-12-20 09:53:25 since no prefix is filtered, the data for all pallets will be downloaded    
⠋     0.001 s	Scraping keys...2023-12-20 09:53:25 scraping key-pairs from remote at block height 0xd01de95547bc8a8f7b3d12ea3c79baea47032e0b41291da58cefbb2beb29a3cf    
✅ Found 110677 keys (10.92s)
[00:00:15] ✅ Downloaded key values 6,919.3176/s [===================================================] 110677/110677 (0s)
✅ Inserted keys into DB (2.29s)
2023-12-20 09:53:55 adding data for hashed prefix: , took 29.86s    
2023-12-20 09:53:55 adding data for hashed key: 3a636f6465    
2023-12-20 09:53:55 adding data for hashed key: 26aa394eea5630e07c48ae0c9558cef7f9cce9c888469bb1a0dceaa129672ef8    
2023-12-20 09:53:55 adding data for hashed key: 26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac    
2023-12-20 09:53:55 initialized state externalities with storage root 0xe00376ae7ef8000060883d964d49c62c46efff2ce33db7bfbb23b1823629bec4 and state_version V0    
2023-12-20 09:53:56 original spec: RuntimeString::Owned("centrifuge")-1023, code hash: cd393115b86484068d173d2b582f843e5f7c15f42d4026470316ce9a64d07701    
2023-12-20 09:53:56 new spec: RuntimeString::Owned("centrifuge")-1024, code hash: e4e36ed1e31caba35f97190f016f9932617b4b8aa226af7fac0785ef490ab464    
2023-12-20 09:54:11 BurnUnburnedMigration: , AccountData of Ethereum domain account has free balance of: 500026000000    
2023-12-20 09:54:11 BurnUnburnedMigration:  Successfully burned 500026000000 LP_ETH_USDC from Ethereum domain account    
2023-12-20 09:54:11 BurnUnburnedMigration:  Migration successfully finished.    
2023-12-20 09:54:11 ⚠️ ParachainSystem declares internal migrations (which *might* execute). On-chain `StorageVersion(2)` vs current storage version `StorageVersion(2)`    
2023-12-20 09:54:11 Balances: On chain storage version StorageVersion(0) doesn't match current storage version StorageVersion(1).    
2023-12-20 09:54:11 ⚠️ XcmpQueue declares internal migrations (which *might* execute). On-chain `StorageVersion(3)` vs current storage version `StorageVersion(2)`    
2023-12-20 09:54:11 XcmpQueue: On chain storage version StorageVersion(3) doesn't match current storage version StorageVersion(2).    
2023-12-20 09:54:11 ⚠️ PolkadotXcm declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`    
2023-12-20 09:54:11 ⚠️ DmpQueue declares internal migrations (which *might* execute). On-chain `StorageVersion(2)` vs current storage version `StorageVersion(1)`    
2023-12-20 09:54:11 DmpQueue: On chain storage version StorageVersion(2) doesn't match current storage version StorageVersion(1).    
2023-12-20 09:54:11 OrmlAssetRegistry: On chain storage version StorageVersion(0) doesn't match current storage version StorageVersion(2).    
2023-12-20 09:54:11 PriceOracleMembership: On chain storage version StorageVersion(0) doesn't match current storage version StorageVersion(4).    
2023-12-20 09:54:11 ⚠️ Ethereum declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `NoStorageVersionSet`    
2023-12-20 09:54:11 Detected multiple errors while executing `try_on_runtime_upgrade`:    
2023-12-20 09:54:11 Other("On chain and current storage version do not match. Missing runtime upgrade?")    
2023-12-20 09:54:11 Other("On chain and current storage version do not match. Missing runtime upgrade?")    
2023-12-20 09:54:11 Other("On chain and current storage version do not match. Missing runtime upgrade?")    
2023-12-20 09:54:11 Other("On chain and current storage version do not match. Missing runtime upgrade?")    
2023-12-20 09:54:11 Other("On chain and current storage version do not match. Missing runtime upgrade?")    
2023-12-20 09:54:11 panicked at /Users/frederikgartenmeister/Projects/centrifuge-chain/runtime/centrifuge/src/lib.rs:2623:65:
called `Result::unwrap()` on an `Err` value: Other("Detected multiple errors while executing `try_on_runtime_upgrade`, check the logs!")    
Error: Input("failed to execute TryRuntime_on_runtime_upgrade: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed\nWASM backtrace:\nerror while executing at wasm backtrace:\n    0: 0x679d37 - <unknown>!rust_begin_unwind\n    1: 0x5d1693 - <unknown>!core::panicking::panic_fmt::hae09c37b9301b789\n    2: 0x5d2272 - <unknown>!core::result::unwrap_failed::h22afadfa28b8acf5\n    3: 0x25d31 - <unknown>!<centrifuge_runtime::Runtime as frame_try_runtime::runtime_decl_for_try_runtime::TryRuntimeV1<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,fp_self_contained::unchecked_extrinsic::UncheckedExtrinsic<sp_runtime::multiaddress::MultiAddress<<<sp_runtime::MultiSignature as sp_runtime::traits::Verify>::Signer as sp_runtime::traits::IdentifyAccount>::AccountId,()>,centrifuge_runtime::RuntimeCall,sp_runtime::MultiSignature,(frame_system::extensions::check_non_zero_sender::CheckNonZeroSender<centrifuge_runtime::Runtime>,frame_system::extensions::check_spec_version::CheckSpecVersion<centrifuge_runtime::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<centrifuge_runtime::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<centrifuge_runtime::Runtime>,frame_system::extensions::check_mortality::CheckMortality<centrifuge_runtime::Runtime>,frame_system::extensions::check_nonce::CheckNonce<centrifuge_runtime::Runtime>,frame_system::extensions::check_weight::CheckWeight<centrifuge_runtime::Runtime>,pallet_transaction_payment::ChargeTransactionPayment<centrifuge_runtime::Runtime>)>>>>::on_runtime_upgrade::h27052cd5a4e75c12\n    4: 0x28087d - <unknown>!TryRuntime_on_runtime_upgrade\nnote: using the `WASMTIME_BACKTRACE_DETAILS=1` environment variable may show more debugging information")

wischli

Two minor nits and one logical error in the post checks. Unfortunately, some integration tests will very likely fail which requires minor fixing.

wischli · 2023-12-20T08:57:56Z

runtime/centrifuge/src/migrations.rs

+				);
+			}
+
+			T::DbWeight::get().reads(1)


The weight is off. Unfortunately, we don't have weights for burn_from. Looking at the source code, 2 reads + 2 writes from mutating the account and the total issuance seem feasible.

Blocker: Weight is still off

Suggested change

T::DbWeight::get().reads(1)

T::DbWeight::get().reads_writes(2, 2)

wischli · 2023-12-20T08:59:49Z

runtime/centrifuge/src/migrations.rs

+			if !post_data.free.is_zero()
+				&& !post_data.frozen.is_zero()
+				&& !post_data.reserved.is_zero()


Wrong logical concatenation: Should be !(free == 0 && frozen == 0 && reserve == 0) or respectively!free == 0 || !frozen == 0 || !reserved == 0.

wischli · 2023-12-20T09:00:27Z

runtime/centrifuge/src/migrations.rs

+			);
+
+			if !pre_data.frozen.is_zero() && !pre_data.reserved.is_zero() {
+				log::error!("AccountData of Ethereum domain account has non free balances...");


Nit: Since this is only executed in try-runtime, it would be feasible to panic here.

lemunozm · 2023-12-20T09:03:22Z

runtime/centrifuge/src/migrations.rs

+		T: orml_tokens::Config + frame_system::Config,
+		<T as orml_tokens::Config>::CurrencyId: From<CurrencyId>,


NIT: Could be the above be

Suggested change

T: orml_tokens::Config + frame_system::Config,

<T as orml_tokens::Config>::CurrencyId: From<CurrencyId>,

T: orml_tokens::Config<CurrencyId = CurrencyId> + frame_system::Config,

to avoid into()s?

lemunozm · 2023-12-20T09:05:34Z

runtime/centrifuge/src/migrations.rs

+				T::CurrencyId::from(LP_ETH_USDC),
+			);
+
+			if !pre_data.frozen.is_zero() && !pre_data.reserved.is_zero() {


I am not sure if I'm right, don't you want instead

Suggested change

if !pre_data.frozen.is_zero() && !pre_data.reserved.is_zero() {

if !pre_data.frozen.is_zero() || !pre_data.reserved.is_zero() {

?

lemunozm · 2023-12-20T09:06:39Z

runtime/centrifuge/src/migrations.rs

+			if !post_data.free.is_zero()
+				&& !post_data.frozen.is_zero()
+				&& !post_data.reserved.is_zero()


Same here, with ||?

lemunozm · 2023-12-20T09:07:53Z

runtime/centrifuge/src/migrations.rs

+				<Domain as Convert<_, T::AccountId>>::convert(ETH_DOMAIN),
+				T::CurrencyId::from(LP_ETH_USDC),
+			);
+


Should we check here also frozen and reserved?

To be safe, we should not burn if either of these entries are non-zero, IMO.

lemunozm · 2023-12-20T09:08:32Z

runtime/centrifuge/src/migrations.rs

+				);
+			}
+
+			T::DbWeight::get().reads(1)


And 1 writing, I think

lemunozm · 2023-12-20T09:11:17Z

Practically overwriting the @william suggestions 😆

wischli · 2023-12-20T10:18:25Z

The migration itself works, but I still get a panic. @wischli do you have a clue what could be the issue here:

As discussed synchronously, the panic is unrelated to this migration as I also experienced it with an empty migration tuple when testing the 1024 upgrade. I suppose these panics were suppressed (only logged) before our update to v0.9.43.

lemunozm

Second round! Some questions, but I think everything is tied 👍🏻

pallets/liquidity-pools/src/lib.rs

runtime/centrifuge/src/lib.rs

runtime/centrifuge/src/migrations.rs

wischli

The migration weight was not updated. Apart from that LGTM!

wischli · 2023-12-21T09:55:30Z

libs/types/src/tokens.rs

-	/// The asset is transferable through all available options
-	All(XcmMetadata),


Q: Curious about this change. Another feature creep?

wischli · 2023-12-21T09:56:26Z

runtime/centrifuge/src/migrations.rs

+				);
+			}
+
+			T::DbWeight::get().reads(1)


Blocker: Weight is still off

Suggested change

T::DbWeight::get().reads(1)

T::DbWeight::get().reads_writes(2, 2)

mustermeiszer · 2023-12-21T10:09:26Z

I will leave that for now. Runtime upgrades block the whole block anyways, and we probably only going to have loans migration besides this one.

wischli

Approving due to expected underweight of migrations for 1025

mustermeiszer added 2 commits December 19, 2023 14:41

fix: bruning instead of transferring

b473882

feat: Migration for burning unburned tokens from domain account

9fd154e

mustermeiszer changed the title ~~Fix/lp accounting~~ Fix: LP accounting Dec 20, 2023

mustermeiszer requested review from wischli, hieronx and lemunozm December 20, 2023 08:46

mustermeiszer added 2 commits December 20, 2023 09:57

fix: better docs

37c6ec8

fix: dangerous to have both, only allow one

c1308ba

wischli reviewed Dec 20, 2023

View reviewed changes

lemunozm reviewed Dec 20, 2023

View reviewed changes

mustermeiszer added 5 commits December 20, 2023 11:20

fix: build and add suggestions

5dc7ca7

fix: try-runtime imports

9247eaf

fix: finaaaaallly

547f181

fix: treasury paying for update, rm only admin

ad43788

chore: bumb runtime version

d1e2286

mustermeiszer requested a review from NunoAlexandre as a code owner December 20, 2023 10:59

lemunozm reviewed Dec 20, 2023

View reviewed changes

pallets/liquidity-pools/src/lib.rs Show resolved Hide resolved

runtime/centrifuge/src/lib.rs Show resolved Hide resolved

runtime/centrifuge/src/migrations.rs Show resolved Hide resolved

mustermeiszer added 2 commits December 20, 2023 14:46

fix: integration tests lp transfer

88ed2e1

fix: integration update_tranceh_token_metadata

43e9a11

wischli added this to the Centrifuge 1025 milestone Dec 20, 2023

wischli added the D8-migration Pull request touches storage and needs migration code. label Dec 20, 2023

fix: transfer and burn correctly

6ae9e12

wischli reviewed Dec 21, 2023

View reviewed changes

mustermeiszer enabled auto-merge (squash) December 21, 2023 10:02

wischli approved these changes Dec 21, 2023

View reviewed changes

mustermeiszer merged commit 3191ed1 into main Dec 21, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: LP accounting #1657

Fix: LP accounting #1657

mustermeiszer commented Dec 20, 2023

mustermeiszer commented Dec 20, 2023

wischli left a comment

wischli Dec 20, 2023

wischli Dec 21, 2023

wischli Dec 20, 2023

wischli Dec 20, 2023

lemunozm Dec 20, 2023

lemunozm Dec 20, 2023

lemunozm Dec 20, 2023

lemunozm Dec 20, 2023

wischli Dec 20, 2023

lemunozm Dec 20, 2023

lemunozm commented Dec 20, 2023

wischli commented Dec 20, 2023

lemunozm left a comment

wischli left a comment

wischli Dec 21, 2023

wischli Dec 21, 2023

mustermeiszer commented Dec 21, 2023

wischli left a comment

	T::DbWeight::get().reads(1)
	T::DbWeight::get().reads_writes(2, 2)

		T: orml_tokens::Config + frame_system::Config,
		<T as orml_tokens::Config>::CurrencyId: From<CurrencyId>,

	T: orml_tokens::Config + frame_system::Config,
	<T as orml_tokens::Config>::CurrencyId: From<CurrencyId>,
	T: orml_tokens::Config<CurrencyId = CurrencyId> + frame_system::Config,

	if !pre_data.frozen.is_zero() && !pre_data.reserved.is_zero() {
	if !pre_data.frozen.is_zero() \|\| !pre_data.reserved.is_zero() {

		/// The asset is transferable through all available options
		All(XcmMetadata),

Fix: LP accounting #1657

Fix: LP accounting #1657

Conversation

mustermeiszer commented Dec 20, 2023

Description

Changes and Descriptions

Checklist:

mustermeiszer commented Dec 20, 2023

wischli left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Dec 20, 2023

wischli commented Dec 20, 2023

lemunozm left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer commented Dec 21, 2023

wischli left a comment

Choose a reason for hiding this comment