Skip to content

Conversation

@Krayt78
Copy link
Contributor

@Krayt78 Krayt78 commented Dec 19, 2025

Summary

This PR migrates pallet-referenda from the deprecated Currency trait with anonymous reserves to the modern fungible traits with typed holds, bumping the storage version from v1 to v2. This is part of the umbrella task #226.

Changes

Config Updates

  • Replaced Currency with NativeBalance using the new fungible traits:
    • fungible::Inspect
    • fungible::Mutate
    • fungible::Balanced
    • fungible::InspectHold
    • fungible::MutateHold
  • Added HoldReason::DecisionDeposit composite enum for tracking deposit holds with a specific reason

Storage Migration (v1 → v2)

Added MigrateV1ToV2<T, I, OldCurrency> migration that:

  1. Iterates through all existing referenda (Ongoing, Approved, Rejected, Cancelled, TimedOut)
  2. For each deposit (submission and decision deposits):
    • Unreserves the old anonymous reserved balance using the legacy ReservableCurrency trait
    • Places a new hold with HoldReason::DecisionDeposit using the new MutateHold trait
  3. Updates storage version from 1 to 2

The migration gracefully handles edge cases:

  • Skips execution if not on version 1
  • Logs warnings if unreserve is incomplete
  • Logs errors if hold placement fails
  • Properly handles Killed referenda (no deposits to migrate)
  • Includes try-runtime hooks for pre/post upgrade verification

Runtime Integration

To use this migration in your runtime:

pub type Migrations = (
    pallet_referenda::migration::v2::MigrateV1ToV2<Runtime, (), Balances>,
);

Testing

Added comprehensive test coverage:

  • migration_v1_to_v2_works - Tests full migration flow with multiple referendum types (Ongoing, Approved, TimedOut)
  • migration_v1_to_v2_skips_if_not_v1 - Verifies migration is skipped when not on version 1
  • migration_v1_to_v2_handles_killed_referendum - Tests handling of killed referenda with no deposits

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: Use /cmd label <label-name> to add labels
    • Maintainers can also add labels manually
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@Krayt78 Krayt78 requested a review from a team as a code owner December 19, 2025 15:47
@Krayt78
Copy link
Contributor Author

Krayt78 commented Dec 19, 2025

/cmd label T2-pallets

@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot added the T2-pallets This PR/Issue is related to a particular pallet. label Dec 19, 2025
@Krayt78
Copy link
Contributor Author

Krayt78 commented Dec 19, 2025

There is a runtime type mismatch now
The Slash type now expects OnUnbalanced<DebtOf<Self, I>> where DebtOf is a fungible Debt (not the old NegativeImbalance).

This means we need to migrate treasury first to use Fungible before this one.

@gui1117
Copy link
Contributor

gui1117 commented Jan 3, 2026

There is a runtime type mismatch now The Slash type now expects OnUnbalanced<DebtOf<Self, I>> where DebtOf is a fungible Debt (not the old NegativeImbalance).

This means we need to migrate treasury first to use Fungible before this one.

That mean we need to upgrade both together, because otherwise pallet-referenda won't support pallet-treasury for slash destination.

IMO we can make treasury Currency type bind both Currency and Fungible and implement both OnUnbalanced<Debt> and OnUnbalanced<NegativeImbalance>. Because pallet-balances support both traits so it is compatible.

>;
/// Currency type for this pallet.
type Currency: ReservableCurrency<Self::AccountId>;
type NativeBalance: Inspect<Self::AccountId>
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO better to keep the same old name Currency. It is still a kind of currency type, the underlying traits are fungible but this is fine. Most users won't see the difference as pallet-balances implements both. And if it breaks the compile error will be fine.

&HoldReason::<I>::DecisionDeposit.into(),
&who,
amount,
Precision::Exact,
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use Precision::BestEffort, we don't want to prevent a kill from happening because the slash is failing for some reason.

Comment on lines +637 to +638
Self::slash_deposit(Some(status.submission_deposit.clone()))?;
Self::slash_deposit(status.decision_deposit.clone())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like that this can return an error here. We don't want to block the kill if for some reason the slash fails. Also fungible API never really specifies why it could fail. In the other comment I say to change to BestEffort to make this scenario less likely but still strictly speaking it doesn't prevent the slash from failing, so maybe better to actually ignore the error.

Suggested change
Self::slash_deposit(Some(status.submission_deposit.clone()))?;
Self::slash_deposit(status.decision_deposit.clone())?;
let _ = Self::slash_deposit(Some(status.submission_deposit.clone())).inspect(/* log some error or something */);
let _ = Self::slash_deposit(status.decision_deposit.clone()).inspect(/* log some error or something */);

Comment on lines +267 to +270
if on_chain_version != 1 {
log::warn!(target: TARGET, "skipping migration from v1 to v2.");
return weight;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use versioned migration to factorize out those checks: e.g.

pub type MigrateV15ToV16<T> = VersionedMigration<
15,
16,
VersionUncheckedMigrateV15ToV16<T>,
Pallet<T>,
<T as frame_system::Config>::DbWeight,
>;

let mut migrated_deposits = 0u32;

for (index, info) in v1::ReferendumInfoFor::<T, I>::iter() {
weight.saturating_accrue(T::DbWeight::get().reads(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it fine to do like this? T::DbWeight::get().reads(1) only returns the ref_time weight and ignores the proof_size weight.

for (index, info) in v1::ReferendumInfoFor::<T, I>::iter() {
weight.saturating_accrue(T::DbWeight::get().reads(1));

let deposits_to_migrate = Self::collect_deposits(&info);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no way for an attacker to start submitting small deposits and make this too big, isn't it? otherwise we should do a multi block migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T2-pallets This PR/Issue is related to a particular pallet.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants