refactor(vara): migrate polkadot-sdk to upstream stable2409#5471
refactor(vara): migrate polkadot-sdk to upstream stable2409#5471ukint-vs wants to merge 38 commits into
Conversation
ukint-vs
force-pushed
the
vs/polkadot-upstream-2409-proof
branch
from
8c183d5 to
819d69e
Compare
ukint-vs
force-pushed
the
vs/polkadot-upstream-2409-proof
branch
2 times, most recently
from
43c64a8 to
197ce22
Compare
ukint-vs
force-pushed
the
vs/polkadot-upstream-2409-proof
branch
from
197ce22 to
6c85944
Compare
breathx
changed the title
…m-2409-proof # Conflicts: # vara/pallets/gear/src/migrations/mod.rs
ukint-vs
added
type: feat
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request migrates the Gear project's dependency on the custom Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates Substrate-based dependencies from the Gear fork to the upstream paritytech/polkadot-sdk repository. It introduces several local crates in the substrate/ directory to house modified or copied logic from the SDK, including the runtime executor and WASM builder. The changes also involve extensive updates to Cargo.toml and Cargo.lock, refinement of build scripts, and adjustments to example projects. Review feedback points out an unnecessary deprecation attribute in the new executor code and a redundant manifest patch for the now-removed wasmi 0.13.2 dependency.
…m-2409-proof # Conflicts: # vara/sdk/gsdk/vara_runtime.scale
|
@claude review |
…m-2409-proof # Conflicts: # .config/hakari.toml # Cargo.lock # Cargo.toml # utils/gear-workspace-hack/Cargo.toml # vara/runtime/interface/sandbox/src/detail.rs # vara/sdk/gsdk/vara_runtime.scale
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the workspace's Polkadot SDK dependencies from the custom gear-tech fork to the upstream paritytech repository (revision 298f676c91d64f15f38ea7fd78f125c5889ab09c). To isolate the remaining fork delta, several Polkadot SDK crates (such as sc-executor, sp-allocator, and substrate-wasm-builder) have been copied locally into the substrate/ directory, and the workspace publishing utilities have been updated to handle these Gear-maintained aliases. Feedback on the changes highlights three correctness issues: a potential integer overflow in the allocator's bump function that could lead to memory corruption, a path-escaping issue on Windows when writing Wasmtime cache configurations to TOML, and an incorrect directory-removal call on files during local registry cleanup.
|
@claude review |
|
Claude finished @ukint-vs's task in 5m 11s —— View job Code Review
Gemini May-29 findings — all three are already resolved in the current code:
No action needed on any of these. medium:
Any published Gear crate that transitively depends on This is fine now, but the invariant — that no published Gear crate may transitively reach low:
Confirmed correct — worth calling out:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request brings several Polkadot SDK crates locally into the workspace under the substrate/ directory to isolate fork deltas, updating Cargo.toml, Cargo.lock, and hakari configurations to patch these dependencies. It also adapts the WASM builder, optimizer, and processor to support these changes, alongside updates to helper scripts and third-party notices. The review identified several issues: an unstable let-chains syntax in wasm-proc that will fail compilation; an unused variable allow_undefined in wasm-optimizer that triggers a warning under -D warnings; an incorrect jq object query in clippy.sh; a type improvement for TOML tables in wasm-builder; and an inconsistency in THIRD_PARTY_NOTICES.md regarding the publishing status of sc-executor.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Payload::CustomSection(section) = | ||
| payload.map_err(|e| format!("Deserialization error: {e}"))? | ||
| && section.name() == expected_section | ||
| { |
There was a problem hiding this comment.
The if let ... && ... syntax (let-chains) is an unstable Rust feature (RFC 2497). Unless #![feature(let_chains)] is explicitly enabled at the crate root, this will fail to compile on both stable and nightly compilers. It is safer and more compatible to use a nested if block instead.
if let Payload::CustomSection(section) =
payload.map_err(|e| format!("Deserialization error: {e}"))?
{
if section.name() == expected_section {
return Ok(true);
}
}| let allow_undefined = rustc_version.major == 1 && rustc_version.minor >= 96; | ||
|
|
||
| let mut rustc_flags = vec!["-Clink-arg=--import-memory"]; | ||
| let mut rustc_flags = vec!["-Clink-arg=--import-memory", "-Clink-arg=--allow-undefined"]; |
There was a problem hiding this comment.
The allow_undefined variable is defined here but is never used because "-Clink-arg=--allow-undefined" was unconditionally added to rustc_flags on line 30. This will trigger an unused_variable warning, which will cause compilation to fail under -D warnings settings. Consider either conditionally adding the flag using allow_undefined or removing the unused variable.
let allow_undefined = rustc_version.major == 1 && rustc_version.minor >= 96;
let mut rustc_flags = vec!["-Clink-arg=--import-memory"];
if allow_undefined {
rustc_flags.push("-Clink-arg=--allow-undefined");
}| mapfile -t no_std < <( | ||
| cargo metadata --no-deps --format-version=1 | | ||
| jq -r '.packages.[] | select(.features | index("std")) | "-p=" + .name' | ||
| jq -r '.workspace_members as $members | .packages.[] | . as $pkg | select($members | index($pkg.id)) | select(.features | index("std")) | select(.name != "sc-executor") | "-p=" + .name' |
There was a problem hiding this comment.
In jq, .features is an object (map) representing the package's features. Using index("std") on an object is not standard and can lead to errors or unexpected behavior depending on the jq version. Using has("std") is the idiomatic and robust way to check for the presence of a key in a jq object.
| jq -r '.workspace_members as $members | .packages.[] | . as $pkg | select($members | index($pkg.id)) | select(.features | index("std")) | select(.name != "sc-executor") | "-p=" + .name' | |
| jq -r '.workspace_members as $members | .packages.[] | . as $pkg | select($members | index($pkg.id)) | select(.features | has("std")) | select(.name != "sc-executor") | "-p=" + .name' |
| /// | ||
| /// The sub-project is written outside the workspace root, so workspace-relative | ||
| /// patch paths like local `substrate/` overrides cannot be resolved there. | ||
| fn remove_local_path_patches(patch: &mut BTreeMap<String, toml::Value>) { |
There was a problem hiding this comment.
The remove_local_path_patches function currently takes &mut BTreeMap<String, toml::Value>. However, toml::value::Table (which is already imported as Table on line 16) is the canonical type for TOML tables and is defined as toml::map::Map in newer versions of the toml crate. Using &mut Table is more robust and avoids potential type mismatches if the underlying representation of Table changes.
| fn remove_local_path_patches(patch: &mut BTreeMap<String, toml::Value>) { | |
| fn remove_local_path_patches(patch: &mut Table) { |
| - `substrate/runtime-executor/common` (`sc-executor-common`, published by Gear as `gsc-executor-common`) | ||
| - `substrate/runtime-executor/polkavm` (`sc-executor-polkavm`, published by Gear as `gsc-executor-polkavm`) | ||
| - `substrate/runtime-executor/wasmtime` (`sc-executor-wasmtime`, published by Gear as `gsc-executor-wasmtime`) | ||
| - `substrate/runtime-executor` (`sc-executor`, published by Gear as `gsc-executor`) |
There was a problem hiding this comment.
There is an inconsistency regarding the sc-executor crate. This file states that sc-executor is published by Gear as gsc-executor. However, substrate/README.md states that sc-executor is not published by Gear, and it is indeed excluded from GEAR_SUBSTRATE_DEPENDENCIES in utils/crates-io/src/lib.rs. Please update this notice to correctly reflect that sc-executor is not published.
| - `substrate/runtime-executor` (`sc-executor`, published by Gear as `gsc-executor`) | |
| - substrate/runtime-executor (sc-executor, not published by Gear) |
|
@claude opus review |
|
Claude finished @ukint-vs's task in 5m 12s —— View job Code Review (Opus)
Focus is on the latest Gemini batch (June-03) plus anything new since the prior Opus reviews. I verified each Gemini point against the current code rather than taking it at face value — two of the five don't hold up, three are real (one is more serious than Gemini rated it). medium — File: jq -r '... | select(.features | index("std")) | select(.name != "sc-executor") | ...'Gemini flagged So low — duplicate File: The PR made let mut rustc_flags = vec!["-Clink-arg=--import-memory", "-Clink-arg=--allow-undefined"]; // line 30 (added here)
...
if allow_undefined { // lines 45–51 (from master, #5533)
rustc_flags.extend_from_slice(&["-C", "link-arg=--allow-undefined"]);
}On the pinned nightly ( low — File: But Gemini June-03 findings I checked and would not act on
Confirmed-correct items from this batch
Net: three small fixes worth folding in (clippy |
3 participants
Summary
Draft proof branch for removing Gear's dependency on the custom
gear-tech/polkadot-sdkstable2409fork and using upstreamparitytech/polkadot-sdkinstead.paritytech/polkadot-sdkrev298f676c91d64f15f38ea7fd78f125c5889ab09csubstrate/: runtime executor, allocator, wasm interface common, and substrate wasm buildersc-executor,sc-executor-common,sc-executor-polkavm,sc-executor-wasmtime, andsubstrate-wasm-builderover upstream package identitiessc-executorhost-api edgewasmtime8.0.1 (default-features = false), matching canonical SDK expectationsvara/sdk/gsdk/vara_runtime.scale; upstream staking addswithdraw_overstake, soRuntimeCallmetadata changesPOLKADOT_UPSTREAM_2409_PROOF.mdnote out of the PR/main branchMigration / deletion notes
substrate/without package renames or broad extra churn.Validation
rtk cargo fmt --checkrtk git diff --checkrtk cargo metadata --no-deps --format-version 1rtk make workspace-hackrtk make clippyrtk cargo +stable clippy -p crates-io --all-targets --all-features -- -D warningsrtk cargo +stable run --release -p crates-io publish --simulate --registry-path /tmp/cargo-http-registryrtk ./scripts/gear.sh clippy gear --all-targets --all-features --lockedrtk cargo nextest run --profile ci -p sc-executor-wasmtime --all-targets --all-features test_stack_depth_reachingrtk cargo nextest run --profile ci -p gsdk --all-targets --all-features set_code_succeedset_code_succeed,alloc_zero_pages,harmless_upload,set_code_failed,program_migration_fails_if_program_exists,program_with_gas_reservation_migrated_to_another_node,get_mailbox,program_migrated_to_another_node,memory_dumpNotes
gear-service --features vara-nativecheck was intentionally skipped.build / try-runtime (debug)green on the Vara snapshot; no storage migration is indicated unless try-runtime regresses on the updated head.