Upgrade Moonbeam to Polkadot SDK stable2512#3633
Open
arturgontijo wants to merge 49 commits intomasterfrom
Open
Upgrade Moonbeam to Polkadot SDK stable2512#3633arturgontijo wants to merge 49 commits intomasterfrom
arturgontijo wants to merge 49 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkspace-wide migration to Polkadot/Substrate stable2512, API and weight signature updates (LazyBlock, parametric XCM weights), removal of explicit SelectCore/GetCoreSelectorApi usages, node service changes to pass collator_peer_id and wire tracing, JS/frontend patch, test expectation updates, and small CI/package tweaks. Changes
Sequence Diagram(s)mermaid Node->>Consensus: start_consensus(collator_peer_id) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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. Comment |
Contributor
Moonbase Weight Difference Report
Moonriver Weight Difference Report
Moonbeam Weight Difference Report
|
RomarQ
added
dependencies
Contributor
WASM runtime size check:Compared to target branchMoonbase runtime: 2064 KB (-36 KB) ✅ Moonbeam runtime: 2188 KB (-24 KB) ✅ Moonriver runtime: 2188 KB (-24 KB) ✅ Compared to latest release (runtime-4202)Moonbase runtime: 2064 KB (+128 KB compared to latest release) Moonbeam runtime: 2188 KB (+156 KB compared to latest release) Moonriver runtime: 2188 KB (+156 KB compared to latest release) |
arturgontijo
force-pushed
the
artur/moonbeam-polkadot-stable2512
branch
from
a13b535 to
ebc8ddd
Compare
# Conflicts: # Cargo.lock # test/suites/dev/moonbase/test-pov/test-precompile-over-pov2.ts
# Conflicts: # runtime/moonbase/src/weights/cumulus_pallet_xcmp_queue.rs # runtime/moonbeam/src/weights/cumulus_pallet_xcmp_queue.rs # runtime/moonriver/src/weights/cumulus_pallet_xcmp_queue.rs # test/suites/dev/moonbase/test-randomness/test-randomness-babe-lottery2.ts
Contributor
Coverage Report@@ Coverage Diff @@
## master artur/moonbeam-polkadot-stable2512 +/- ##
======================================================================
- Coverage 77.15% 77.13% -0.02%
Files 389 389
+ Lines 77169 77195 +26
======================================================================
+ Hits 59535 59543 +8
+ Misses 17634 17652 +18
|
# Conflicts: # Cargo.lock # Cargo.toml # test/suites/dev/moonbase/test-author/test-author-failed-association.ts # test/suites/dev/moonbase/test-author/test-author-non-author-clearing.ts # test/suites/dev/moonbase/test-author/test-author-registered-clear.ts # test/suites/dev/moonbase/test-author/test-author-simple-association.ts # test/suites/dev/moonbase/test-author/test-author-unregistered-clear.ts # test/suites/dev/moonbase/test-randomness/test-randomness-babe-lottery3.ts # test/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.ts # test/suites/dev/moonbase/test-xcm-v4/test-xcm-payment-api.ts # test/suites/dev/moonbase/test-xcm-v5/test-xcm-payment-api.ts
… file Convert Notion export (CSV + markdown) into a single .md file with inline markdown tables, shortened link labels, and deduplicated URLs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
test/suites/dev/common/test-proxy/test-proxy-balance.ts (1)
27-27: Event index adjustments align with SDK upgrade expectations.The incremented indices (+1 for each
ExtrinsicSuccessassertion) are consistent with the PR objective stating that the Polkadot SDK stable2512 upgrade introduces additional events in block results.As an optional improvement, consider finding events by method name rather than hardcoded indices to make tests more resilient to future runtime changes:
const extrinsicSuccessEvent = result!.events.find( (e) => e.event.method === "ExtrinsicSuccess" ); expect(extrinsicSuccessEvent).to.exist;This would prevent test breakage when event ordering changes in future upgrades.
,Also applies to: 42-42, 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/common/test-proxy/test-proxy-balance.ts` at line 27, Replace brittle index-based assertions that check result!.events[8].event.method (and similar at the other two locations) with searches for the event by method name; specifically, locate the "ExtrinsicSuccess" event via result!.events.find(e => e.event.method === "ExtrinsicSuccess") (or equivalent) and assert it exists, updating the three occurrences that reference hardcoded indices so tests no longer depend on event ordering.patches/@acala-network__chopsticks-core.patch (1)
1-142: Consider tracking upstream for patch removal.This patch modifies compiled distribution files, which is fragile and will likely break on any
chopsticks-coreversion bump.Consider:
- Opening an upstream issue/PR at
@acala-network/chopsticksfor a proper fix- Adding a comment in
package.jsonor creating a tracking issue noting when this patch can be removed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/`@acala-network__chopsticks-core.patch around lines 1 - 142, This change patches compiled artifacts to add Block.extrinsicMeta and update getValidationData to use parent.extrinsicMeta; instead of committing distro edits, open an upstream issue/PR against `@acala-network/chopsticks` to land the fix in source (mention Block.extrinsicMeta and the change in inherent/parachain/validation-data.js/getValidationData), and add a short tracking note in package.json (or a repo-level TRACKING.md) that references the upstream issue/PR number and the chopsticks-core version when this patch can be removed; also add a one-line TODO comment next to the current patch entry so reviewers know this is temporary and should be reverted once upstream fixes it.test/suites/dev/moonbase/test-sudo/test-sudo.ts (1)
82-87: Consider using consistent assertion style for better error messages.Line 82 uses
expect(result!.events.length === 7).to.be.truewhile T01 (line 32) usesexpect(result!.events.length).to.eq(7). The latter provides more informative failure messages by showing actual vs expected values.♻️ Suggested consistency fix
- expect(result!.events.length === 7).to.be.true; + expect(result!.events.length).to.eq(7);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-sudo/test-sudo.ts` around lines 82 - 87, Replace the boolean-style length assertion with a value equality assertion for consistency and better failure messages: change the check that uses result!.events.length === 7 to use expect(result!.events.length).to.eq(7); ensure similar event assertions remain as-is (they use context.polkadotJs().events.* helpers like system.NewAccount, balances.Endowed, system.ExtrinsicFailed) so only update the length assertion around the result variable to the .to.eq form to match the other tests.node/service/src/rpc.rs (1)
286-307: Remove commented-outgraph.clone()argument from theEth::newcall.The commented-out
// graph.clone()at line 289 serves no purpose. TheEth::newconstructor in the current Frontier version (moonbeam-polkadot-stable2512) no longer requires thegraphparameter, unlike earlier versions. The comment should be removed entirely rather than left as dead code. Thegraphparameter remains necessary forEthFilter::newandTxPool::new, but it has been removed from theEth::newAPI.Suggested cleanup
io.merge( Eth::<_, _, _, _, _, _, MoonbeamEthConfig<_, _>>::new( Arc::clone(&client.clone()), Arc::clone(&pool), - // graph.clone(), convert_transaction, Arc::clone(&sync), signers,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/service/src/rpc.rs` around lines 286 - 307, Remove the dead commented-out argument by deleting the leftover "// graph.clone()" within the Eth::new(...) invocation; ensure the call to Eth::<_, _, _, _, _, _, MoonbeamEthConfig<_, _>>::new(...) only contains the actual live arguments (convert_transaction, Arc::clone(&sync), signers, etc.). Do not change usages of graph elsewhere—keep graph.clone for EthFilter::new and TxPool::new where required—but remove this commented fragment to clean up the Eth::new call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 267-273: The workspace disables SQL for fc-db but the code still
references the SQL backend; update the fc-db dependency declaration that
currently reads like `fc-db = { workspace = true }` to include `features =
["sql"]` so the `FrontierBackendConfig::Sql` match arm can call
`fc_db::sql::Backend::new()` and construct `fc_db::sql::BackendConfig::Sqlite()`
successfully; alternatively, remove the unused SQL branch by deleting the
`FrontierBackendConfig::Sql` handling in the service lib (and the related CLI
usage) if you intend to keep SQL support disabled.
In `@test/suites/dev/moonbase/test-author/test-author-failed-association.ts`:
- Line 46: The test contains a no-op assertion: the line that passes a boolean
expression into expect (the expect around result?.events.length === 7) never
calls an assertion method and will always pass; fix it by replacing that usage
with a real assertion that calls an assertion method on the length value (i.e.,
invoke expect on result?.events.length and call the framework's equality matcher
such as .to.equal(7) for Chai or .toBe(7) for Jest) in the
test-author-failed-association.ts test so the event count is actually asserted.
In `@test/suites/dev/moonbase/test-author/test-author-non-author-clearing.ts`:
- Line 31: The assertion currently uses a boolean expression
(`expect(result?.events.length === 5)`) which is a no-op; change it to assert
the length properly by calling an assertion matcher on the events length (e.g.,
use `expect(result?.events.length).toBe(5)` or
`expect(result?.events).toHaveLength(5)`), referencing the `result` variable and
its `events` property in the test `test-author-non-author-clearing.ts`.
In `@test/suites/dev/moonbase/test-author/test-author-registered-clear.ts`:
- Around line 26-29: The test currently has a no-op assertion using
`expect(result?.events.length === 6)` and then reads `result.events[6]` which is
out of bounds for length 6; change the length assertion to a real Chai assertion
on `result?.events.length` (not a boolean expression) and update the expected
length to 7 (or adjust the accessed index) so `result.events[6]` is valid,
keeping references to `result`, `result.events`, `expect`, and the
`api.events.*` checks (`api.events.balances.Unreserved`,
`api.events.authorMapping.KeysRemoved`, `api.events.system.ExtrinsicSuccess`)
intact.
In `@test/suites/dev/moonbase/test-author/test-author-simple-association.ts`:
- Line 50: The current assertion expect(result?.events.length === 9) is a no-op
because it asserts a boolean expression rather than using the assertion matcher;
change the call to use the testing library's matcher on the value itself, e.g.,
replace the expect invocation around the boolean with
expect(result?.events.length).to.equal(9) (or .toBe(9) if using Jest) so the
test actually asserts the length; update the expect call referencing
result?.events.length in test-author-simple-association.ts accordingly.
In `@test/suites/dev/moonbase/test-author/test-author-unregistered-clear.ts`:
- Line 20: The assertion expect(result?.events.length === 7) is a no-op; change
the expect call to test the value with a matcher instead of comparing inside
expect — replace it with a proper assertion on result?.events.length (for
example, expect(result?.events.length).to.equal(7) if using Chai or
expect(result?.events.length).toBe(7) if using Jest) so the test actually fails
when the length is not 7; update the expect invocation that references
result?.events.length accordingly.
In `@test/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.ts`:
- Around line 48-49: The log message incorrectly says "Estimated Gas for
startLottery" while the value in estimatedGas corresponds to fulfillRequest;
update the log call that prints estimatedGas to reference fulfillRequest instead
of startLottery (locate the log(...) call near the
expect(estimatedGas).to.equal(165855n) assertion), so the message accurately
reflects the function/operation being measured (fulfillRequest).
In `@test/suites/dev/moonbase/test-xcm-v4/test-xcm-payment-api.ts`:
- Around line 136-146: The test currently only checks deliveryFees.isOk is false
which allows any failure; update the assertion to verify the specific error for
"delivery fees not configured" by inspecting deliveryFees.asErr and asserting
the expected variant/message from the xcmPaymentApi response (e.g. check a
specific enum variant or error string) instead of just isOk; locate the call to
polkadotJs.call.xcmPaymentApi.queryDeliveryFees and replace the broad
expect(deliveryFees.isOk).to.be.false with an assertion that deliveryFees.asErr
matches the concrete "delivery fees not configured" variant/message.
In `@test/suites/dev/moonbase/test-xcm-v5/test-xcm-payment-api.ts`:
- Around line 150-160: The test currently only checks deliveryFees.isOk ===
false which accepts any error; change the assertion to require
deliveryFees.isErr and assert the specific error variant/message that represents
"no delivery fees configured" from
polkadotJs.call.xcmPaymentApi.queryDeliveryFees (look up the exact enum/variant
returned) and assert against that expected value (use the
deliveryFees.unwrapErr() or pattern-match the error to confirm the expected
variant/message) so the test fails for malformed asset_id or unrelated runtime
errors; update references to deliveryFees and queryDeliveryFees in the test
accordingly.
---
Nitpick comments:
In `@node/service/src/rpc.rs`:
- Around line 286-307: Remove the dead commented-out argument by deleting the
leftover "// graph.clone()" within the Eth::new(...) invocation; ensure the call
to Eth::<_, _, _, _, _, _, MoonbeamEthConfig<_, _>>::new(...) only contains the
actual live arguments (convert_transaction, Arc::clone(&sync), signers, etc.).
Do not change usages of graph elsewhere—keep graph.clone for EthFilter::new and
TxPool::new where required—but remove this commented fragment to clean up the
Eth::new call site.
In `@patches/`@acala-network__chopsticks-core.patch:
- Around line 1-142: This change patches compiled artifacts to add
Block.extrinsicMeta and update getValidationData to use parent.extrinsicMeta;
instead of committing distro edits, open an upstream issue/PR against
`@acala-network/chopsticks` to land the fix in source (mention Block.extrinsicMeta
and the change in inherent/parachain/validation-data.js/getValidationData), and
add a short tracking note in package.json (or a repo-level TRACKING.md) that
references the upstream issue/PR number and the chopsticks-core version when
this patch can be removed; also add a one-line TODO comment next to the current
patch entry so reviewers know this is temporary and should be reverted once
upstream fixes it.
In `@test/suites/dev/common/test-proxy/test-proxy-balance.ts`:
- Line 27: Replace brittle index-based assertions that check
result!.events[8].event.method (and similar at the other two locations) with
searches for the event by method name; specifically, locate the
"ExtrinsicSuccess" event via result!.events.find(e => e.event.method ===
"ExtrinsicSuccess") (or equivalent) and assert it exists, updating the three
occurrences that reference hardcoded indices so tests no longer depend on event
ordering.
In `@test/suites/dev/moonbase/test-sudo/test-sudo.ts`:
- Around line 82-87: Replace the boolean-style length assertion with a value
equality assertion for consistency and better failure messages: change the check
that uses result!.events.length === 7 to use
expect(result!.events.length).to.eq(7); ensure similar event assertions remain
as-is (they use context.polkadotJs().events.* helpers like system.NewAccount,
balances.Endowed, system.ExtrinsicFailed) so only update the length assertion
around the result variable to the .to.eq form to match the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b890952-b82b-4358-b8bc-0a23dd5127bb
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.github/workflows/build.yml.github/workflows/weight-diff-report.yml.gitignoreCargo.tomlMakefilenode/cli/src/command.rsnode/service/src/lazy_loading/mod.rsnode/service/src/lib.rsnode/service/src/rpc.rspackage.jsonpatches/@acala-network__chopsticks-core.patchprecompiles/crowdloan-rewards/src/mock.rsprecompiles/proxy/src/tests.rsprecompiles/relay-encoder/src/mock.rsprimitives/account/src/lib.rsruntime/common/Cargo.tomlruntime/common/src/apis.rsruntime/moonbase/src/lib.rsruntime/moonbase/src/weights/cumulus_pallet_xcmp_queue.rsruntime/moonbase/src/weights/pallet_xcm.rsruntime/moonbase/tests/common/mod.rsruntime/moonbase/tests/xcm_mock/statemint_like.rsruntime/moonbeam/src/lib.rsruntime/moonbeam/src/weights/cumulus_pallet_xcmp_queue.rsruntime/moonbeam/src/weights/pallet_xcm.rsruntime/moonbeam/tests/common/mod.rsruntime/moonbeam/tests/xcm_mock/statemint_like.rsruntime/moonriver/src/lib.rsruntime/moonriver/src/weights/cumulus_pallet_xcmp_queue.rsruntime/moonriver/src/weights/pallet_xcm.rsruntime/moonriver/tests/common/mod.rsruntime/moonriver/tests/xcm_mock/statemine_like.rsscripts/verify-licenses.shtest/package.jsontest/suites/dev/common/test-proxy/test-proxy-author-mapping.tstest/suites/dev/common/test-proxy/test-proxy-balance.tstest/suites/dev/moonbase/test-author/test-author-failed-association.tstest/suites/dev/moonbase/test-author/test-author-non-author-clearing.tstest/suites/dev/moonbase/test-author/test-author-registered-clear.tstest/suites/dev/moonbase/test-author/test-author-simple-association.tstest/suites/dev/moonbase/test-author/test-author-unregistered-clear.tstest/suites/dev/moonbase/test-balance/test-balance-extrinsics.tstest/suites/dev/moonbase/test-polkadot-js/test-polkadot-api.tstest/suites/dev/moonbase/test-randomness/test-randomness-babe-lottery2.tstest/suites/dev/moonbase/test-randomness/test-randomness-babe-lottery3.tstest/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.tstest/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery5.tstest/suites/dev/moonbase/test-storage-growth/test-precompile-storage-growth.tstest/suites/dev/moonbase/test-sudo/test-sudo.tstest/suites/dev/moonbase/test-xcm-v4/test-xcm-payment-api.tstest/suites/dev/moonbase/test-xcm-v5/test-xcm-payment-api.tszombienet/configs/moonbeam-polkadot.tomlzombienet/configs/moonriver-kusama.toml
💤 Files with no reviewable changes (6)
- precompiles/crowdloan-rewards/src/mock.rs
- runtime/moonriver/src/lib.rs
- runtime/moonbase/src/lib.rs
- precompiles/relay-encoder/src/mock.rs
- precompiles/proxy/src/tests.rs
- runtime/moonbeam/src/lib.rs
test/suites/dev/moonbase/test-author/test-author-failed-association.ts
Outdated
Show resolved
Hide resolved
test/suites/dev/moonbase/test-author/test-author-non-author-clearing.ts
Outdated
Show resolved
Hide resolved
test/suites/dev/moonbase/test-author/test-author-registered-clear.ts
Outdated
Show resolved
Hide resolved
test/suites/dev/moonbase/test-author/test-author-simple-association.ts
Outdated
Show resolved
Hide resolved
test/suites/dev/moonbase/test-author/test-author-unregistered-clear.ts
Outdated
Show resolved
Hide resolved
test/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.ts
Outdated
Show resolved
Hide resolved
- Add Title column to all tables (commit column was misused for this) - Update Commit column to actual hashes on moonbeam-polkadot-stable2512 - Verify all 'Included' items exist on the branch, all 'Dropped' do not - Fix evm EIP-7939: change from Dropped to Included (still on 0.43.x fork) - Flag frontier PR#1855 and PR#1832 as not yet cherry-picked - Clear commit column for Dropped items (no longer on current branch)
Change status from 'PR Upstream Merged' to 'Permanent' — the WILD_MULTI_ASSET_MAX_LIMIT constant does not exist in upstream stable2512 and there is no upstream PR.
… file Convert Notion export (CSV + markdown) into a single .md file with inline markdown tables, shortened link labels, and deduplicated URLs. Verify all commits against moonbeam-polkadot-stable2506 branches. Add Title column and update Commit column to actual hashes on branch. Fix WildMultiAsset status to Permanent (no upstream PR exists). Flag ParachainTracingExecuteBlock as not found on the branch.
'Forbid transactions without chain ID by default' (cdd4ecd9a) was present on the moonbeam-polkadot-stable2512 frontier branch and in the stable2506 doc, but missing from the stable2512 doc.
Documents the cherry-pick tracking process, table schema, verification method (merge-base diffing), upstream remote mapping, and common pitfalls learned from the stable2506→stable2512 upgrade.
This reverts commit a8d14e4.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cherry-picks/polkadot-sdk-stable2506.md`:
- Line 44: Edit the table entry that currently reads "Do client side check of
withdraw-ability" and hyphenate "client-side" so the title becomes "Do
client-side check of withdraw-ability"; locate the row containing that string in
the table (the cell alongside the moonbeam-foundation/frontier commit and
polkadot-evm/frontier PR links) and update only the title text to the hyphenated
form.
In `@docs/cherry-picks/polkadot-sdk-stable2512.md`:
- Around line 75-76: Update the table rows referencing PRs
[polkadot-evm/frontier#1855] and [polkadot-evm/frontier#1832] so the "Cherry
pick" column matches the note; either replace the "Included" cell value with
"Not yet cherry-picked onto the branch" for those two rows, or change the
trailing note to state they are included—ensure consistency between the
"Included" cell and the final note for the entries mentioning
createAndFinalizeBlock() and eth_blockNumber mapping-sync.
- Around line 83-84: Update the status notes for the two table rows "Update to
polkadot-sdk stable2512" and "Add slot based collator" so they no longer say the
PR "can be merged"—replace those pending/mergeable notes with a merged-complete
note (e.g., "Merged" or "Completed") to match the "PR Merged" status in both
rows and ensure the table accurately reflects the final state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4199b633-35bc-45ea-88ec-e12382cd6023
📒 Files selected for processing (2)
docs/cherry-picks/polkadot-sdk-stable2506.mddocs/cherry-picks/polkadot-sdk-stable2512.md
…adot-stable2512 # Conflicts: # .github/workflows/build.yml
Six test files used `expect(value === n)` which passes a boolean to expect() without calling an assertion method, making the check a no-op that never fails. Changed all occurrences to `expect(value).to.equal(n)` so the event length is actually asserted.
The log incorrectly referenced startLottery while the estimate was for the fulfillRequest operation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/suites/dev/moonbase/test-author/test-author-unregistered-clear.ts (1)
20-23: Prefer event-type membership checks over fixed offsets.These assertions are tightly coupled to the exact event ordering/count, which already shifted in this SDK bump. Unless the order itself is part of the contract, asserting that
NewAccount,Endowed, andExtrinsicFailedare present will make this test much less upgrade-sensitive.♻️ Possible rewrite
- expect(result?.events.length).to.equal(7); - expect(api.events.system.NewAccount.is(result!.events[2].event)).to.be.true; - expect(api.events.balances.Endowed.is(result!.events[3].event)).to.be.true; - expect(api.events.system.ExtrinsicFailed.is(result!.events[6].event)).to.be.true; + const events = result!.events.map(({ event }) => event); + expect(events.some((event) => api.events.system.NewAccount.is(event))).to.be.true; + expect(events.some((event) => api.events.balances.Endowed.is(event))).to.be.true; + expect(events.some((event) => api.events.system.ExtrinsicFailed.is(event))).to.be.true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-author/test-author-unregistered-clear.ts` around lines 20 - 23, The test currently asserts specific events by fixed indices (result?.events[2], [3], [6]) which is brittle; update the assertions in the test to check membership instead of offsets by searching result?.events for any event where api.events.system.NewAccount.is(evt.event), api.events.balances.Endowed.is(evt.event), and api.events.system.ExtrinsicFailed.is(evt.event) return true (e.g., use Array.prototype.some or find on result.events) and assert those are present—replace the index-based expects with presence checks that reference result?.events and the api.events.*.is helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/dev/moonbase/test-author/test-author-unregistered-clear.ts`:
- Around line 20-23: The test currently asserts specific events by fixed indices
(result?.events[2], [3], [6]) which is brittle; update the assertions in the
test to check membership instead of offsets by searching result?.events for any
event where api.events.system.NewAccount.is(evt.event),
api.events.balances.Endowed.is(evt.event), and
api.events.system.ExtrinsicFailed.is(evt.event) return true (e.g., use
Array.prototype.some or find on result.events) and assert those are
present—replace the index-based expects with presence checks that reference
result?.events and the api.events.*.is helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 007a1256-6007-4523-991d-771b6725a522
📒 Files selected for processing (7)
test/suites/dev/moonbase/test-author/test-author-failed-association.tstest/suites/dev/moonbase/test-author/test-author-missing-deposit-fail.tstest/suites/dev/moonbase/test-author/test-author-non-author-clearing.tstest/suites/dev/moonbase/test-author/test-author-registered-clear.tstest/suites/dev/moonbase/test-author/test-author-simple-association.tstest/suites/dev/moonbase/test-author/test-author-unregistered-clear.tstest/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/suites/dev/moonbase/test-author/test-author-simple-association.ts
- test/suites/dev/moonbase/test-author/test-author-registered-clear.ts
- test/suites/dev/moonbase/test-author/test-author-failed-association.ts
- test/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.ts
- test/suites/dev/moonbase/test-author/test-author-non-author-clearing.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/cherry-picks/polkadot-sdk-stable2512.md (1)
75-76:⚠️ Potential issue | 🟡 MinorResolve the
Includedvsnot yet cherry-pickedcontradiction.Line 75 and Line 76 still conflict internally:
Cherry picksaysIncluded, but the note says these areNot yet cherry-picked onto the branch. Please align those fields so the ledger has a single source of truth for whether these fixes are actually present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cherry-picks/polkadot-sdk-stable2512.md` around lines 75 - 76, The table rows for "Wait for chain head and eth \"latest\" in createAndFinalizeBlock()" and "Avoid eth_blockNumber returning 0x00 when mapping-sync lags" have conflicting status fields: the Cherry-pick column shows "Included" while the note column says "Not yet cherry-picked onto the branch"; update those entries so both columns agree (either mark both as "Included" and remove the "Not yet cherry-picked onto the branch" note, or change the Cherry-pick column to "Not yet cherry-picked" to match the note) and ensure the PR links ([polkadot-evm/frontier#1855] and [polkadot-evm/frontier#1832]) remain intact for traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cherry-picks/polkadot-sdk-stable2512.md`:
- Around line 11-12: The table has commit links placed under the "Upstream PR"
column for entries like "Add storage benchmark --keys-limit option" and
"ParachainTracingExecuteBlock"; move any non-PR references (raw commit links
such as moonbeam-foundation/polkadot-sdk@ef91563 and
moonbeam-foundation/polkadot-sdk@3e68f82) into the "Commit" or "Note" column and
clear the "Upstream PR" cell unless there is an actual upstream PR URL (ensure
the "ParachainTracingExecuteBlock" row reflects the PRs only in the Upstream PR
column and the commit hashes in Commit/Note), and apply the same change for the
duplicate occurrence mentioned in the comment.
- Line 50: The sentence "Could be dropped since it related to was fixed in
[paritytech/polkadot-sdk#2292]..." is ambiguous—replace it with an explicit
decision sentence: state whether this cherry-pick should be kept or dropped
(e.g., "Keep: investigations show the SDK fix in paritytech/polkadot-sdk#2292
did not fully resolve the issue, so this cherry-pick is required"), and preserve
the references to paritytech/polkadot-sdk#2292, paritytech/polkadot-sdk#1833,
and the investigation note by Eloïs so readers know testing is still needed to
confirm. Ensure the new text unambiguously communicates the current decision
(Keep or Drop) and any next steps (test/reproduce) in one clear sentence.
---
Duplicate comments:
In `@docs/cherry-picks/polkadot-sdk-stable2512.md`:
- Around line 75-76: The table rows for "Wait for chain head and eth \"latest\"
in createAndFinalizeBlock()" and "Avoid eth_blockNumber returning 0x00 when
mapping-sync lags" have conflicting status fields: the Cherry-pick column shows
"Included" while the note column says "Not yet cherry-picked onto the branch";
update those entries so both columns agree (either mark both as "Included" and
remove the "Not yet cherry-picked onto the branch" note, or change the
Cherry-pick column to "Not yet cherry-picked" to match the note) and ensure the
PR links ([polkadot-evm/frontier#1855] and [polkadot-evm/frontier#1832]) remain
intact for traceability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 702c97f1-1910-4873-a47a-6fc966a3a16e
📒 Files selected for processing (1)
docs/cherry-picks/polkadot-sdk-stable2512.md
Both PRs are not yet present on the moonbeam-polkadot-stable2512 frontier branch. Changed Cherry pick column from Included to Pending to match the existing note and Applied: No status.
The test accesses events[6] (7th event) but asserted length === 6. The old no-op expect() never caught this; our fix exposed the mismatch.
librelois
requested changes
Mar 12, 2026
Collaborator
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
Findings
-
Build-breaking RPC constructor mismatch
- File:
node/service/src/rpc.rs - The
Eth::new(...)call no longer matches the Frontier stable2512 signature. graph.clone()was commented out, but the remaining arguments were not realigned, so later parameters are now passed in the wrong positions.- Risk: this should fail to compile.
- File:
-
Build-breaking lookahead consensus param mismatch
- File:
node/service/src/lib.rs - The lookahead collator params were updated as if they used the slot-based API.
- In the checked stable2512 moonkit sources,
lookahead::Paramsstill expectsadditional_relay_keysand does not exposecollator_peer_id. - Risk: this should fail to compile.
- File:
-
Build-breaking trait bound removal
- File:
node/service/src/lib.rs GetCoreSelectorApiwas removed from the service bounds, but the slot-based collator path still requires it in stable2512 moonkit.- Risk: the service should fail to type-check when the slot-based path is compiled.
- File:
-
Likely bridge test regression
- Files:
zombienet/configs/moonbeam-polkadot.toml,zombienet/configs/moonriver-kusama.toml - These configs now launch
rococo-local, but the bridge scripts and test flows still treat the networks as Polkadot/Kusama. - Risk: Moonbeam/Moonriver bridge zombienet scenarios may boot with relay-chain assumptions that no longer match the transfer destinations and bridge setup.
- Files:
Suggestions
-
Fix the stable2512 API migrations in:
node/service/src/rpc.rsnode/service/src/lib.rs
-
Reconcile the relay-chain change:
- Either keep these zombienet configs on
polkadot-local/kusama-local - Or update the bridge scripts/tests so they consistently target
rococo-local
- Either keep these zombienet configs on
-
After fixing the above, run:
- a targeted node build
- the affected bridge zombienet scenario
The extrinsic metadata decode fix is now upstream via AcalaNetwork/chopsticks#1005, so the local patch on @acala-network/chopsticks-core is no longer needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Upgrades all Polkadot SDK dependencies from
moonbeam-polkadot-stable2506tomoonbeam-polkadot-stable2512, along with the necessary adaptations across the node, runtime, precompiles, and tests.Related PRs
Changes
Dependency Upgrades
polkadot-sdkworkspace dependencies updated from branchmoonbeam-polkadot-stable2506→moonbeam-polkadot-stable2512moonbeam-polkadot-stable2506→moonbeam-polkadot-stable2512moonbeam-polkadot-stable2506→moonbeam-polkadot-stable2512pallet-author-slot-filterdependency toruntime/commonrocksdbfeature to Frontier dependenciesRuntime API Changes
execute_blockandcheck_inherentsnow use<Block as BlockT>::LazyBlockinstead ofBlockGetCoreSelectorApiimplementation (no longer required by the SDK)XcmPaymentApi::query_delivery_feesnow accepts an additionalasset_id: VersionedAssetIdparameter and usesAssetExchangerweigh_message()weight function topallet_xcmweights (moonbase, moonbeam, moonriver)Node Service Changes
cumulus_primitives_core::GetCoreSelectorApitrait bound from runtime API requirementsProposerwrapper with direct use ofProposerFactorycollator_peer_id: PeerIdparameter to collator/consensus startupParachainTracingExecuteBlocksupport via newtracing_execute_blockfield inspawn_tasksadditional_relay_keys→additional_relay_state_keysin lookahead collator paramsParachain Inherent Data Refactor
ParachainInherentDatasplit intoBasicParachainInherentData+InboundMessagesDataset_validation_datacall now takes separatedataandinbound_messages_dataparametersPrecompile Updates
SelectCoreconfig fromcumulus_pallet_parachain_system::Configin mock runtimes (crowdloan-rewards, relay-encoder)Weight Updates
cumulus_pallet_xcmp_queue::take_first_concatenated_xcmnow takes an: u32component parameter (all runtimes)Test Fixes
using_fake_author()toinitialize_pending_block()Relevant Upstream PRs (polkadot-sdk)
LazyBlocktype used inexecute_blockandcheck_inherentsParachainInherentDataintoBasicParachainInherentData+InboundMessagesData, adds offchain processing of inbound messagesCoreInfovia a digest to the runtime — moves core selection to collator side, removesGetCoreSelectorApi/SelectCoreSelectCoredigest — preparation for core selection reworkon_initializeafter each pallet — changes event ordering (explains shifted event indices in tests)collator_peer_idparameter to collator consensustrace_block: Support overwritingexecute_block— introducesParachainTracingExecuteBlockVersionedAssetIdinstead ofu32to specify asset for feestake_first_concatenated_xcm()improvements — addsnparameter to weight functionProposerwrapperCheckInherentslogicadditional_relay_keys→additional_relay_state_keysRelevant Moonkit PRs
[author-slot-filter]Addusing_fake_authorlogicRuntime API
Core::execute_blocksignature changed (#9480): Theexecute_blockruntime API now accepts<Block as BlockT>::LazyBlockinstead ofBlock. Extrinsics are lazily decoded, deferring deserialization until needed. The same applies tocheck_inherents. Downstream tools that call these APIs directly must be updated.GetCoreSelectorApiremoved (#9002, #8903): TheGetCoreSelectorApiruntime API and theSelectCoreconfig type oncumulus_pallet_parachain_system::Confighave been fully removed. Core selection is now handled on the collator side and forwarded to the runtime via a digest. Any runtime implementing this API or configuringSelectCoremust remove it.XcmPaymentApibumped to v2 (#9963, #10243):query_delivery_feesnow requires an additionalasset_id: VersionedAssetIdparameter, allowing fee estimation in non-native assets. The v1 signature is retained as#[changed_in(2)]. Clients querying delivery fees must pass the new parameter.Node / Collator
Proposerwrapper removed (#9869):cumulus_client_consensus_proposer::Proposerhas been removed. Usesc_basic_authorship::ProposerFactorydirectly.collator_peer_idnow required (#10145): Collator startup functions (start_consensus, lookahead params) now require aPeerIdparameter. The peer ID is sent via UMP for protocol-level identification.tracing_execute_blockfield added toSpawnTasksParams(#9871):sc_service::SpawnTasksParamsnow includes atracing_execute_blockfield. Parachains should passSome(Arc::new(ParachainTracingExecuteBlock::new(client)))to supporttrace_blockRPC.additional_relay_keysrenamed (#9262): The lookahead collator parameteradditional_relay_keyshas been renamed toadditional_relay_state_keys.Parachain Inherent Data
ParachainInherentDatasplit (#8860): The monolithicParachainInherentDatastruct has been split intoBasicParachainInherentData(validation data, relay chain state, collator peer ID, relay parent descendants) andInboundMessagesData(downward + horizontal messages). Theset_validation_datainherent call now takes both as separate parameters. This enables offchain pre-processing of inbound messages before passing them to the runtime.CheckInherentslogic removed (#9732): TheCheckInherentsparameter inregister_validate_blockhas been removed. Inherent validation is now handled viaConsensusHookand pallet logic. Runtimes no longer need to implementCheckInherents.Event Ordering
on_initializeweight after each pallet's execution rather than batching them. This emits an additional weight-related event early in the block, shifting all subsequent event indices by one. Any off-chain code or tests that reference events by index must be updated.Weights
take_first_concatenated_xcmsignature changed (#9539): The weight function now takes an: u32component parameter instead of being a zero-argument function. Weight implementations must be updated to match the new trait signature.pallet_xcm::WeightInfo::weigh_messageadded: A new required method on thepallet_xcm::WeightInfotrait. Custom weight implementations must add this function.