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

Upgrade to Polkadot-v1.7.2 #1833

Merged
merged 78 commits into from
Jun 7, 2024
Merged

Upgrade to Polkadot-v1.7.2 #1833

merged 78 commits into from
Jun 7, 2024

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented May 9, 2024

Description

Checklist:

  • Upgraded all dependencies to polkadot-sdk with version 1.7.2.
  • cargo check & cargo test using runtime-benchmarks feature
    • libs
    • pallets
      • anchors
      • block-rewards
      • bridge
      • collator-allowlist
      • ethereum-transaction
      • fees
      • foreign-investments
      • interest-accrual
      • investments
      • keystore
      • liquidity-pools
      • liquidity-pools-gateway
        • liquidity-pools-gateway-routers
        • axelar-gateway-precompile
      • liquidity-rewards
      • loans
      • oracle-collection
      • oracle-feed
      • order-book
      • permissions
      • pool-fees
      • pool-registry
      • pool-system
      • restricted-tokens
      • restricted-xtokens
      • rewards
      • swaps
      • token-mux
      • transfer-allowlist
    • runtimes
      • common
      • development
      • altair
      • centrifuge
    • integration-tests
    • node
      • Currently using some deprecated code. This needs to be fixed.
  • use latest fudge
  • check all required patched dependencies are patched
  • check real benchmarks
  • check try-runtime
  • check docker files
  • modify CI and scripts
  • Evaluate if we need to perform migrations from 3parties
  • Check node usage
  • Check any missing TODO in the PR
  • Ensure cargo update using latest polkadot

@lemunozm lemunozm added Q7-very-hard Can be done by an expert coder with a deep knowledge of the codebase. I9-release A specific release. labels May 9, 2024
@lemunozm lemunozm requested review from cdamian and wischli May 9, 2024 10:55
@lemunozm lemunozm self-assigned this May 9, 2024
@lemunozm lemunozm marked this pull request as draft May 9, 2024 10:57
@lemunozm lemunozm marked this pull request as ready for review May 10, 2024 06:47
@lemunozm lemunozm marked this pull request as draft May 10, 2024 06:47
@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2024

There is a last test failing. It seems like the HostConfiguration is not applied and I do not know why: https://github.com/centrifuge/centrifuge-chain/actions/runs/9386407945/job/25846936686?pr=1833

As a fast-dirty-fix to make this work I did: 7e69e68

A better solution should be configuring the xcm part correctly. By now, I left this as debt-tech. We, anyway, need to clear and organize the 8K lines of liquidity pools, probably before starting the new features related to it

@lemunozm lemunozm marked this pull request as ready for review June 6, 2024 08:22
@lemunozm lemunozm requested a review from hieronx as a code owner June 6, 2024 08:22
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 5.44465% with 521 lines in your changes missing coverage. Please review.

Project coverage is 46.76%. Comparing base (2b4f94f) to head (d2072ce).

Files Patch % Lines
node/src/chain_spec.rs 0.00% 86 Missing ⚠️
...ntime/common/src/migrations/restricted_location.rs 0.00% 73 Missing ⚠️
runtime/common/src/migrations/hold_reason.rs 0.00% 51 Missing ⚠️
runtime/common/src/lib.rs 0.00% 47 Missing ⚠️
node/src/service.rs 0.00% 40 Missing ⚠️
pallets/restricted-tokens/src/impl_fungibles.rs 26.19% 31 Missing ⚠️
node/src/service/evm.rs 0.00% 26 Missing ⚠️
runtime/altair/src/lib.rs 0.00% 18 Missing ⚠️
runtime/centrifuge/src/lib.rs 0.00% 18 Missing ⚠️
runtime/common/src/transfer_filter.rs 0.00% 18 Missing ⚠️
... and 18 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1833      +/-   ##
==========================================
+ Coverage   46.70%   46.76%   +0.06%     
==========================================
  Files         169      169              
  Lines       13203    13169      -34     
==========================================
- Hits         6166     6159       -7     
+ Misses       7037     7010      -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lemunozm and others added 4 commits June 6, 2024 17:25
* migration for restricted transfer location

* add correct location for centrifuge

* minor changes

* fix benchmarks

* fix IT compilation

* chore: extend v1.7.2 transfer location migration (#1858)

* feat: add pre + post-upgrade

* feat: add nuking migration for dev and demo

* fix: remove account list for demo

* refactor: Use `VersionedLocation` in `RestrictedTransferLocation` (#1861)

* feat: add pre + post-upgrade

* feat: add nuking migration for dev and demo

* fix: remove account list for demo

* refactor: use VersionedLocation

* fix: transfer allowlist benchmarks + tests

* fix: clippy

* fix: benchmark cli

* fix: revert tight coupling

* fmt

* fix IT compilation

---------

Co-authored-by: William Freudenberger <w.freude@icloud.com>
@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2024

@william could you double check the last non-checked point in the PR description? If considered checked, I think we can merge this!

@@ -23,6 +23,7 @@ pub type UpgradeAltair1035 = (
runtime_common::migrations::increase_storage_version::Migration<OrderBook, 0, 1>,
runtime_common::migrations::increase_storage_version::Migration<ForeignInvestments, 0, 1>,
pallet_collator_selection::migration::v1::MigrateToV1<crate::Runtime>,
pallet_collator_selection::migration::v2::MigrationToV2<crate::Runtime>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have 2 migrations here? Out of curisoity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both needed. The first one is sorting just our invulnerables and pushing storage version from 0 to 1, the second one translates the storage entry and bumps 1 to 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this migraiton at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

See above

runtime/altair/src/xcm.rs Show resolved Hide resolved
runtime/centrifuge/src/xcm.rs Show resolved Hide resolved
type SubscriptionService = PolkadotXcm;
type Trader = Trader;
type TransactionalProcessor = FrameTransactionalProcessor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lemunozm have you looked into the codebase what this type is doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
@@ -1010,6 +1056,7 @@ parameter_types! {

// periods between treasury spends
pub const SpendPeriod: BlockNumber = 14 * DAYS;
pub const PayoutPeriod: BlockNumber = 30 * DAYS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Config value taken from?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its a new parameter based on Polkadot et al. config: #1833 (comment)

runtime/altair/src/lib.rs Show resolved Hide resolved
Comment on lines +68 to +81
pallet_balances::Holds::<T>::translate::<OldHolds, _>(|who, holds| {
if Self::account_can_be_migrated(&who, &transfer_allowlist_accounts) {
log::info!("{LOG_PREFIX} Migrating hold for account {who:?}");
let id_amount = IdAmount::<RuntimeHoldReason, Balance> {
id: HoldReason::TransferAllowance.into(),
// Non-emptiness ensured above
amount: holds[0].amount,
};
weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1));

Some(NewHolds::truncate_from(vec![id_amount]))
} else {
None
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope that one is not too heavy. How many entries do we have again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just two on production luckily. They can only emerge from transfer allowlist for native currency. For more info, see here #1849 (comment)

@wischli
Copy link
Contributor

wischli commented Jun 7, 2024

@william could you double check the last non-checked point in the PR description? If considered checked, I think we can merge this!

You linked a stranger :) Sorry for the confusing Github name. It's a reference to my birth name. I am ensuring a Docker image can be build and then all checks are done!

Only thing left at this point is re-adding the DMP Queue for the lazy migration.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 7, 2024

Only thing left at this point is re-adding the DMP Queue for the lazy migration.

Doing it now

mustermeiszer
mustermeiszer previously approved these changes Jun 7, 2024
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Awesome work everyone! So happy we can merge this bad boy 🤘

@lemunozm lemunozm enabled auto-merge (squash) June 7, 2024 11:16
@lemunozm lemunozm merged commit ccbe93d into main Jun 7, 2024
11 of 12 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. Q7-very-hard Can be done by an expert coder with a deep knowledge of the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants