Skip to content

Add migration for rescaling time frames after block time change - #1455

Merged
Chralt98 merged 9 commits into
mainfrom
chralt98-add-pm-timing-migrations
Nov 28, 2025
Merged

Add migration for rescaling time frames after block time change#1455
Chralt98 merged 9 commits into
mainfrom
chralt98-add-pm-timing-migrations

Conversation

@Chralt98

@Chralt98 Chralt98 commented Nov 26, 2025

Copy link
Copy Markdown

What does it do?

Add migration for rescaling time frames after block time change.

try-runtime-zeitgeist-output.txt
try-runtime-battery-station-output.txt

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

Summary by CodeRabbit

  • Refactor

    • Adjusted executive wiring to change how single-block migrations are provided.
  • New Features

    • Added a multi-block time-frame rescale migration with stateful, weighted stepping and runtime upgrade hooks.
    • Exposed migration scaffold and added a flag to track rescale completion; storage version bumped to 9.
  • Bug Fixes / Improvements

    • Increased migration cursor and identifier size limits in non-benchmark builds.
  • Tests

    • Added tests for migration progression, state handling, and version bumping.
  • Chore

    • Removed explicit package manifest overrides.

✏️ Tip: You can customize this high-level summary in your review settings.

@Chralt98 Chralt98 self-assigned this Nov 26, 2025
@Chralt98 Chralt98 added the s:in-progress The pull requests is currently being worked on label Nov 26, 2025
@coderabbitai

coderabbitai Bot commented Nov 26, 2025

Copy link
Copy Markdown

Walkthrough

Rewires Executive migrations typing; adds a public multi-step TimeFrameRescaleMigration (mbm) with cursored, weight‑bounded migration and try-runtime hooks; bumps prediction-markets storage version to 9; enlarges migration cursor/identifier sizes in two runtimes; appends the new migration to runtime lists; removes pnpm overrides.

Changes

Cohort / File(s) Summary
Runtime Executive config
runtime/common/src/lib.rs
Adjusted Executive type parameter to use () for the migrations parameter; changed frame_system::Config::SingleBlockMigrations wiring.
Prediction‑markets: migrations & hooks
zrml/prediction-markets/src/migrations.rs, zrml/prediction-markets/src/lib.rs
Added public mbm module with TimeFrameRescaleMigration<T>, Cursor, constants, stepped multi-block migration with cursored progress and weight accounting, try-runtime pre_upgrade/post_upgrade/on_runtime_upgrade, new LastTimeFrameRescaled storage, and bumped STORAGE_VERSION to 9; updated imports to expose migrations.
Runtime migration lists
runtime/.../runtime_migrations/mod.rs
runtime/battery-station/src/runtime_migrations/mod.rs, runtime/zeitgeist/src/runtime_migrations/mod.rs
Imported and appended TimeFrameRescaleMigration<crate::Runtime> into LegacyMigrations tuples.
Migration cursor / identifier sizes
runtime/battery-station/src/lib.rs, runtime/zeitgeist/src/lib.rs
Increased non-benchmark MigrationsCursorMaxLen (8 → 64) and MigrationsIdentifierMaxLen (8 → 128).
Integration tests manifest
integration-tests/package.json
Removed the pnpm.overrides block, leaving only the package version entry.
Migration tests (mock‑gated)
zrml/prediction-markets/src/migrations.rs
Added mock-enabled tests validating migration progression, cursor transitions, storage-version bump, and MarketId movements across time frames.

Sequence Diagram

sequenceDiagram
    actor Scheduler as Block/Runtime
    participant Migration as TimeFrameRescaleMigration
    participant Cursor as MigrationCursor (storage)
    participant Storage as RuntimeStorage

    Scheduler->>Migration: invoke on_runtime_upgrade / run_step(weight)
    activate Migration

    Migration->>Cursor: load or init cursor state
    Cursor-->>Migration: {current_time_frame, offset, max_source_time_frame}
    Migration->>Storage: read source time-frame buckets & MarketIds
    Storage-->>Migration: return batch data

    loop while weight remains and work left
        Migration->>Storage: move/insert MarketIds to target frames (bounded batch)
        Migration->>Storage: remove/mark processed source keys
        Migration->>Cursor: advance offset / mark frame complete
    end

    alt incomplete
        Migration->>Storage: persist Cursor state
        Migration-->>Scheduler: return InProgress (consumed weight)
    else complete
        Migration->>Storage: set STORAGE_VERSION = 9
        Migration-->>Scheduler: return Completed
    end

    deactivate Migration
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Review focus:
    • Cursor persistence and recovery across steps
    • Weight accounting, batch boundaries, and idempotency
    • Correctness of MarketId moves and cleanup
    • Integration of STORAGE_VERSION bump into runtime migration tuples
    • try-runtime hooks and new tests

Suggested labels

s:accepted

Suggested reviewers

  • robhyrk
  • saboonikhil

Poem

🐰 I hopped through code with nimble paws,
I shifted frames without a pause ⏳,
The cursor kept each careful trace,
Market IDs found their new place,
Storage bumped — a tidy upgrade's cause!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically summarizes the primary change: adding a migration to rescale time frames after a block time change, which is the main objective of the PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chralt98-add-pm-timing-migrations

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
zrml/prediction-markets/src/migrations.rs (3)

32-38: Tie TIME_FRAME_SCALE_FACTOR explicitly to this specific block-time change

TIME_FRAME_SCALE_FACTOR is computed via integer division of PREVIOUS_MILLISECS_PER_BLOCK by MILLISECS_PER_BLOCK and then used to rescale LastTimeFrame. That works for the current “12s → 6s” halving, but:

  • If MILLISECS_PER_BLOCK changes again, this constant will silently become wrong, and the migration would mis‑scale or collapse to 0.
  • The migration is also implicitly assuming an integer scaling factor.

I’d strongly suggest either:

  • Making this migration clearly one‑off (documenting and guarding it with a check that the current MILLISECS_PER_BLOCK yields the expected ratio), or
  • Adding a debug/assert check that TIME_FRAME_SCALE_FACTOR equals the intended value (e.g., 2) before doing any rescaling, so it fails loudly if reused in a different context.

Also applies to: 73-83


85-100: insert_with_shift can be expensive and is invisible to the WeightMeter

insert_with_shift performs an unbounded loop over successive time frames, doing storage get/insert calls until it finds a non‑full bucket. From the perspective of the migration weight accounting:

  • The outer step loop only budgets db_weight.reads_writes(3, 3) per item before calling insert_with_shift, but this helper may perform multiple reads/writes if it has to walk through several full buckets.
  • Those extra DB ops are not reflected in the WeightMeter, so the migration may under‑account weight and do more work per block than intended (not a consensus bug, but it can stress block execution during migration).

If you expect many time frames near capacity, consider:

  • Either charging a more conservative weight per item (e.g., assume a small bounded number of shifts), or
  • Pushing the bucket shifting into the main step loop where you can account for each additional get/insert via weight_or_insufficient.

226-269: Tests cover only the simplest happy path; consider adding partial-step and multi-bucket cases

The current test (migration_rescales_time_frames) nicely validates:

  • Storage version bump from 8 → 9,
  • Rescaling of LastTimeFrame,
  • Moving a single time frame key with one market ID.

To better lock in migration correctness, it would be valuable to add tests that:

  • Run with a tight WeightMeter limit to force at least one InsufficientWeight path, ensuring cursor persistence (current_time_frame, offset) works correctly over multiple step calls.
  • Cover multiple time frames and multiple markets per frame (including where buckets are at or near CacheSize), validating that all buckets are eventually migrated and removed.
  • Include at least one scenario where some keys should not be migrated (e.g., explicitly simulating “post‑upgrade” bucket(s)) if that’s part of the intended behavior.

These would make it much easier to refactor the stepping logic safely in the future.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f56828c and afe76ac.

