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

Release v0.10.34 #1573

Merged
merged 23 commits into from Oct 4, 2023
Merged

Release v0.10.34 #1573

merged 23 commits into from Oct 4, 2023

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Sep 27, 2023

Description

LPs

The following changes affect all runtimes:

  • Removes StumpInboundQueue and sets LiquidityPools as InboundQueue implementer
  • Removes filter on OutboundQueue
  • Removes UpdateTrancheInvestmentLimit message
  • Moves configuration of LP related pallets to new liquidity_pools module

USDC

The following changes only affect centrifuge runtime:

  • Adds three more LP wrapped USDC variants
  • Enables bidirectional OrderBook trading pairs for all four LP wrapped USDC variants to/from DOT native USDC

Runtime upgrade

  • Bumps spec and toml versions
  • Updates weights

Misc

  • Enables benchmarks for some missing pallets
    • Altair: Elections, PolkadotXcm, CumulusXcmpQueue, LiquidityRewards
    • Centrifuge: PolkadotXcm, LiquidityRewards
  • Fixes minor issue with pallet-loans benchmark for apply_transfer_debt which did not factor in a paremeter as defined in the corresponding WeightInfo

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

@wischli
Copy link
Contributor Author

wischli commented Sep 28, 2023

/benchmark centrifuge

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

✅ Uploaded benchmarks for: centrifuge
Find the artifact here: https://github.com/centrifuge/centrifuge-chain/actions/runs/6334700805

@wischli wischli self-assigned this Sep 28, 2023
@wischli wischli added the I9-release A specific release. label Sep 28, 2023
@wischli
Copy link
Contributor Author

wischli commented Sep 28, 2023

/benchmark centrifuge

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

✅ Uploaded benchmarks for: centrifuge
Find the artifact here: https://github.com/centrifuge/centrifuge-chain/actions/runs/6337391897

@wischli
Copy link
Contributor Author

wischli commented Sep 28, 2023

FYI: Need to benchmark again, because the benchmarks were missing #1465 which introduced a breaking weights change. Moreover, the Centrifuge runtime did not have pallet_xcm benchmarks enabled, such that these were not generated.

@@ -38,7 +39,7 @@ libs/traits/src/changes.rs @lemunozm
libs/traits/src/data.rs @lemunozm

## Changes to runtime
runtime/common/* @mustermeiszer @NunoAlexandre @hieronx @lemunozm
runtime/common/* @mustermeiszer @NunoAlexandre @hieronx @lemunozm @wischli
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope it's fine I have added myself here.

@wischli wischli marked this pull request as ready for review September 28, 2023 14:29
Comment on lines +481 to +484
let n in 2..Helper::<T>::max_active_loans() - 2;

let any = account("any", 0, 0);
let pool_id = Helper::<T>::prepare_benchmark();
let pool_id = Helper::<T>::initialize_active_state(n);
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 I noticed that the loans benchmarks did parametrize apply_transfer_debt which created compiler issues as the pallet_loans::weights::WeightInfo expect the existence of a parameter. AFAICT, this provided solution should be appropriate. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

My fault! You did the perfect fix for this! 💯

Thanks, and sorry for missing this

Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the work on this @wischli !! 🚀

type TrancheId = TrancheId;
type TrancheTokenPrice = PoolSystem;
type TreasuryAccount = TreasuryAccount;
type WeightInfo = ();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have fear when I see the () type as it seems as if it's using Zero weights. Maybe as a way of confirming the reading the weights have actually a value, we could name the type as DefensiveWeights, and use it here.

type TokenSwapOrderId = u64;
type TokenSwaps = OrderBook;
type TrancheId = TrancheId;
type WeightInfo = ();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should WeightInfo be removed if there are no extrinsics associated with this pallet?

libs/types/src/tokens.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +69 to +75
pallet_pool_system::Pool::<Runtime>::mutate(ANEMOY_POOL_ID, |maybe_details| {
if let Some(details) = maybe_details {
details.currency = CURRENCY_ID_DOT_NATIVE;
log::info!("anemoy_pool::Migration: currency set to USDC ✓");
} else {
log::warn!("anemoy_pool::Migration: Pool details empty, skipping migration");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment: This would have bricked Catalyst due to unwrapping a None variant. We need to be more attentive of never unwrapping outside of tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the catch, @wischli. This is my bad but also reveals the necessity for a more formal review process and review requirements. In this case, requiring a migration on Centrifuge to be tested against Centrifuge AND Catalyst before being approved, whereas we only tested against Centrifuge.

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

LGTM!


parameter_types! {
pub const MaxIncomingMessageSize: u32 = 1024;
pub Sender: AccountId = GatewayAccountProvider::<Runtime, LocationToAccountId>::get_gateway_account();
}

impl pallet_liquidity_pools_gateway::Config for Runtime {
type AdminOrigin = EnsureRoot<AccountId>;
type InboundQueue = crate::LiquidityPools;
type AdminOrigin = EnsureRootOr<HalfOfCouncil>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why half of council in dev? No blocker, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy paste error. I had this file removed at some point. Would it be fine to fix this in a post-release PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course. Root is still enabled so no problem.

}

impl pallet_liquidity_pools::Config for Runtime {
// NOTE: No need to adapt that. The Router is an artifact and will be removed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Artifact?

Copy link
Contributor Author

@wischli wischli Sep 29, 2023

Choose a reason for hiding this comment

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

Artifact indeed, sorry about that. Can I remove this in a post-release PR? CI takes ages as each push builds new images.

Next release I need to apply another branching strategy, i.e. have an rc branch which wants to merge into release-vx.y.z which is based on main after feature freeze. This way, we don't need to rebase the rc branch on any non release related push to main. However, merging the release branch into main still creates CI overhead but I suppose that's fine.

@wischli wischli merged commit e53b5e8 into main Oct 4, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I9-release A specific release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants