Skip to content
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

migration: Add LP Gateway migration for outbound messages #1946

Merged
merged 4 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions runtime/altair/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("altair"),
impl_name: create_runtime_str!("altair"),
authoring_version: 1,
spec_version: 1400,
spec_version: 1401,
impl_version: 1,
apis: RUNTIME_API_VERSIONS,
transaction_version: 2,
Expand Down Expand Up @@ -2066,7 +2066,7 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
migrations::UpgradeAltair1400,
migrations::UpgradeAltair1401,
>;

// Frame Order in this block dictates the index of each one in the metadata
Expand Down
5 changes: 4 additions & 1 deletion runtime/altair/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

use crate::Runtime;

/// The migration set for Altair @ Kusama.
/// It includes all the migrations that have to be applied on that chain.
pub type UpgradeAltair1400 = ();
pub type UpgradeAltair1401 =
runtime_common::migrations::liquidity_pools_gateway::Migration<Runtime>;
4 changes: 2 additions & 2 deletions runtime/centrifuge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("centrifuge"),
impl_name: create_runtime_str!("centrifuge"),
authoring_version: 1,
spec_version: 1400,
spec_version: 1401,
impl_version: 1,
apis: RUNTIME_API_VERSIONS,
transaction_version: 2,
Expand Down Expand Up @@ -2077,7 +2077,7 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
migrations::UpgradeCentrifuge1400,
migrations::UpgradeCentrifuge1401,
>;

// Frame Order in this block dictates the index of each one in the metadata
Expand Down
5 changes: 4 additions & 1 deletion runtime/centrifuge/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

use crate::Runtime;

/// The migration set for Centrifuge @ Polkadot.
/// It includes all the migrations that have to be applied on that chain.
pub type UpgradeCentrifuge1400 = ();
pub type UpgradeCentrifuge1401 =
runtime_common::migrations::liquidity_pools_gateway::Migration<Runtime>;
127 changes: 127 additions & 0 deletions runtime/common/src/migrations/liquidity_pools_gateway.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
use cfg_types::domain_address::Domain;
use frame_support::{
pallet_prelude::ValueQuery,
storage::{child::MultiRemovalResults, generator::StorageValue, unhashed},
storage_alias,
traits::{Get, OnRuntimeUpgrade},
Blake2_128Concat, StorageMap,
};
use pallet_liquidity_pools_gateway::{pallet::Pallet as LPGateway, Config};
use pallet_order_book::weights::Weight;
use sp_runtime::{DispatchError, TryRuntimeError};
use sp_std::vec::Vec;

#[storage_alias]
pub type OutboundMessageNonceStore<T: Config> =
StorageValue<pallet_liquidity_pools_gateway::Pallet<T>, u64, ValueQuery>;

#[storage_alias]
pub type OutboundMessageQueue<T: Config + frame_system::Config> = StorageMap<
pallet_liquidity_pools_gateway::Pallet<T>,
Blake2_128Concat,
u64,
(
Domain,
<T as frame_system::Config>::AccountId,
pallet_liquidity_pools::Message,
),
>;

#[storage_alias]
pub type FailedOutboundMessages<T: Config + frame_system::Config> = StorageMap<
pallet_liquidity_pools_gateway::Pallet<T>,
Blake2_128Concat,
u64,
(
Domain,
<T as frame_system::Config>::AccountId,
pallet_liquidity_pools::Message,
DispatchError,
),
>;

pub struct Migration<T>(sp_std::marker::PhantomData<T>);