📒 Files selected for processing (2)
  • runtime/common/src/lib.rs (2 hunks)
  • zrml/prediction-markets/src/migrations.rs (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (1)
runtime/common/src/lib.rs (1)

135-142: Executive/SingleBlockMigrations wiring looks correct; please confirm all intended migrations are covered

Switching Executive’s last generic to () and routing single‑block migrations via frame_system::Config::SingleBlockMigrations = SingleBlockMigrations matches the newer Substrate migration pattern with pallet_migrations handling multi‑block upgrades. Just make sure:

  • Every single‑block migration you still expect to run on version changes is included in the SingleBlockMigrations alias.
  • The new time‑frame rescale migration is wired into your MultiBlockMigrations tuple so it actually executes via pallet_migrations.

Also applies to: 538-571

Comment thread zrml/prediction-markets/src/migrations.rs Outdated
Comment thread zrml/prediction-markets/src/migrations.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
integration-tests/package.json (1)

20-41: Consider documenting the purpose of the pnpm overrides reorganization.

The pnpm overrides block has been reorganized to explicitly pin Polkadot packages alongside other dependencies. While the current structure is clear, adding a brief comment or ensuring related Rust-side changes are well-documented would help future maintainers understand why these specific versions are required in tandem with the migration refactoring.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between afe76ac and 99c4c29.

⛔ Files ignored due to path filters (1)
  • integration-tests/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • integration-tests/package.json (2 hunks)
  • zrml/prediction-markets/src/migrations.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • zrml/prediction-markets/src/migrations.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (1)
integration-tests/package.json (1)

34-40: Based on my verification, I now have sufficient information to rewrite the review comment. Let me provide the updated version:


Polkadot package versions are compatible and appropriate for supporting the block time protocol change.

The pnpm overrides pin @polkadot/api@16.5.2 and @polkadot/util@13.5.2, which are documented as compatible by polkadot-js. This version split (core packages at 16.5.2; utility packages at 13.5.2) aligns with the TimeFrameRescaleMigration in zrml/prediction-markets/src/migrations.rs, which rescales time frames following the AsyncBacking upgrade that halved block time from 12s to 6s. The v16.5.2 upgrade includes metadata v16 changes required to support this protocol change.

The integration tests (runtime upgrade and smoke tests) verify basic functionality after the upgrade, though they do not explicitly test the migration behavior itself. Consider adding a focused integration test that validates the TimeFrameRescaleMigration completes successfully and correctly rescales storage values if migration-specific verification is needed.

@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Nov 27, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
zrml/prediction-markets/src/migrations.rs (1)

104-155: Concurrent writes can still cause new markets to be rescaled as “legacy” (double‑scaling risk)

The migration now:

  • Snapshots max_source_time_frame once via iter_keys().max() (lines 138–140),
  • Uses next_start_time_frame(max) which picks any key <= max from iter_keys() each time (lines 104–108, 153–155),
  • Processes and removes each such key while the migration runs across multiple blocks.

MarketIdsPerCloseTimeFrame is also written by normal pallet logic (insert_auto_close / manually_close_market) without any guard on the pallet storage version. While the stepped migration is in progress, new markets created by extrinsics can insert entries whose time frame key is <= max_source_time_frame (e.g., if existing markets already reach very far into the future). Those new buckets will then be picked up by next_start_time_frame(max) and rescaled as if they were legacy entries, effectively pushing their close time frame further into the future than intended.

This is the same underlying concurrency hazard that was raised in an earlier review: the current changes (iterating over keys and snapshotting max_source_time_frame) address iterator‑ordering issues but do not fully isolate pre‑migration “source space” from new inserts.

Possible hardening options:

  • Gate writes to MarketIdsPerCloseTimeFrame on StorageVersion::<Pallet<T>>() >= TARGET_STORAGE_VERSION (or the inverse) inside the pallet’s insertion paths, or
  • Use a stricter notion of the legacy range (e.g., a stored “max legacy frame” written once at upgrade time) and ensure new markets always land strictly above that boundary, or
  • Explicitly document and enforce at the runtime level that no markets are created while this migration is active.
#!/bin/bash
# Inspect all write sites for MarketIdsPerCloseTimeFrame to see if any are gated
rg -n "MarketIdsPerCloseTimeFrame::<" zrml/prediction-markets/src/lib.rs -n -C3
🧹 Nitpick comments (2)
zrml/prediction-markets/src/migrations.rs (2)

73-102: Weight‑bounded insert_with_shift trades safety for resilience; consider whether silent drops are acceptable

insert_with_shift now:

  • Tries at most 3 adjacent time frames to fit a market_id into a bounded vec,
  • Charges read/write weights per attempt,
  • Logs an error and returns Ok(()) if all three buckets are full.

Functionally this means that in the (very unlikely) case that three consecutive TimeFrame buckets are saturated, that market id is silently omitted from any close‑time cache; the migration will still “succeed” and bump the storage version.

If you want stronger guarantees, consider returning SteppedMigrationError::Failed instead of swallowing the failure so the network clearly sees the migration as incomplete, or at least emit a prominent comment documenting that under extreme contention some markets may lose their auto‑close entry during this migration.


222-263: Tests nicely cover happy‑path rescaling but miss weight‑limited stepping

The two tests validate:

  • LastTimeFrame is rescaled once via Pallet::on_runtime_upgrade,
  • All buckets and IDs are moved from old to new time frames at the expected scale,
  • Storage version is bumped from 8 to 9 only after the stepped migration completes.

Given the non‑trivial weight‑metering logic, it would be worth adding at least one unit test that exercises the “insufficient weight” branch (tiny weight limit, enough IDs to require multiple steps) to catch regressions in cursor/offset handling and partial writes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8da0c19 and f719c99.

📒 Files selected for processing (2)
  • zrml/prediction-markets/src/lib.rs (4 hunks)
  • zrml/prediction-markets/src/migrations.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
zrml/prediction-markets/src/migrations.rs (1)
zrml/prediction-markets/src/lib.rs (5)
  • market_id (2086-2086)
  • Self (1945-1949)
  • StorageVersion (1879-1879)
  • StorageVersion (1897-1897)
  • on_runtime_upgrade (1876-1893)
zrml/prediction-markets/src/lib.rs (1)
zrml/prediction-markets/src/migrations.rs (2)
  • StorageVersion (236-236)
  • StorageVersion (268-268)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (3)
zrml/prediction-markets/src/lib.rs (3)

37-71: Imports and new try‑runtime dependencies look consistent with migration usage

Pulling migrations into scope alongside weights and gating Decode/Encode/TryRuntimeError under try-runtime is idiomatic and matches how post_upgrade is written. No issues here.


89-91: Storage version bump aligns with new migration target

Updating STORAGE_VERSION to 9 matches TARGET_STORAGE_VERSION in migrations::mbm and the tests, so the pallet’s declared version is consistent with the stepped migration.


1875-1918: Migration wiring verified across all runtimes; no issues found

The verification confirms the original review comment is accurate. Both active runtimes (zeitgeist and battery-station) have TimeFrameRescaleMigration properly wired into their migration pipelines via the LegacyMigrations tuple, which is assigned to the MultiBlockMigrations type used by pallet_migrations. The on_runtime_upgrade guard and try-runtime validation hooks are correctly implemented, and the version bump is appropriately delegated to TimeFrameRescaleMigration::step().

Comment thread zrml/prediction-markets/src/migrations.rs
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:review-needed The pull request requires reviews labels Nov 27, 2025
@mergify mergify Bot added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Nov 27, 2025
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Nov 27, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
zrml/prediction-markets/src/migrations.rs (1)

145-229: Stepping logic now avoids order‑dependence and cursor bugs; re‑check invariants for new markets during migration

The new snapshotting (max_source_time_frame = iter_keys().max()) plus next_start_time_frame(max) no longer relies on StorageMap iteration order, and the updated continuation branch (resetting offset to 0 when persisting the tail) fixes the earlier bug where some IDs could be skipped when resuming mid‑bucket. Overall the multi‑block stepping logic looks sound.

One thing to double‑check at the runtime level: do we guarantee that any new timestamp‑based markets created while this migration is running will always land in time frames strictly greater than max_source_time_frame? If not, those new entries (with keys <= max_source_time_frame) will also be treated as “source” and rescaled, which may or may not be intended.

🧹 Nitpick comments (2)
zrml/prediction-markets/src/migrations.rs (2)

73-124: insert_with_shift silently drops auto‑close entry when all probed buckets are full

Bounding the shift loop to three adjacent time frames is reasonable, but if all three buckets are full you only log an error and return Ok(()), effectively losing the market’s auto‑close entry for this migration run.

If you consider that too lossy, you might instead return SteppedMigrationError::Failed (or at least surface a stronger signal via metrics/docs) so operators can detect and react to the data-loss risk rather than relying only on logs.


251-335: Tests cover basic paths; consider a bounded‑weight, multi‑step scenario

The two tests nicely exercise LastTimeFrame rescaling plus single‑ and multi‑bucket cache migration, but they always run with effectively unlimited weight, so the continuation path (cursor/offset when the meter runs out mid‑bucket) is never hit.

A small additional test that runs with a tight WeightMeter limit and repeatedly calls TimeFrameRescaleMigration::<Runtime>::step until it returns None would help guard against regressions in the multi‑step continuation logic.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f719c99 and 36cb5de.

📒 Files selected for processing (2)
  • zrml/prediction-markets/src/lib.rs (4 hunks)
  • zrml/prediction-markets/src/migrations.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
zrml/prediction-markets/src/migrations.rs (1)
zrml/prediction-markets/src/lib.rs (4)
  • Self (1951-1955)
  • StorageVersion (1879-1879)
  • StorageVersion (1903-1903)
  • on_runtime_upgrade (1876-1899)
zrml/prediction-markets/src/lib.rs (1)
zrml/prediction-markets/src/migrations.rs (2)
  • StorageVersion (254-254)
  • StorageVersion (286-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: Coverage
  • GitHub Check: Test standalone build
  • GitHub Check: Checks (standalone)
  • GitHub Check: Format
  • GitHub Check: Coverage
  • GitHub Check: Test parachain build
  • GitHub Check: Fuzz
  • GitHub Check: Checks (standalone)
  • GitHub Check: Checks (parachain)
  • GitHub Check: Test standalone build
  • GitHub Check: Checks (clippy)
  • GitHub Check: Quick check benchmarks
  • GitHub Check: Coverage
  • GitHub Check: Summary
🔇 Additional comments (2)
zrml/prediction-markets/src/lib.rs (2)

37-38: Imports and storage version bump are consistent with the new migration

Exposing migrations at the crate level and switching to use crate::{migrations, weights::*}; plus bumping STORAGE_VERSION to StorageVersion::new(9) line up with TARGET_STORAGE_VERSION: u16 = 9 in mbm and ensure the pallet’s metadata matches the migration you wired into the runtimes.

Also applies to: 56-61, 90-91


1875-1899: on_runtime_upgrade and try‑runtime hooks correctly isolate LastTimeFrame rescaling

The new on_runtime_upgrade gates on the on‑chain storage version, rescales LastTimeFrame once using migrations::mbm::TIME_FRAME_SCALE_FACTOR, and leaves the actual bump to version 9 to the multi‑block migration (which calls StorageVersion::new(9).put), matching the tests in migrations.rs. The pre_upgrade/post_upgrade pair under try-runtime cleanly captures whether the upgrade ran and asserts the expected rescaled LastTimeFrame.

From a pallet perspective this looks coherent; just ensure the Executive’s multi‑block migration wiring calls TimeFrameRescaleMigration in addition to this hook so version 9 is only considered “done” once the cache migration has completed.

Also applies to: 1901-1924

@codecov-commenter

codecov-commenter commented Nov 27, 2025

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 86.36364% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.28%. Comparing base (21c4b89) to head (49ddb63).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
zrml/prediction-markets/src/migrations.rs 86.53% 35 Missing ⚠️
zrml/prediction-markets/src/lib.rs 84.61% 4 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1455      +/-   ##
==========================================
- Coverage   93.32%   93.28%   -0.04%     
==========================================
  Files         181      181              
  Lines       34769    35049     +280     
==========================================
+ Hits        32448    32696     +248     
- Misses       2321     2353      +32     
Flag Coverage Δ
tests 93.28% <86.36%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mergify mergify Bot added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Nov 27, 2025
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Nov 27, 2025
@Chralt98
Chralt98 requested a review from robhyrk November 27, 2025 15:01

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
zrml/prediction-markets/src/migrations.rs (2)

73-117: Weight accounting in insert_with_shift is conservative and may double‑charge

The combination of:

  • outer meter.try_consume(db_weight.reads_writes(3, 3)) in step, and
  • inner weight_or_insufficient(meter, db_weight.reads(1)) / ...writes(1) inside insert_with_shift

means each migrated ID can be charged more weight than it actually consumes. That’s safe for correctness (no under‑charging) but can cause earlier‑than‑necessary InsufficientWeight errors and reduce progress per block.

If you want more predictable stepping, consider either:

  • dropping the outer reads_writes(3, 3) reservation and relying solely on the inner weight_or_insufficient calls, or
  • making insert_with_shift a pure storage op (no extra weight_or_insufficient) and letting the caller account all reads/writes.

254-395: Tests cover core flows; consider adding a targeted multi‑block/weight‑exhaustion test

The new tests exercise:

  • single‑bucket happy path (including StorageVersion and LastTimeFrame interactions),
  • the “target buckets full” hard‑failure case, and
  • multi‑bucket rescaling and on_runtime_upgrade idempotence.

What’s still untested is the mid‑bucket weight‑exhaustion path that persists a tail slice and resumes via the cursor. Given the previous bug in this area, it’s worth adding a small test that:

  1. Seeds a bucket with several IDs.
  2. Uses a very tight WeightMeter limit to force early return in the inner loop.
  3. Asserts that repeated step calls eventually migrate all IDs with none lost and that the source key is fully removed.

This would lock in the new cursor semantics and guard future refactors.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36cb5de and 49ddb63.

📒 Files selected for processing (2)
  • zrml/prediction-markets/src/lib.rs (5 hunks)
  • zrml/prediction-markets/src/migrations.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
zrml/prediction-markets/src/migrations.rs (1)
zrml/prediction-markets/src/lib.rs (4)
  • Self (1965-1969)
  • StorageVersion (1880-1880)
  • StorageVersion (1909-1909)
  • on_runtime_upgrade (1876-1905)
zrml/prediction-markets/src/lib.rs (1)
zrml/prediction-markets/src/migrations.rs (4)
  • StorageVersion (257-257)
  • StorageVersion (291-291)
  • StorageVersion (327-327)
  • StorageVersion (344-344)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: Checks (clippy)
  • GitHub Check: Format
  • GitHub Check: Coverage
  • GitHub Check: Test parachain build
  • GitHub Check: Checks (parachain)
  • GitHub Check: Format
  • GitHub Check: Fuzz
  • GitHub Check: Checks (clippy)
  • GitHub Check: Quick check benchmarks
  • GitHub Check: Test standalone build
  • GitHub Check: Checks (standalone)
  • GitHub Check: Summary
🔇 Additional comments (7)
zrml/prediction-markets/src/migrations.rs (2)

153-183: Iteration over MarketIdsPerCloseTimeFrame now correctly avoids ordering assumptions

The new snapshot logic:

  • max_source_time_frame = MarketIdsPerCloseTimeFrame::<T>::iter_keys().max();
  • next_start_time_frame(max) using iter_keys().find(|tf| *tf <= max)

drops the previous reliance on iter().last() and hash‑order monotonicity. Combined with key removal after processing, this guarantees every original bucket with tf <= max_source_time_frame is visited exactly once, independent of underlying storage ordering.

This is a solid fix to the earlier iterator‑ordering pitfall, and the code reads clearly with the updated comments.


192-212: Cursor continuation after weight exhaustion now preserves all remaining IDs

In the weight‑exhaustion branch you now:

  • write back remaining = ids[idx..].to_vec() to storage,
  • either remove the key or re‑insert the truncated bucket, and
  • reset state.current_time_frame and state.offset so the next step starts at index 0 of the persisted slice.

This removes the previous off‑by‑offset behavior where some markets at the front of the truncated slice could be silently skipped on resume. The continuation invariants for multi‑block runs look correct with this change.

zrml/prediction-markets/src/lib.rs (5)

37-38: Importing migrations at the pallet level aligns the runtime upgrade path

Switching to use crate::{migrations, weights::*}; cleanly exposes migrations::mbm::TIME_FRAME_SCALE_FACTOR to the pallet and keeps all migration‑specific constants encapsulated under crate::migrations. This keeps the pallet’s upgrade logic decoupled from the concrete migration implementation while still allowing the tests and hooks to reference the scale factor.


90-105: STORAGE_VERSION bump to 9 is consistent with the new migration target

STORAGE_VERSION: StorageVersion = StorageVersion::new(9); now matches TARGET_STORAGE_VERSION: u16 = 9 in migrations.rs, and the pallet is annotated with #[pallet::storage_version(STORAGE_VERSION)]. Together with the final step in TimeFrameRescaleMigration that writes StorageVersion 9, this keeps the versioning story coherent between the pallet and the stepped migration.


1874-1905: on_runtime_upgrade + LastTimeFrameRescaled cleanly separate LastTimeFrame scaling from the stepped migration

The new hook:

  • Guards on StorageVersion::get::<Self>() < STORAGE_VERSION so it only runs for the 8→9 upgrade.
  • Rescales LastTimeFrame once using migrations::mbm::TIME_FRAME_SCALE_FACTOR, with proper db‑weight accounting.
  • Sets LastTimeFrameRescaled::<T> as an idempotence flag.

With TimeFrameRescaleMigration later killing LastTimeFrameRescaled and bumping the pallet StorageVersion to 9, the responsibilities are nicely split:

  • pallet on_runtime_upgrade: rescale the single LastTimeFrame value and mark that it happened,
  • stepped migration: rescale the per‑time‑frame caches and finalize the upgrade.

The logic and weight handling here look sound.


1907-1938: Try‑runtime pre_upgrade / post_upgrade assertions correctly pin the expected LastTimeFrame behavior

The try‑runtime hooks:

  • Encode whether the upgrade should run (ran), the pre‑upgrade LastTimeFrame, and the pre‑upgrade LastTimeFrameRescaled flag.
  • In post_upgrade, short‑circuit if ran is false, otherwise:
    • assert the flag was not already set pre‑upgrade,
    • verify that LastTimeFrame was multiplied by TIME_FRAME_SCALE_FACTOR, and
    • ensure LastTimeFrameRescaled is now set.

This is a good, minimal invariant set for the LastTimeFrame portion of the migration and should catch mis‑ordering or missed updates in future changes to the runtime upgrade wiring.


2056-2060: Ephemeral LastTimeFrameRescaled storage item is used consistently and cleaned up after migration

Defining LastTimeFrameRescaled<T> as a StorageValue<_, bool, ValueQuery> with a getter, then:

  • setting it in on_runtime_upgrade, and
  • deleting it at the end of TimeFrameRescaleMigration::step

gives you a simple handshake flag between the single‑block LastTimeFrame rescale and the multi‑block cache migration. Once the migration completes, the flag is removed, so it doesn’t linger as dead state for new deployments.

This pattern looks appropriate for a one‑off migration marker.

@Chralt98 Chralt98 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Nov 28, 2025
@Chralt98
Chralt98 merged commit 20807c7 into main Nov 28, 2025
45 of 50 checks passed
@Chralt98
Chralt98 deleted the chralt98-add-pm-timing-migrations branch November 28, 2025 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s:accepted This pull request is ready for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants