Update integration test for valid relay chain timestamp in parachain storage #1456
Conversation
WalkthroughAdds a new zombienet battery-station upgrade environment and docs, introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Collator as Collator/Node
participant Parachain as Parachain Chain
participant Sudo as Sudo/Root
Note over Test: prepare wasm bytes & compute wasm_hash
Test->>Collator: submit signed `system.authorizeUpgrade(wasm_hash)`
Collator->>Parachain: include authorizeUpgrade extrinsic
Parachain-->>Test: emit events/logs (authorization recorded)
Note right of Test: wait for validation marker (e.g., storage/event)
Test->>Sudo: submit `sudo(applyAuthorizedUpgrade(wasm_bytes))`
Sudo->>Collator: include sudo extrinsic
Collator->>Parachain: execute applyAuthorizedUpgrade → runtime updated
Parachain-->>Test: emit events/logs (upgrade applied)
Test->>Parachain: query `:code` storage
Parachain-->>Test: return runtime `:code` → Test compares to expected wasm
Note over Test,Parachain: Test also reads `RelayTimestampNow` across blocks and verifies monotonic increase
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
integration-tests/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
integration-tests/README.md(1 hunks)integration-tests/moonwall.config.json(2 hunks)integration-tests/package.json(1 hunks)integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(3 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 (10)
integration-tests/moonwall.config.json (1)
7-23: LGTM! Configuration correctly aligned to battery-station runtime.The environment name, runtime upgrade path, and zombie spec configuration are all consistently updated to reference battery-station runtime.
integration-tests/package.json (1)
15-15: LGTM! Dependency addition is correct and consistent.The @polkadot/util package (version 13.5.2) is correctly added and matches the versions of related @PolkaDot packages, with proper override configuration already in place.
integration-tests/README.md (1)
89-92: LGTM! Documentation correctly updated.The documentation accurately reflects the new battery-station runtime test environment and command, matching the configuration changes.
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (7)
27-28: LGTM! Appropriate utilities imported.The @polkadot/util and @polkadot/util-crypto imports are correctly used for storage key construction and wasm hashing in the authorized upgrade flow and relay timestamp verification.
88-93: LGTM! Enhanced logging for debugging.The specVersion and rtUpgradePath logging provides valuable context for understanding the upgrade process.
148-181: LGTM! Dynamic call discovery provides flexibility.The
findCallhelper and upgrade call discovery logic allows the test to adapt to different runtime configurations where upgrade calls might be in different pallets. The logging provides good visibility into the discovered sections.
220-236: LGTM! Comprehensive upgrade verification.The verification logic properly confirms that:
- Block production continued after upgrade
- Runtime code changed to the expected wasm
- Both conditions are logged for debugging
257-277: Verify the exact timestamp equality expectation.The test assumes exact equality between
tsPara1andtsRelay1(line 277). This is a strict assertion that might fail if there are any rounding differences or if the parachain applies any transformation to the relay timestamp.Please confirm that the parachain timestamp is expected to exactly mirror the relay timestamp with no transformation. If there's any documentation or specification for this feature, it would help verify this assumption.
279-293: LGTM! Monotonicity check is appropriate.The verification that the relay timestamp increases across blocks (line 291) is a good sanity check for the relay proof mechanism. The test properly waits for new blocks and verifies both relay and parachain timestamps advance together.
244-255: Based on my verification of the codebase context, I can now provide the rewritten review comment:
The manual storage key construction using xxhashAsU8a is the correct and necessary approach for accessing relay chain storage.
RelayTimestampNow is relay chain state that must be accessed via low-level storage queries since polkadot.js does not expose query APIs for relay chain pallets to parachains. The pattern of combining xxhashAsU8a("runtime", 128) with xxhashAsU8a("RelayTimestampNow", 128) is standard Substrate storage key derivation and is the only way to access this storage from the parachain. The u64 type assumption aligns with the standard Timestamp pallet type used elsewhere in the test (e.g.,
paraApi.query.timestamp.now()). The code properly validates the storage exists before decoding, so no changes are needed.
| const txStatus = async (tx: any, label: string) => | ||
| new Promise<void>((resolve, reject) => { | ||
| let unsubscribe: (() => void) | undefined; | ||
| tx.signAndSend(alice, (result: any) => { | ||
| if (result.dispatchError) { | ||
| // Dispatch errors won't throw, so surface them explicitly. | ||
| const errText = result.dispatchError.toString(); | ||
| log(`${label} dispatchError=${errText}`); | ||
| reject(new Error(`${label} failed: ${errText}`)); | ||
| unsubscribe?.(); | ||
| return; | ||
| } | ||
| log( | ||
| `${label} status=${result.status?.type ?? "unknown"}, events=${result.events | ||
| ?.map((ev: any) => `${ev.event.section}.${ev.event.method}`) | ||
| .join(",")}` | ||
| ); | ||
| if (result.status?.isInBlock || result.status?.isFinalized) { | ||
| unsubscribe?.(); | ||
| resolve(); | ||
| } | ||
| }) | ||
| .then((unsub: () => void) => { | ||
| unsubscribe = unsub; | ||
| }) | ||
| .catch(reject); | ||
| }); |
There was a problem hiding this comment.
Add timeout protection to prevent indefinite hangs.
The txStatus helper lacks timeout protection. If a transaction never reaches isInBlock or isFinalized status (due to network issues, dropped transactions, etc.), the promise will hang indefinitely, causing the test to timeout at the suite level rather than providing a clear failure reason.
Consider adding a timeout mechanism:
const txStatus = async (tx: any, label: string) =>
new Promise<void>((resolve, reject) => {
let unsubscribe: (() => void) | undefined;
+ const timeout = setTimeout(() => {
+ unsubscribe?.();
+ reject(new Error(`${label} timed out after 60s`));
+ }, 60000);
tx.signAndSend(alice, (result: any) => {
if (result.dispatchError) {
const errText = result.dispatchError.toString();
log(`${label} dispatchError=${errText}`);
+ clearTimeout(timeout);
reject(new Error(`${label} failed: ${errText}`));
unsubscribe?.();
return;
}
log(
`${label} status=${result.status?.type ?? "unknown"}, events=${result.events
?.map((ev: any) => `${ev.event.section}.${ev.event.method}`)
.join(",")}`
);
if (result.status?.isInBlock || result.status?.isFinalized) {
+ clearTimeout(timeout);
unsubscribe?.();
resolve();
}
})
.then((unsub: () => void) => {
unsubscribe = unsub;
})
- .catch(reject);
+ .catch((err) => {
+ clearTimeout(timeout);
+ reject(err);
+ });
});🤖 Prompt for AI Agents
In
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
around lines 120-146, the txStatus promise can hang indefinitely because it
never rejects if a transaction never becomes InBlock/Finalized; add timeout
protection by starting a timer when the promise is created that rejects with a
descriptive timeout error after a configurable duration (e.g., 30s), ensure the
timer is cleared on resolve/reject, and call unsubscribe before rejecting on
timeout so the subscription is cleaned up; also ensure any caught errors clear
the timer and unsubscribe as well.
| if (upgradeAvailable) { | ||
| // Zeitgeist runtime blocks `setCode`, so use the authorized upgrade flow. | ||
| const wasmHash = blake2AsHex(wasm); | ||
| log("Authorizing runtime upgrade via system.authorizeUpgrade"); | ||
| const authorizeTx = | ||
| authorizeUpgrade.meta.args.length === 1 | ||
| ? authorizeUpgrade(wasmHash) | ||
| : authorizeUpgrade(wasmHash, true); | ||
| log( | ||
| `authorizeUpgrade located in section=${upgradeCallLocation.authorize}, args=${authorizeUpgrade.meta.args.length}` | ||
| ); | ||
| await txStatus( | ||
| paraApi.tx.sudo.sudo(authorizeTx), | ||
| "authorizeUpgrade" | ||
| ); | ||
|
|
||
| log("Waiting for validation function approval"); | ||
| await context.waitBlock(2); | ||
|
|
||
| log("Enacting authorized upgrade"); | ||
| log( | ||
| `applyAuthorizedUpgrade located in section=${upgradeCallLocation.enact}, args=${applyAuthorizedUpgrade.meta.args.length}` | ||
| ); | ||
| await txStatus( | ||
| paraApi.tx.sudo.sudo(applyAuthorizedUpgrade(rtHex)), | ||
| "applyAuthorizedUpgrade" | ||
| ); | ||
| } else { | ||
| throw new Error( | ||
| "Runtime upgrade calls missing in metadata; expected system.authorizeUpgrade/applyAuthorizedUpgrade" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for authorizeUpgrade and applyAuthorizedUpgrade definitions in runtime
# First, find runtime-related directories
echo "=== Locating runtime directories ==="
fd -t d "runtime" | head -20
echo -e "\n=== Searching for authorizeUpgrade definitions ==="
rg -n "authorizeUpgrade\|applyAuthorizedUpgrade" \
--type rust \
-B 2 -A 5 \
2>/dev/null | head -100Repository: zeitgeistpm/zeitgeist
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Read the test file to understand the full context
echo "=== Reading test file (lines 150-240 for context) ==="
sed -n '150,240p' integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
echo -e "\n=== Checking Cargo.toml for SDK version ==="
find . -name "Cargo.toml" -path "*/integration-tests/*" | head -1 | xargs cat | grep -A 5 -B 5 "polkadot"Repository: zeitgeistpm/zeitgeist
Length of output: 3696
🏁 Script executed:
#!/bin/bash
# Find integration-tests Cargo.toml and check SDK version
echo "=== Finding integration-tests Cargo.toml ==="
find . -path "*/integration-tests/Cargo.toml" -exec cat {} \;
echo -e "\n=== Searching for other meta.args.length patterns ==="
rg -n "meta\.args\.length" --type typescript --type javascript -B 2 -A 2
echo -e "\n=== Searching for waitBlock patterns to understand timing ==="
rg -n "waitBlock" --type typescript -B 2 -A 2 | head -50Repository: zeitgeistpm/zeitgeist
Length of output: 314
🏁 Script executed:
#!/bin/bash
# Better approach to find integration-tests files
echo "=== Integration-tests directory structure ==="
ls -la integration-tests/ 2>/dev/null | head -20
echo -e "\n=== Finding Cargo.toml files ==="
find integration-tests -name "Cargo.toml" 2>/dev/null
echo -e "\n=== Search for meta.args.length (no type filter) ==="
rg -n "meta\.args\.length" -B 2 -A 2 | head -60
echo -e "\n=== Search for waitBlock (no type filter) ==="
rg -n "waitBlock" -B 2 -A 2 | head -60Repository: zeitgeistpm/zeitgeist
Length of output: 5027
🏁 Script executed:
#!/bin/bash
# Check package.json for SDK versions and find waitBlock implementation
echo "=== Integration-tests package.json ==="
cat integration-tests/package.json
echo -e "\n=== Search for context.waitBlock definition/implementation ==="
rg -n "waitBlock" --type typescript -A 5 -B 2 | grep -A 10 "function\|const.*waitBlock\|=>" | head -40
echo -e "\n=== Search for moonwall context/framework files ==="
rg -n "class.*Context\|function.*context" integration-tests -B 2 -A 5 | head -60Repository: zeitgeistpm/zeitgeist
Length of output: 1621
🏁 Script executed:
#!/bin/bash
# Search for test-zombienet-runtime-upgrade.ts full content to understand context better
echo "=== Full test file content (part 1 - imports and setup) ==="
head -100 integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
echo -e "\n=== Check for comments about timing or block requirements ==="
rg -n "block\|timing\|wait" integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts -i | head -40
echo -e "\n=== Search for authorizeUpgrade in Polkadot docs or comments ==="
rg -n "authorizeUpgrade\|applyAuthorizedUpgrade\|SDK\|version" integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts -i -B 1 -A 1Repository: zeitgeistpm/zeitgeist
Length of output: 3789
🏁 Script executed:
#!/bin/bash
# Search for more context about the meta.args check and what it's handling
echo "=== Full context around meta.args.length check (lines 170-220) ==="
sed -n '170,220p' integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
echo -e "\n=== Search for any SDK or version-related comments in test ==="
rg -n "SDK\|version\|meta\|args" integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts | head -30Repository: zeitgeistpm/zeitgeist
Length of output: 2331
🌐 Web query:
Polkadot SDK 16.5.2 authorizeUpgrade signature args parameters
💡 Result:
Summary (@polkadot/api v16.5.2)
-
Extrinsic: api.tx.system.authorizeUpgrade(codeHash: H256)
- Single argument: codeHash of type H256 (the runtime code hash).
- Origin required: Root. [1][2]
-
Related extrinsics:
- api.tx.system.authorizeUpgradeWithoutChecks(codeHash: H256) — same arg but skips safety checks. [1][2]
- api.tx.system.applyAuthorizedUpgrade(code: Bytes) — provides the runtime WASM preimage (Bytes). [3]
Sources:
- Polkadot JS extrinsics docs (system.authorizeUpgrade / authorizeUpgradeWithoutChecks). [1]
- Substrate / Polkadot extrinsics listing (applyAuthorizedUpgrade / authorizeUpgrade). [3]
- @polkadot/api (v16.5.2) package listing. [4]
🏁 Script executed:
#!/bin/bash
# Search for authorizeUpgrade definition in battery-station runtime
echo "=== Searching for authorizeUpgrade in battery-station runtime ==="
rg -n "authorizeUpgrade" runtime/battery-station -B 3 -A 3 --type rust
echo -e "\n=== Checking runtime/zeitgeist as well ==="
rg -n "authorizeUpgrade" runtime/zeitgeist -B 3 -A 3 --type rust
echo -e "\n=== Search for pallet_system config or extensions ==="
rg -n "pallet_system\|System" runtime/battery-station/src/lib.rs -B 2 -A 10 | head -80Repository: zeitgeistpm/zeitgeist
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Check battery-station runtime configuration
echo "=== Reading battery-station runtime/lib.rs ==="
wc -l runtime/battery-station/src/lib.rs
echo -e "\n=== Grep for System pallet config ==="
rg -n "impl.*frame_system\|frame_system::Config\|System" runtime/battery-station/src/lib.rs -B 2 -A 5 | head -100
echo -e "\n=== Check for any comments about upgrade or args ==="
rg -n "upgrade\|args\|variant" runtime/battery-station/src/lib.rs -i | head -30Repository: zeitgeistpm/zeitgeist
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Read the full battery-station runtime lib.rs
echo "=== Full battery-station/src/lib.rs ==="
cat -n runtime/battery-station/src/lib.rs
echo -e "\n=== Check runtime structure for other files ==="
find runtime/battery-station/src -type f -name "*.rs" | head -20Repository: zeitgeistpm/zeitgeist
Length of output: 7574
🌐 Web query:
Polkadot runtime upgrade authorizeUpgrade applyAuthorizedUpgrade timing blocks requirements
💡 Result:
Short answer — how authorizeUpgrade + applyAuthorizedUpgrade work, timing and requirements:
- authorizeUpgrade(code_hash) must be called with Root origin and simply records an authorized code hash (two‑phase flow useful when the WASM preimage is large or sent later). [1][2]
- applyAuthorizedUpgrade(code) can be called by any origin; it supplies the WASM preimage for a previously authorized hash. If the authorization required version checks, applyAuthorizedUpgrade enforces that the spec_name is unchanged and spec_version increases. [1][2]
- Timing: whether the new runtime is applied immediately in the same block or scheduled for a later block depends on the runtime’s OnSetCode implementation (T::OnSetCode). In many chains (including Polkadot/parachains) the OnSetCode implementation may schedule the actual set_code to a future block (so applyAuthorizedUpgrade may only enqueue the change). The system::set_code path likewise delegates to T::OnSetCode::set_code. [2][3]
- Parachain note: parachains generally must use the authorize/apply_authorized two‑step flow to coordinate upgrades with the relay chain’s expectations (hence the authorize step is required for parachain upgrade workflows). [1][3]
- Practical checks/requirements: authorizeUpgrade requires Root; applyAuthorizedUpgrade validates the preimage matches an authorized hash and (unless using the WithoutChecks variants) enforces spec/version checks; there are also set_code and without_checks variants if you need single‑step or skip safety checks. [1][2]
Sources: polkadot.js extrinsics docs (system.authorizeUpgrade / applyAuthorizedUpgrade). [1] Substrate frame_system implementation and OnSetCode behavior (set_code / set_code_without_checks / OnSetCode trait). [2][3]
The meta.args.length check is dead code; 2-block wait is reasonable but undocumented.
1. Lines 189-190 — Unnecessary conditional: The check for authorizeUpgrade.meta.args.length === 1 will always be true with Polkadot SDK v16.5.2, which defines authorizeUpgrade(codeHash: H256) with only one argument. The else branch (authorizeUpgrade(wasmHash, true)) is dead code. This appears to be legacy compatibility logic for older SDK versions. While not a functional issue, it creates unnecessary complexity and could confuse future maintainers.
2. Line 200 — 2-block wait is reasonable: The waitBlock(2) timing is appropriate for parachain upgrade workflows, as the OnSetCode implementation may schedule the actual code change for a future block. However, there is no explicit documentation in the code explaining why 2 blocks is chosen. The subsequent verification at lines 220–225 properly validates that the upgrade succeeded, so the wait is effective.
Consider:
- Simplifying the conditional to remove the dead else branch (or add a comment explaining the version compatibility intent if intentional).
- Adding a comment on line 199 explaining why 2 blocks is sufficient (e.g., "wait for OnSetCode scheduling and block finalization").
🤖 Prompt for AI Agents
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
lines 183-214: the conditional that checks authorizeUpgrade.meta.args.length and
the else branch with authorizeUpgrade(wasmHash, true) is dead/legacy code for
current Polkadot SDK and should be simplified to always call
authorizeUpgrade(wasmHash); remove the unused else branch; additionally,
annotate the waitBlock(2) call (around line 199) with a brief comment like "wait
for OnSetCode scheduling and block finalization (2 blocks)" so the choice is
documented for future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (2)
120-146: Timeout protection still missing.As previously noted, the
txStatushelper lacks timeout protection and can hang indefinitely if a transaction never reachesisInBlockorisFinalizedstatus.Please see the previous review comment for the suggested timeout implementation.
183-214: Conditional check is still dead code; 2-block wait needs documentation.As previously noted:
- The
authorizeUpgrade.meta.args.length === 1check will always be true with Polkadot SDK v16.5.2, making the else branch dead code.- The
waitBlock(2)call (line 200) lacks documentation explaining why 2 blocks is sufficient.Please refer to the previous review comment for the recommended simplification and documentation.
🧹 Nitpick comments (2)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (2)
27-28: Remove unused imports.The imports
u8aConcatandxxhashAsU8aare not used in this file. Onlyblake2AsHexis used (line 185).Apply this diff to remove the unused imports:
-import { u8aConcat } from "@polkadot/util"; -import { blake2AsHex, xxhashAsU8a } from "@polkadot/util-crypto"; +import { blake2AsHex } from "@polkadot/util-crypto";
247-254: Stricter error handling for storage decoding.Line 253 uses
rawHex ?? 0as a fallback, which could mask decoding failures. IfrawHexis undefined (butrawis not null), the function silently returns 0 instead of surfacing the actual issue.Apply this diff for clearer error handling:
const readRelayTs = async (): Promise<number> => { const raw = await paraApi.rpc.state.getStorage(relayTsStorageKey); expect(raw, "RelayTimestampNow storage should exist").to.not.be.null; const rawHex = raw?.toHex(); - expect(rawHex, "RelayTimestampNow should decode to hex").to.not.be.undefined; + if (!rawHex) { + throw new Error("RelayTimestampNow failed to decode to hex"); + } log(`RelayTimestampNow raw=${rawHex}`); - return paraApi.createType("u64", rawHex ?? 0).toNumber(); + return paraApi.createType("u64", rawHex).toNumber(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(3 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 (6)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (6)
88-93: Good observability addition.The specVersion and upgrade path logging will help with debugging runtime upgrade flows.
148-156: LGTM!The
findCallhelper correctly locates calls across API sections and returns both the call and its section for later use.
158-182: LGTM!The upgrade call discovery logic defensively locates the required calls and provides useful logging for troubleshooting.
220-235: LGTM!The verification logic is thorough, with detailed logging and strong assertions ensuring the runtime upgrade succeeded.
276-280: Exact timestamp equality is appropriate for this test.The test correctly validates that the parachain timestamp exactly mirrors the relay timestamp, which is the expected behavior when the parachain reads timestamps from relay chain proofs.
Also applies to: 290-295
244-246: Add a comment documenting the hardcoded storage key instead of computing it dynamically.The storage key
0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063is correctly hardcoded and appropriate for this integration test. For tests running against a specific runtime version, hardcoding known storage keys is standard practice. Attempting to compute it dynamically adds unnecessary complexity without improving reliability. Simply document the key's derivation:+ // Storage key for RelayTimestampNow: xxhash128("runtime") + xxhash128("RelayTimestampNow") const relayTsStorageKey = "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063";
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (1)
244-245: Hardcoded storage key is fragile and undocumented.The raw storage key
0x54dbd40f...will silently break if the pallet or storage item name changes. Consider computing it dynamically or documenting its derivation.Option 1 - Compute dynamically using the imported
xxhashAsU8a:import { u8aToHex } from "@polkadot/util"; // RelayTimestampNow storage key = xxhash128("RelayTimestampNow") twice (module + item) const moduleHash = xxhashAsU8a("RelayTimestampNow", 128); const itemHash = xxhashAsU8a("RelayTimestampNow", 128); const relayTsStorageKey = u8aToHex(u8aConcat(moduleHash, itemHash));Option 2 - At minimum, add a comment explaining the derivation:
// Storage key for pallet "RelayTimestampNow", item "RelayTimestampNow" // Computed as: xxhash128("RelayTimestampNow") || xxhash128("RelayTimestampNow") const relayTsStorageKey = "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(3 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 (6)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (6)
88-93: LGTM!Good diagnostic logging for debugging test failures. The specVersion and upgrade path information will be helpful when investigating issues.
148-156: LGTM!Clean helper for dynamically locating extrinsics across sections. The undefined return for missing calls is properly handled downstream.
158-176: LGTM!The dynamic discovery of upgrade calls provides good SDK version compatibility. The
upgradeAvailablecheck ensures both required calls exist before proceeding.
178-181: LGTM!Useful diagnostic logging to understand the available upgrade-related transaction sections.
256-269: LGTM!Good retry pattern with bounded attempts and clear logging. The subsequent assertion at line 276 will provide a clear failure message if the timestamp never populates.
277-280: Assertion is correct—parachain timestamp is architecturally designed to derive from relay timestamp.Zeitgeist's runtime explicitly configures the parachain's
pallet_timestampto useRelayTimestampas its time source. TheRelayTimestampNowstorage value is populated from relay chain state proofs viaConsensusHookWrapperForRelayTimestamp, so exact equality is the correct expectation. The test validates this integral design.
| const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString(); | ||
| log( | ||
| `Before: #${blockNumberBefore}, After: #${blockNumberAfter}, code changed=${ | ||
| codeAfter !== codeString | ||
| }` | ||
| ); | ||
| log( | ||
| `Code (before): ${codeString.slice(0, 10)}...${codeString.slice(-10)}, code (after): ${ | ||
| codeAfter ? codeAfter.slice(0, 10) + "..." + codeAfter.slice(-10) : "undefined" | ||
| }` | ||
| ); | ||
| expect( | ||
| blockNumberAfter, | ||
| "Block number did not increase" | ||
| ).to.be.greaterThan(blockNumberBefore); | ||
| expect(codeAfter, "Runtime code should match upgraded wasm").to.equal(rtHex); |
There was a problem hiding this comment.
Add explicit null check before code comparison.
The optional chaining at line 220 could result in codeAfter being undefined. If storage retrieval fails, the assertion at line 235 would produce a confusing failure message ("Runtime code should match upgraded wasm") rather than indicating that the storage read failed.
const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString();
+ expect(codeAfter, "Failed to read :code storage after upgrade").to.not.be.undefined;
log(
`Before: #${blockNumberBefore}, After: #${blockNumberAfter}, code changed=${
codeAfter !== codeString📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString(); | |
| log( | |
| `Before: #${blockNumberBefore}, After: #${blockNumberAfter}, code changed=${ | |
| codeAfter !== codeString | |
| }` | |
| ); | |
| log( | |
| `Code (before): ${codeString.slice(0, 10)}...${codeString.slice(-10)}, code (after): ${ | |
| codeAfter ? codeAfter.slice(0, 10) + "..." + codeAfter.slice(-10) : "undefined" | |
| }` | |
| ); | |
| expect( | |
| blockNumberAfter, | |
| "Block number did not increase" | |
| ).to.be.greaterThan(blockNumberBefore); | |
| expect(codeAfter, "Runtime code should match upgraded wasm").to.equal(rtHex); | |
| const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString(); | |
| expect(codeAfter, "Failed to read :code storage after upgrade").to.not.be.undefined; | |
| log( | |
| `Before: #${blockNumberBefore}, After: #${blockNumberAfter}, code changed=${ | |
| codeAfter !== codeString | |
| }` | |
| ); | |
| log( | |
| `Code (before): ${codeString.slice(0, 10)}...${codeString.slice(-10)}, code (after): ${ | |
| codeAfter ? codeAfter.slice(0, 10) + "..." + codeAfter.slice(-10) : "undefined" | |
| }` | |
| ); | |
| expect( | |
| blockNumberAfter, | |
| "Block number did not increase" | |
| ).to.be.greaterThan(blockNumberBefore); | |
| expect(codeAfter, "Runtime code should match upgraded wasm").to.equal(rtHex); |
🤖 Prompt for AI Agents
In
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
around lines 220 to 235, the optional chaining when reading storage can yield
undefined and the subsequent equality assertion will give a misleading message;
add an explicit null/undefined check for codeAfter immediately after retrieving
it and fail with a clear error if storage read returned undefined (e.g.,
expect(codeAfter, "Failed to read :code storage after
upgrade").to.not.be.undefined), then proceed to compare the non-null codeAfter
value to rtHex using the existing equality assertion.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (3)
184-215: Simplify dead conditional and document waitBlock timing.This issue was flagged in a previous review but remains unaddressed:
1. Lines 189-191 — Dead code: The check for
authorizeUpgrade.meta.args.length === 1will always be true with Polkadot SDK v16.5.2, which definesauthorizeUpgrade(codeHash: H256)with a single argument. The else branch (authorizeUpgrade(wasmHash, true)) is dead code for current SDK versions and creates unnecessary complexity.2. Line 200 — Undocumented wait: The
waitBlock(2)call lacks an explanatory comment. While 2 blocks is appropriate for parachain upgrade workflows (OnSetCode implementation may schedule code change for a future block), this should be documented for future maintainers.Apply this diff to simplify the dead conditional and add documentation:
if (upgradeAvailable) { // Zeitgeist runtime blocks `setCode`, so use the authorized upgrade flow. const wasmHash = blake2AsHex(wasm); log("Authorizing runtime upgrade via system.authorizeUpgrade"); - const authorizeTx = - authorizeUpgrade.meta.args.length === 1 - ? authorizeUpgrade(wasmHash) - : authorizeUpgrade(wasmHash, true); + const authorizeTx = authorizeUpgrade(wasmHash); log( `authorizeUpgrade located in section=${upgradeCallLocation.authorize}, args=${authorizeUpgrade.meta.args.length}` ); await txStatus( paraApi.tx.sudo.sudo(authorizeTx), "authorizeUpgrade" ); - log("Waiting for validation function approval"); + // Wait for OnSetCode scheduling and validation function approval (2 blocks) + log("Waiting for validation function approval (2 blocks)"); await context.waitBlock(2);
121-147: Still missing timeout protection to prevent indefinite hangs.This issue was flagged in a previous review but remains unaddressed. The
txStatushelper lacks timeout protection. If a transaction never reachesisInBlockorisFinalizedstatus (due to network issues, dropped transactions, etc.), the promise will hang indefinitely, causing the test to timeout at the suite level rather than providing a clear failure reason.Consider adding a timeout mechanism:
const txStatus = async (tx: any, label: string) => new Promise<void>((resolve, reject) => { let unsubscribe: (() => void) | undefined; + const timeout = setTimeout(() => { + unsubscribe?.(); + reject(new Error(`${label} timed out after 60s`)); + }, 60000); tx.signAndSend(alice, (result: any) => { if (result.dispatchError) { const errText = result.dispatchError.toString(); log(`${label} dispatchError=${errText}`); + clearTimeout(timeout); reject(new Error(`${label} failed: ${errText}`)); unsubscribe?.(); return; } log( `${label} status=${result.status?.type ?? "unknown"}, events=${result.events ?.map((ev: any) => `${ev.event.section}.${ev.event.method}`) .join(",")}` ); if (result.status?.isInBlock || result.status?.isFinalized) { + clearTimeout(timeout); unsubscribe?.(); resolve(); } }) .then((unsub: () => void) => { unsubscribe = unsub; }) - .catch(reject); + .catch((err) => { + clearTimeout(timeout); + reject(err); + }); });
221-236: Add explicit undefined check before code comparison.This issue was flagged in a previous review but remains unaddressed. The optional chaining at line 221 could result in
codeAfterbeingundefinedif storage retrieval fails. The assertion at line 236 would then produce a confusing failure message ("Runtime code should match upgraded wasm") rather than indicating that the storage read failed.Apply this diff to add an explicit undefined check:
const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString(); + expect(codeAfter, "Failed to read :code storage after upgrade").to.not.be.undefined; log( `Before: #${blockNumberBefore}, After: #${blockNumberAfter}, code changed=${ codeAfter !== codeString }` );
🧹 Nitpick comments (4)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (4)
27-28: Remove unused imports.
u8aConcatandxxhashAsU8aare imported but never used in this file.Apply this diff to remove the unused imports:
-import { u8aConcat } from "@polkadot/util"; -import { blake2AsHex, xxhashAsU8a } from "@polkadot/util-crypto"; +import { blake2AsHex } from "@polkadot/util-crypto";
245-246: Document the hardcoded storage key.The hardcoded hex string for
RelayTimestampNowstorage key lacks documentation. If storage layout changes or a reader needs to verify this key is correct, there's no context about what it represents or how it was derived (e.g.,twox128("RelayTimestampNow")or similar).Consider adding a comment explaining the key derivation:
+ // Storage key for RelayTimestampNow - derived from twox128 hash of storage item name const relayTsStorageKey = "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063";Alternatively, compute the key dynamically if the storage item is queryable via the API.
259-272: Improve retry failure messaging.The retry loop attempts to read a non-zero relay timestamp up to 5 times, but if all retries are exhausted and
tsRelay1remains zero, the test continues to line 279 where the assertion will fail without explaining the retry context. This makes debugging failures more difficult.Consider adding an explicit check after the retry loop:
let tsRelay1 = 0n; let retries = 0; while (tsRelay1 === 0n && retries < 5) { log(`Attempt ${retries + 1}: reading RelayTimestampNow`); tsRelay1 = await readRelayTs(); const rawDirect = await paraApi.rpc.state.getStorage( relayTsStorageKey ); log(`RelayTimestampNow direct RPC read: ${rawDirect?.toHex() ?? "null"}`); if (tsRelay1 === 0n) { await context.waitBlock(1); } retries++; } + + if (tsRelay1 === 0n) { + throw new Error( + `RelayTimestampNow remained zero after ${retries} attempts; relay proofs may not be arriving` + ); + }
280-298: Exact timestamp equality may be fragile.Lines 283 and 298 assert that the parachain timestamp exactly equals the relay timestamp. This is a strong assumption that could be fragile if there's any clock skew, rounding differences, or timing variations in how the relay proof is processed. While this may work in the current implementation, consider whether approximate equality (within a small tolerance) would be more robust for long-term test maintenance.
If exact equality is a strict requirement of the implementation (i.e., parachain timestamp must precisely mirror relay timestamp), consider adding a comment explaining this requirement. Otherwise, consider using a tolerance-based check:
// If using approximate equality: expect( Math.abs(Number(tsPara1 - tsRelay1)), "Parachain timestamp should be close to relay timestamp" ).to.be.lessThan(1000); // within 1 second
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(3 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 (3)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (3)
89-94: LGTM!The specVersion logging provides useful diagnostic context before the upgrade.
149-157: LGTM!The
findCallhelper provides a clean way to dynamically locate calls in the API metadata.
159-183: LGTM!The discovery logic for upgrade calls is well-structured and includes helpful logging for debugging.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (3)
121-147: Missing timeout protection can cause indefinite hangs.The
txStatushelper still lacks timeout protection (as flagged in previous review). If a transaction never reachesisInBlockorisFinalizedstatus, the promise will hang indefinitely.As previously suggested, add timeout mechanism:
const txStatus = async (tx: any, label: string) => new Promise<void>((resolve, reject) => { let unsubscribe: (() => void) | undefined; + const timeout = setTimeout(() => { + unsubscribe?.(); + reject(new Error(`${label} timed out after 60s`)); + }, 60000); tx.signAndSend(alice, (result: any) => { if (result.dispatchError) { const errText = result.dispatchError.toString(); log(`${label} dispatchError=${errText}`); + clearTimeout(timeout); reject(new Error(`${label} failed: ${errText}`)); unsubscribe?.(); return; } log( `${label} status=${result.status?.type ?? "unknown"}, events=${result.events ?.map((ev: any) => `${ev.event.section}.${ev.event.method}`) .join(",")}` ); if (result.status?.isInBlock || result.status?.isFinalized) { + clearTimeout(timeout); unsubscribe?.(); resolve(); } }) .then((unsub: () => void) => { unsubscribe = unsub; }) - .catch(reject); + .catch((err) => { + clearTimeout(timeout); + reject(err); + }); });
184-215: Dead code and undocumented timing remain unresolved.Two issues from previous review are still present:
Lines 189-190: The
meta.args.length === 1check is dead code. Polkadot SDK v16.5.2 definesauthorizeUpgrade(codeHash: H256)with exactly one argument. The else branch (authorizeUpgrade(wasmHash, true)) is unreachable.Line 201: The
waitBlock(2)call lacks documentation explaining why 2 blocks is necessary (OnSetCode scheduling for parachain upgrades).Apply these changes:
- const authorizeTx = - authorizeUpgrade.meta.args.length === 1 - ? authorizeUpgrade(wasmHash) - : authorizeUpgrade(wasmHash, true); + const authorizeTx = authorizeUpgrade(wasmHash); log( `authorizeUpgrade located in section=${upgradeCallLocation.authorize}, args=${authorizeUpgrade.meta.args.length}` ); await txStatus( paraApi.tx.sudo.sudo(authorizeTx), "authorizeUpgrade" ); - log("Waiting for validation function approval"); + log("Waiting for OnSetCode scheduling and block finalization (2 blocks)"); await context.waitBlock(2);
221-236: Missing null check before code comparison.The optional chaining at line 221 can result in
codeAfterbeingundefined. If storage retrieval fails, the assertion at line 236 produces a misleading error message.Add explicit null check as previously suggested:
const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString(); + expect(codeAfter, "Failed to read :code storage after upgrade").to.not.be.undefined; log( `Before: #${blockNumberBefore}, After: #${blockNumberAfter}, code changed=${ codeAfter !== codeString
🧹 Nitpick comments (2)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (2)
259-272: Zero timestamp assumption may be incorrect.The retry loop at line 268 assumes
tsRelay1 === 0nindicates an invalid/missing timestamp. However, if the relay chain uses epoch-based timestamps, zero could theoretically be a valid (albeit unlikely) value. The current retry logic is pragmatic but could mask edge cases.Consider logging when zero is encountered to aid debugging:
if (tsRelay1 === 0n) { + log(`Warning: RelayTimestampNow is zero at attempt ${retries + 1}`); await context.waitBlock(1); }
274-302: Suggest helper for drift calculation.The drift calculation and assertion logic is duplicated at lines 280-284 and 299-302. Extracting this into a helper would improve maintainability.
const checkDrift = (tsPara: bigint, tsRelay: bigint, label: string) => { const drift = tsPara > tsRelay ? tsPara - tsRelay : tsRelay - tsPara; const driftLimitMs = 5_000n; expect(drift, `${label}: Parachain timestamp should be close to relay timestamp`).to.be.lte(driftLimitMs); return drift; }; // Usage: checkDrift(tsPara1, tsRelay1, "First check"); // ... wait blocks ... checkDrift(tsPara2, tsRelay2, "Second check");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integration-tests/configs/zombieZeitgeist.json(1 hunks)integration-tests/moonwall.config.json(2 hunks)integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration-tests/moonwall.config.json
⏰ 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 (5)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (2)
89-94: LGTM: Enhanced upgrade diagnostics.Logging the specVersion and upgrade path provides valuable context for debugging upgrade workflows.
149-157: LGTM: findCall utility is well-structured.The utility cleanly locates runtime calls and returns both the call and its section, which is useful for diagnostics and dynamic call discovery.
integration-tests/configs/zombieZeitgeist.json (3)
31-32: LGTM: Collator port updates are consistent and aligned.The collator RPC port (9954) matches the parachain endpoint configured in
moonwall.config.json(line 68:"ws://127.0.0.1:9954"). The P2P port (33059) and RPC port follow the consistent offset pattern.
13-13: LGTM: RPC port 9957 correctly aligns with moonwall configuration.The charlie node RPC port on line 13 is correctly set to 9957, which matches the
ws://127.0.0.1:9957endpoint inmoonwall.config.jsonfor thezombienet_zeitgeist_upgradeenvironment. Similarly, the bob node RPC port on line 18 (9960) is consistent with the configuration.Also applies to: 18-18
25-26: No action needed. The parachain ID 2092 and chain name "zeitgeist" are correctly configured throughout the codebase:
- Runtime constant in
node/src/main.rs:38definesPOLKADOT_PARACHAIN_ID: u32 = 2092- XCM configuration in
runtime/zeitgeist/src/xcm_config/config.rs:63confirmsID: u32 = 2092These values are consistent with the integration test configuration.
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
Outdated
Show resolved
Hide resolved
| const relayTsStorageKey = | ||
| "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Hardcoded storage key is fragile.
The storage key 0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063 is hardcoded, making the code brittle to storage layout changes. Use the Polkadot API to derive the key dynamically.
Replace the hardcoded key with dynamic derivation:
- const relayTsStorageKey =
- "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063";
+ // Derive storage key: twox_128("RelayDataProvider") + twox_128("RelayTimestampNow")
+ const relayTsStorageKey = u8aConcat(
+ xxhashAsU8a("RelayDataProvider", 128),
+ xxhashAsU8a("RelayTimestampNow", 128)
+ );This explains the previously imported but unused u8aConcat and xxhashAsU8a utilities.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const relayTsStorageKey = | |
| "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063"; | |
| // Derive storage key: twox_128("RelayDataProvider") + twox_128("RelayTimestampNow") | |
| const relayTsStorageKey = u8aConcat( | |
| xxhashAsU8a("RelayDataProvider", 128), | |
| xxhashAsU8a("RelayTimestampNow", 128) | |
| ); |
🤖 Prompt for AI Agents
In
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
around lines 245-246, replace the hardcoded storage key with a dynamic
derivation using the Polkadot API/utility functions: derive the storage key by
hashing the pallet name and storage item name (and any key params) with
xxhashAsU8a and concatenating with u8aConcat (or simply call
api.query.<pallet>.<storage>.key(...)) to produce the correct storage key at
runtime; update the code to use the previously imported u8aConcat and
xxhashAsU8a (or api.query.*.key) so the key adapts to storage layout changes
instead of remaining hardcoded.
Chralt98
changed the title
Chralt98
changed the title
Chralt98
added
s:review-needed
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
integration-tests/configs/zombieZeitgeist.json (1)
7-7: Past relaychain mismatch issue resolved.The relaychain is now correctly configured as
rococo-local, which aligns with the test expectation at line 51 oftest-zombienet-runtime-upgrade.tsthat checks for "rococo" in the relay network specName.integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (4)
188-190: Simplify the conditional—the else branch is dead code.According to Polkadot SDK v16.5.2 documentation,
authorizeUpgradeaccepts a single argument (codeHash: H256). Themeta.args.length === 1check will always be true, making the else branch (authorizeUpgrade(wasmHash, true)) unreachable legacy code.Apply this diff to remove the dead branch:
- const authorizeTx = - authorizeUpgrade.meta.args.length === 1 - ? authorizeUpgrade(wasmHash) - : authorizeUpgrade(wasmHash, true); + const authorizeTx = authorizeUpgrade(wasmHash);
244-245: Replace hardcoded storage key with dynamic derivation.The hardcoded storage key
0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063is fragile and will break if the storage layout changes. Use the Polkadot API to derive the key dynamically or document the derivation.Consider using the API's query interface to derive the key at runtime, or at minimum add a comment explaining the key derivation (e.g., "twox_128('RelayDataProvider') + twox_128('RelayTimestampNow')").
120-146: Add timeout protection to prevent indefinite hangs.The
txStatushelper lacks timeout protection. If a transaction never reachesisInBlockorisFinalized(due to network issues or dropped transactions), the promise will hang indefinitely, causing unclear test failures.Consider adding a timeout mechanism as suggested in previous reviews.
220-235: Add explicit null check before code comparison.The optional chaining at line 220 could result in
codeAfterbeingundefined. The assertion at line 235 would then produce a misleading failure message rather than indicating that storage retrieval failed.Apply this diff:
const codeAfter = (await paraApi.rpc.state.getStorage(":code"))?.toString(); + expect(codeAfter, "Failed to read :code storage after upgrade").to.not.be.undefined; log(
🧹 Nitpick comments (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (1)
199-200: Document the 2-block wait rationale.While the 2-block wait is appropriate for parachain upgrade workflows (allowing OnSetCode scheduling and finalization), the code lacks an explanatory comment.
Add a brief comment:
log("Waiting for validation function approval"); + // Wait for OnSetCode scheduling and block finalization await context.waitBlock(2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
integration-tests/README.md(1 hunks)integration-tests/configs/hydradx.yml(1 hunks)integration-tests/configs/zombieZeitgeist.json(1 hunks)integration-tests/scripts/build-zeitgeist-spec.sh(1 hunks)integration-tests/scripts/download-polkadot.sh(1 hunks)integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration-tests/README.md
🧰 Additional context used
🪛 Shellcheck (0.11.0)
integration-tests/scripts/build-zeitgeist-spec.sh
[warning] 8-8: Quote this to prevent word splitting.
(SC2046)
⏰ 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 (4)
integration-tests/scripts/download-polkadot.sh (1)
193-196: LGTM! Useful optimization for CI/dev workflows.The early exit when
POLKADOT_SKIP_DOWNLOADis set provides a clean way to bypass downloads when binaries are already present, which is valuable for cached CI environments or local development.integration-tests/configs/hydradx.yml (1)
1-5: LGTM! Improved resilience through endpoint redundancy.Converting the endpoint to an array of four providers ensures failover capability and better reliability for integration tests.
integration-tests/scripts/build-zeitgeist-spec.sh (1)
11-69: LGTM! Robust approach to spec generation.The targeted string replacement strategy avoids JSON parsing issues with large integers, and the runtime WASM extraction keeps the upgrade test artifact in sync with genesis code. Good error handling with warning messages when patterns fail.
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (1)
239-303: LGTM! Comprehensive relay timestamp validation.The new test T03 properly validates that:
- Relay timestamp storage exists and is non-zero
- Parachain and relay timestamps are synchronized within a reasonable drift tolerance (5s)
- Relay timestamp increases monotonically across blocks
The retry logic (lines 258-271) handles transient read issues gracefully, and the u64 little-endian decoding is correct.
| set -euo pipefail | ||
|
|
||
| # Always run the commands from the "integration-tests" dir | ||
| cd $(dirname $0)/.. |
There was a problem hiding this comment.
Quote the command substitution to prevent word splitting.
Shellcheck flags the unquoted $(dirname $0) which could break if the path contains spaces.
Apply this diff:
-cd $(dirname $0)/..
+cd "$(dirname "$0")/.."Based on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cd $(dirname $0)/.. | |
| cd "$(dirname "$0")/.." |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 8-8: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In integration-tests/scripts/build-zeitgeist-spec.sh around line 8, the command
substitution cd $(dirname $0)/.. is unquoted which can break when the path
contains spaces; change it to quote the substitution like cd "$(dirname
"$0")/.." (or assign to a variable first) so the resulting path is treated as a
single word and avoid word-splitting.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1456 +/- ##
=======================================
Coverage 93.28% 93.28%
=======================================
Files 181 181
Lines 35049 35049
=======================================
Hits 32696 32696
Misses 2353 2353
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| timeout: 120000, | ||
| test: async function () { | ||
| const relayTsStorageKey = | ||
| "0x54dbd40f5201dbc18b0eed4b2ecd9cc67e2cdf745d68eeb295336330e3a1a063"; |
There was a problem hiding this comment.
Is it okay to hardcode this value? relayTsStorageKey
There was a problem hiding this comment.
It's just a storage key called pallet runtime and storage item name RelayTimestampNow. If it were to change, we would know instantly by this test.
Exactly what we have in /Users/chralt/development/zeitgeist/runtime/common/src/relay_timestamp.rs:
struct RelayTimestampNowPrefix;
impl StorageInstance for RelayTimestampNowPrefix {
const STORAGE_PREFIX: &'static str = "RelayTimestampNow";
fn pallet_prefix() -> &'static str {
"runtime"
}
}
| unsubscribe = unsub; | ||
| }) | ||
| .catch(reject); | ||
| }); |
There was a problem hiding this comment.
Is there a risk of indefinite hangs here if the TX fails to reach isInBlock or isFinalized?
Chralt98
added
s:accepted
What does it do?
Updates the integration tests to verify the relay chain timestamp now storage item has a valid value.
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
zeitgeistpm/polkadot-sdk@87971b3...83df353
paritytech/polkadot-sdk#9262
moonbeam-foundation/polkadot-sdk@ececbc6
Summary by CodeRabbit
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.