impl<T> OnRuntimeUpgrade for Migration<T>
where
T: Config + frame_system::Config,
{
fn on_runtime_upgrade() -> Weight {
let mut weight = Self::clear_storage(
OutboundMessageNonceStore::<T>::storage_prefix(),
Copy link
Contributor

@wischli wischli Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only represents the b"OutboundMessageNonceStore" but is missing the pallet prefix. The solution is to always use the final storage key for clearing.

Suggested change
OutboundMessageNonceStore::<T>::storage_prefix(),
OutboundMessageNonceStore::<T>::storage_value_final_key(),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought the storage_alias takes care of that.

"OutboundMessageNonceStore",
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative, and my preference, would be just utilizing OutboundMessageNonceStore::<T>::kill() here as it is a simple storage value.


for (nonce, entry) in OutboundMessageQueue::<T>::iter() {
log::warn!("Found outbound message:\nnonce:{nonce}\nentry:{entry:?}");
weight = weight.saturating_add(<T as frame_system::Config>::DbWeight::get().reads(1));
}

for (nonce, entry) in FailedOutboundMessages::<T>::iter() {
log::warn!("Found failed outbound message:\nnonce:{nonce}\nentry:{entry:?}");
weight = weight.saturating_add(<T as frame_system::Config>::DbWeight::get().reads(1));
}

weight
}

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
// Extra check to confirm that the storage alias is correct.
assert!(
OutboundMessageNonceStore::<T>::get() > 0,
"OutboundMessageNonce should be > 0"
);

assert_eq!(
OutboundMessageQueue::<T>::iter_keys().count(),
0,
"OutboundMessageQueue should be empty!"
);

assert_eq!(
FailedOutboundMessages::<T>::iter_keys().count(),
0,
"FailedOutboundMessages should be empty!"
);

Ok(Vec::new())
}

#[cfg(feature = "try-runtime")]
fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
assert_eq!(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lemunozm this is the assert that fails when running try-runtime.

Not sure where that panic comes from tho, that's a different story.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the error it tells you? "OutboundMessageNonceStore should be 0!" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with the latest version that's what I get in the post check. But, I guess that makes sense since the on_runtime_upgrade doesn't really get triggered or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm.. I think the assert should pass IIUC this 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous message was failing correctly because "there is" entry if you read. But if you check the value it should be the default one, 0

OutboundMessageNonceStore::<T>::get(),
0,
"OutboundMessageNonceStore should be 0!"
);

assert_eq!(
OutboundMessageQueue::<T>::iter_keys().count(),
0,
"OutboundMessageQueue should be empty!"
);

assert_eq!(
FailedOutboundMessages::<T>::iter_keys().count(),
0,
"FailedOutboundMessages should be empty!"
);

Ok(())
}
}

impl<T: Config + frame_system::Config> Migration<T> {
fn clear_storage(prefix: &[u8], storage_name: &str) -> Weight {
let res = unhashed::clear_prefix(prefix, None, None);

match res.maybe_cursor {
None => log::info!("{storage_name} was cleared"),
Some(_) => log::error!("{storage_name} was not completely cleared"),
};

<T as frame_system::Config>::DbWeight::get()
.reads_writes(res.loops.into(), res.unique.into())
}
}
1 change: 1 addition & 0 deletions runtime/common/src/migrations/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

pub mod hold_reason;
pub mod increase_storage_version;
pub mod liquidity_pools_gateway;
pub mod loans;
pub mod nuke;
pub mod precompile_account_codes;
Expand Down
4 changes: 2 additions & 2 deletions runtime/development/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("centrifuge-devel"),
impl_name: create_runtime_str!("centrifuge-devel"),
authoring_version: 1,
spec_version: 1400,
spec_version: 1401,
impl_version: 1,
apis: RUNTIME_API_VERSIONS,
transaction_version: 2,
Expand Down Expand Up @@ -2168,7 +2168,7 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
crate::migrations::UpgradeDevelopment1400,
crate::migrations::UpgradeDevelopment1401,
>;

// Frame Order in this block dictates the index of each one in the metadata
Expand Down
5 changes: 4 additions & 1 deletion runtime/development/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

pub type UpgradeDevelopment1400 = ();
use crate::Runtime;

pub type UpgradeDevelopment1401 =
runtime_common::migrations::liquidity_pools_gateway::Migration<Runtime>;