Support pocket-ic v9#317
Conversation
WalkthroughSwaps many Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Replica as CLI Replica
participant PIC9 as pocket-ic 9.x (Mops)
participant PIC4 as pocket-ic 4.0.0
User->>Replica: start(version)
alt version == 9.x.x
Replica->>PIC9: PocketIcServerMops.start()
Replica->>PIC9: PocketIcMops.create()
Replica-->>User: ready (9.x)
else version == 4.0.0
Replica->>PIC4: PocketIcServer.start()
Replica->>PIC4: PocketIc.create()
Replica-->>User: ready (4.0.0)
else unsupported
Replica-->>User: error (unsupported version)
end
sequenceDiagram
autonumber
participant Replica as CLI Replica
participant PIC as pocket-ic (4.0.0 | 9.x)
participant Agent as HttpAgent/Actor
participant Canister as Deployed Canister
Replica->>PIC: setupCanister({ wasm, idlFactory }) %% unified call
PIC-->>Replica: { canisterId }
Replica->>Agent: Actor.createActor(idlFactory, agent, canisterId)
Agent-->>Replica: actor
Replica->>Canister: attach logs & post-setup
Replica-->>User: { canisterId, actor }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
cli/commands/owner.ts (1)
28-31: Validate principal input to avoid uncaught parse errors.
Wrap Principal.fromText(...) to give a friendly error instead of throwing.Apply this diff in both addOwner and removeOwner:
- let principal = Principal.fromText(owner); + let principal: Principal; + try { + principal = Principal.fromText(owner); + } catch { + console.error(chalk.red('Error: ') + `Invalid principal: ${owner}`); + process.exit(1); + }Also applies to: 70-73
cli/api/downloadPackageFiles.ts (1)
67-75: Avoid O(n²) array copies when concatenating chunks.
Spreading into a new array on each iteration repeatedly copies data.Apply:
- let data : Array<number> = []; + let data: number[] = []; for (let i = 0n; i < fileMeta.chunkCount; i++) { let chunkRes = await storage.downloadChunk(fileId, i); if ('err' in chunkRes) { throw chunkRes.err; } let chunk = chunkRes.ok; - data = [...data, ...chunk]; + data.push(...chunk); }cli/declarations/bench/index.js (1)
15-16: Replacenew HttpAgentwithawait HttpAgent.createincreateActor
TheHttpAgentconstructor is deprecated in v4.x; switch to the async factory.--- a/cli/declarations/bench/index.js +++ b/cli/declarations/bench/index.js @@ -1,6 +1,7 @@ -export const createActor = (canisterId, options = {}) => { - const agent = options.agent || new HttpAgent({ ...options.agentOptions }); +export const createActor = async (canisterId, options = {}) => { + const agent = options.agent || await HttpAgent.create({ ...options.agentOptions }); const actor = Actor.createActor(idlFactory, { agent, canisterId, @@ -}; +};Update all call sites of
createActorto handle its returned Promise.cli/commands/test/test.ts (1)
58-65: Don’t swallow replica.start() rejections.
Wrapping in a Promise without catch can leave replicaStartPromise stuck pending on failure.async function startReplicaOnce(replica : Replica, type : ReplicaName) { if (!replicaStartPromise) { - replicaStartPromise = new Promise((resolve) => { - replica.start({type, silent: true}).then(resolve); - }); + replicaStartPromise = replica + .start({type, silent: true}) + .catch((e) => { + replicaStartPromise = undefined; + throw e; + }); } return replicaStartPromise; }cli/api/actors.ts (1)
21-28: Remove unsupportedshouldSyncTimeoption from HttpAgent.create call.
TheshouldSyncTimefield isn’t defined in @icp-sdk/core v4.x’s HttpAgentOptions; retain only valid options likeshouldFetchRootKeyandverifyQuerySignatures.cli/commands/maintainer.ts (2)
28-31: Validate and fail fast on invalid Principal text.Wrap
Principal.fromTextin try/catch to provide a clear error and non-zero exit.- let principal = Principal.fromText(maintainer); + let principal: Principal; + try { + principal = Principal.fromText(maintainer); + } catch { + console.error(chalk.red('Invalid principal: ') + maintainer); + process.exit(1); + }
70-73: Do the same validation in removeMaintainer.Mirror the error handling for symmetry.
- let principal = Principal.fromText(maintainer); + let principal: Principal; + try { + principal = Principal.fromText(maintainer); + } catch { + console.error(chalk.red('Invalid principal: ') + maintainer); + process.exit(1); + }cli/declarations/storage/index.d.ts (1)
15-28: Keep JS and d.ts in sync:agent?: Agentmust be honored at runtimeThe declaration allows passing a pre-configured
agent?: Agent, butstorage/index.jsignores it and always constructs a newHttpAgent. Please update the JS to respectoptions.agent(and warn if both agent and agentOptions are provided), mirroringmain/index.js.cli/declarations/storage/index.js (1)
13-15: Honor provided Agent; warn on mixed optionsConstructing a new
HttpAgentunconditionally breaks callers passing a custom Agent/identity.-export const createActor = (canisterId, options) => { - const agent = new HttpAgent({ ...options?.agentOptions }); +export const createActor = (canisterId, options = {}) => { + const agent = options.agent || new HttpAgent({ ...options.agentOptions }); + if (options.agent && options.agentOptions) { + console.warn( + "Detected both agent and agentOptions passed to createActor. Ignoring agentOptions and proceeding with the provided agent." + ); + }
🧹 Nitpick comments (9)
cli/commands/owner.ts (1)
16-17: Guard against empty package name before calling the backend.
Right now '' may be sent to canister methods if name is missing.Apply this pattern in printOwners/addOwner/removeOwner before actor calls:
- let owners = await actor.getPackageOwners(config.package?.name || ''); + const pkg = config.package?.name; + if (!pkg) { + console.error(chalk.red('Error: ') + `Package name is missing in mops.toml`); + process.exit(1); + } + let owners = await actor.getPackageOwners(pkg);And similarly replace occurrences in addOwner/removeOwner:
- let res = await actor.addOwner(config.package?.name || '', principal); + let res = await actor.addOwner(pkg, principal);- let res = await actor.removeOwner(config.package?.name || '', principal); + let res = await actor.removeOwner(pkg, principal);Also applies to: 54-55, 96-97
cli/mops.ts (1)
49-58: ESM-safe __dirname fallback.
In ESM, __dirname is undefined; while the try branch should usually succeed, make the fallback ESM-safe to avoid surprises.- catch { - networkFile = path.join(__dirname, 'network.txt'); - } + catch { + const {fileURLToPath} = await import('node:url'); + const dirname = path.dirname(fileURLToPath(import.meta.url)); + networkFile = path.join(dirname, 'network.txt'); + }cli/commands/replica.ts (3)
91-93: Use semver for the version guard and echo the offending version.String checks can misfire (e.g., “9” or “^9.0.3”). Prefer semver.
- if (config.toolchain?.['pocket-ic'] !== '4.0.0' && !config.toolchain?.['pocket-ic']?.startsWith('9.')) { - console.error('Current Mops CLI only supports pocket-ic 9.x.x and 4.0.0'); + const picVer = config.toolchain?.['pocket-ic'] ?? ''; + if (! (semver.eq(picVer, '4.0.0') || semver.satisfies(picVer, '>=9.0.0 <10'))) { + console.error(`Current Mops CLI only supports pocket-ic 9.x.x and 4.0.0; found "${picVer}"`); process.exit(1); }Add this import at the top of the file:
import semver from 'semver';
96-115: Tie PIC runtime logs to theverboseflag.Make logs visible only when requested.
- this.pocketIcServer = await PocketIcServerMops.start({ - showRuntimeLogs: false, - showCanisterLogs: false, + this.pocketIcServer = await PocketIcServerMops.start({ + showRuntimeLogs: this.verbose, + showCanisterLogs: this.verbose, binPath: pocketIcBin, ttl: this.ttl, }); @@ - this.pocketIcServer = await PocketIcServer.start({ - showRuntimeLogs: false, - showCanisterLogs: false, + this.pocketIcServer = await PocketIcServer.start({ + showRuntimeLogs: this.verbose, + showCanisterLogs: this.verbose, binPath: pocketIcBin, ttl: this.ttl, });
241-245: Avoid intersecting function types forsetupCanistercast.Use an explicit, minimal signature for stability across libs.
- type PocketIcSetupCanister = PocketIcMops['setupCanister'] & PocketIc['setupCanister']; - let {canisterId, actor} = await (this.pocketIc.setupCanister as PocketIcSetupCanister)({ - wasm, - idlFactory, - }); + type SetupCanisterFn = (args: { wasm: string; idlFactory: IDL.InterfaceFactory }) => + Promise<{ canisterId: { toText(): string } | string; actor: unknown }>; + const setup = this.pocketIc.setupCanister as unknown as SetupCanisterFn; + const { canisterId, actor } = await setup({ wasm, idlFactory });cli/declarations/main/index.js (1)
24-33: Unify root-key fetch condition across declarationsHere you gate on
DFX_NETWORK !== "ic", whilestorage/index.jsusesNODE_ENV !== "production". Recommend standardizing on one (preferDFX_NETWORKlike dfx scaffolding) to avoid surprise root-key fetches.- if (process.env.DFX_NETWORK !== "ic") { + if (process.env.DFX_NETWORK !== "ic") { agent.fetchRootKey().catch((err) => { console.warn( "Unable to fetch root key. Check to ensure that your local replica is running" ); console.error(err); }); }cli/declarations/storage/index.d.ts (1)
30-40: Doc typos and namingFix minor typos for polish and IDE hints:
- “Intializes” → “Initializes”
- “ActorSubClass” → “ActorSubclass”
- “Intialized” → “Initialized”
- “Supercedes” → “Supersedes”
- * Intializes an {@link ActorSubclass}, configured with the provided SERVICE interface of a canister. - * @constructs {@link ActorSubClass} + * Initializes an {@link ActorSubclass}, configured with the provided SERVICE interface of a canister. + * @constructs {@link ActorSubclass} - * Intialized Actor using default settings, ready to talk to a canister using its candid interface + * Initialized Actor using default settings, ready to talk to a canister using its candid interface - * @param {CreateActorOptions["agent"]} options.agent - a pre-configured agent you'd like to use. Supercedes agentOptions + * @param {CreateActorOptions["agent"]} options.agent - a pre-configured agent you'd like to use. Supersedes agentOptionscli/declarations/main/index.d.ts (1)
30-40: Doc typos and namingSame nitpicks as storage d.ts: “Intializes” → “Initializes”, “ActorSubClass” → “ActorSubclass”, “Supercedes” → “Supersedes”.
- * Intializes an {@link ActorSubclass}, configured with the provided SERVICE interface of a canister. - * @constructs {@link ActorSubClass} + * Initializes an {@link ActorSubclass}, configured with the provided SERVICE interface of a canister. + * @constructs {@link ActorSubclass} - * @param {CreateActorOptions["agent"]} options.agent - a pre-configured agent you'd like to use. Supercedes agentOptions + * @param {CreateActorOptions["agent"]} options.agent - a pre-configured agent you'd like to use. Supersedes agentOptionscli/declarations/storage/index.js (1)
16-22: Standardize dev root-key fetch conditionUse the same condition as
main/index.js(DFX_NETWORK !== "ic") to avoid fetching root key on non-local setups whenNODE_ENV=development.- if(process.env.NODE_ENV !== "production") { + if (process.env.DFX_NETWORK !== "ic") { agent.fetchRootKey().catch(err=>{ console.warn("Unable to fetch root key. Check to ensure that your local replica is running"); console.error(err); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/bun.lockis excluded by!**/*.lockcli/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
cli/api/actors.ts(1 hunks)cli/api/downloadPackageFiles.ts(1 hunks)cli/commands/maintainer.ts(1 hunks)cli/commands/owner.ts(1 hunks)cli/commands/replica.ts(4 hunks)cli/commands/test/test.ts(1 hunks)cli/declarations/bench/bench.did.d.ts(1 hunks)cli/declarations/bench/index.d.ts(1 hunks)cli/declarations/bench/index.js(1 hunks)cli/declarations/main/index.d.ts(1 hunks)cli/declarations/main/index.js(1 hunks)cli/declarations/main/main.did.d.ts(1 hunks)cli/declarations/storage/index.d.ts(1 hunks)cli/declarations/storage/index.js(1 hunks)cli/declarations/storage/storage.did.d.ts(1 hunks)cli/mops.ts(1 hunks)cli/package.json(2 hunks)cli/pem.ts(1 hunks)cli/tsconfig.json(1 hunks)mops.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: node 20, moc 0.14.13, ic-mops@1.0.0
🔇 Additional comments (24)
cli/commands/owner.ts (1)
5-5: Import migration looks good.
Principal import aligned with the repo-wide switch to @icp-sdk/core.cli/tsconfig.json (1)
11-12: Module settings changed; confirm toolchain compatibility.
Switching to "module": "ES2022" and "moduleResolution": "bundler" can affect Node/ts-node and bundlers. Verify builds and runtime ESM behavior across CLI.Run:
#!/bin/bash set -euo pipefail # Compile CLI with the updated tsconfig tsc -p cli/tsconfig.json --noEmit # Grep for CommonJS-only globals (__dirname/__filename) in ESM files rg -nP '(__dirname|__filename)\b' cli --type=ts --type=js -g '!**/dist/**' -g '!**/bundle/**'cli/api/downloadPackageFiles.ts (1)
1-1: Import migration looks good.
Principal import aligned with @icp-sdk/core.cli/mops.ts (1)
4-4: Import migration looks good.
Identity import updated to @icp-sdk/core/agent.cli/declarations/bench/index.js (1)
1-1: Import migration looks good.
Actor/HttpAgent imported from @icp-sdk/core/agent.mops.toml (1)
21-22: Pocket-IC toolchain pinned.
Adding pocket-ic = "9.0.3" looks correct for PIC 9 support; wasmtime is already >=14.cli/commands/test/test.ts (1)
13-13: Import migration looks good.
ActorMethod type now sourced from @icp-sdk/core/agent.cli/api/actors.ts (1)
1-2: Import migration looks good.
Agent/Actor/Principal imports updated to @icp-sdk/core paths.cli/commands/maintainer.ts (1)
5-5: SDK import migration looks good.
Principalimport path change is correct and non-breaking.cli/package.json (1)
48-49: Deps switch to @icp-sdk/core and pic-js-mops looks right; verify no stray @dfinity imports remain.#!/bin/bash # Ensure no remaining DFINITY imports in CLI sources rg -nP '@dfinity/' -g 'cli/**' -g '!**/node_modules/**' || echo 'OK: no @dfinity imports in cli/' # Sanity: confirm both PIC libs are referenced only in replica.ts rg -nP '\bfrom\s+[\'"](pic-ic|pic-js-mops)[\'"]' -n cli/commands/replica.ts # Confirm pinned ICP SDK version in package.json jq -r '.dependencies["@icp-sdk/core"]' cli/package.jsonAlso applies to: 80-81
cli/pem.ts (1)
4-5: Identity import migration LGTM.Compatible replacements for
Ed25519KeyIdentityandSecp256k1KeyIdentity.cli/declarations/storage/storage.did.d.ts (1)
1-3: Type import source updates are correct.No surface/type changes beyond module paths.
cli/declarations/bench/bench.did.d.ts (1)
1-3: Type import migration looks good.cli/declarations/bench/index.d.ts (1)
6-8: Agent/Principal/IDL type imports correctly moved to ICP SDK core.cli/commands/replica.ts (3)
8-12: ICP SDK core + dual PIC imports look correct.Import set aligns with the new runtime support.
29-31: Union-typing PIC server/client fields is fine.Keeps the rest of the class unchanged at call sites.
251-251: ```shellSearch for addCycles method definitions across TypeScript files
rg -nP --type=ts 'addCycles(' -C3
Locate pocketIc instantiation or type import
rg -nP --type=ts 'pocketIc' -C3
</blockquote></details> <details> <summary>cli/declarations/main/main.did.d.ts (1)</summary><blockquote> `1-3`: **Import path migration complete; IDL API parity confirmed** The `@icp-sdk/core/candid` package re-exports the Candid `IDL` namespace—including `IDL.InterfaceFactory`, `IDL.Type[]`, and `typeof IDL`—matching the previous API surface. </blockquote></details> <details> <summary>cli/declarations/main/index.js (2)</summary><blockquote> `1-1`: **LGTM: moved imports to ICP SDK Core** Import path update to `@icp-sdk/core/agent` is correct; no behavior change. --- `34-41`: **Type/runtime mismatch: `main` export declared in d.ts but not here** `cli/declarations/main/index.d.ts` declares `export const main: ActorSubclass<_SERVICE>;` but this file does not export `main`. Either export it or remove from d.ts. If you want to keep it, append: ```diff return Actor.createActor(idlFactory, { agent, canisterId, ...options.actorOptions, }); }; + +// Optional: keep in sync with index.d.ts +export const main = createActor(canisterId);Run to find any imports relying on
main:#!/bin/bash rg -nP -C2 'import\s+\{[^}]*\bmain\b[^}]*\}\s+from\s+["\'][^"\']*declarations/(main|main/index)["\']'cli/declarations/storage/index.d.ts (1)
6-9: LGTM: type imports migrated to ICP SDK CoreThe new import sources line up with ICP SDK Core typings.
cli/declarations/main/index.d.ts (2)
1-8: LGTM: migrated to ICP SDK Core type importsTypes point to
@icp-sdk/core/*; signatures unchanged.
46-50: Type/runtime mismatch:maindeclared here but not exported in JS
index.jsdoesn’t exportmain. Either export it in JS or drop it here to prevent runtime import errors.#!/bin/bash # Confirm mismatch rg -n 'export\s+const\s+main\b' cli/declarations/main/index.jscli/declarations/storage/index.js (1)
1-6: Missing exports vs d.ts:canisterIdandstorage
cli/declarations/storage/index.d.tsdeclarescanisterIdandstorage, but this file doesn’t export them. Export to match types (or adjust d.ts).import { idlFactory } from './storage.did.js'; export { idlFactory } from './storage.did.js'; +/* CANISTER_ID is replaced by bundlers based on environment (dfx ≥0.15 standardizes naming) */ +export const canisterId = process.env.CANISTER_ID_STORAGE; + /** * * @param {string | import("@icp-sdk/core/principal").Principal} canisterId Canister ID of Agent @@ return Actor.createActor(idlFactory, { agent, canisterId, ...options?.actorOptions, }); }; + +// Keep JS in sync with d.ts +export const storage = createActor(canisterId);To see if anything imports these today:
#!/bin/bash rg -nP -C2 'import\s+\{[^}]*\b(storage|canisterId)\b[^}]*\}\s+from\s+["\'][^"\']*declarations/storage'
| /** | ||
| * | ||
| * @param {string | import("@dfinity/principal").Principal} canisterId Canister ID of Agent | ||
| * @param {{agentOptions?: import("@dfinity/agent").HttpAgentOptions; actorOptions?: import("@dfinity/agent").ActorConfig}} [options] | ||
| * @return {import("@dfinity/agent").ActorSubclass<import("./storage.did.js")._SERVICE>} | ||
| * @param {string | import("@icp-sdk/core/principal").Principal} canisterId Canister ID of Agent | ||
| * @param {{agentOptions?: import("@icp-sdk/core/agent").HttpAgentOptions; actorOptions?: import("@icp-sdk/core/agent").ActorConfig}} [options] | ||
| * @return {import("@icp-sdk/core/agent").ActorSubclass<import("./storage.did.js")._SERVICE>} | ||
| */ |
There was a problem hiding this comment.
JSDoc and runtime options out of sync with d.ts
The JSDoc omits options.agent and the implementation ignores it. Align with d.ts and main/index.js: accept agent, warn if agentOptions is also provided, and use the provided agent.
- * @param {{agentOptions?: import("@icp-sdk/core/agent").HttpAgentOptions; actorOptions?: import("@icp-sdk/core/agent").ActorConfig}} [options]
- * @return {import("@icp-sdk/core/agent").ActorSubclass<import("./storage.did.js")._SERVICE>}
+ * @param {{agent?: import("@icp-sdk/core/agent").Agent; agentOptions?: import("@icp-sdk/core/agent").HttpAgentOptions; actorOptions?: import("@icp-sdk/core/agent").ActorConfig}} [options]
+ * @return {import("@icp-sdk/core/agent").ActorSubclass<import("./storage.did.js")._SERVICE>}📝 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.
| /** | |
| * | |
| * @param {string | import("@dfinity/principal").Principal} canisterId Canister ID of Agent | |
| * @param {{agentOptions?: import("@dfinity/agent").HttpAgentOptions; actorOptions?: import("@dfinity/agent").ActorConfig}} [options] | |
| * @return {import("@dfinity/agent").ActorSubclass<import("./storage.did.js")._SERVICE>} | |
| * @param {string | import("@icp-sdk/core/principal").Principal} canisterId Canister ID of Agent | |
| * @param {{agentOptions?: import("@icp-sdk/core/agent").HttpAgentOptions; actorOptions?: import("@icp-sdk/core/agent").ActorConfig}} [options] | |
| * @return {import("@icp-sdk/core/agent").ActorSubclass<import("./storage.did.js")._SERVICE>} | |
| */ | |
| /** | |
| * | |
| * @param {string | import("@icp-sdk/core/principal").Principal} canisterId Canister ID of Agent | |
| * @param {{agent?: import("@icp-sdk/core/agent").Agent; agentOptions?: import("@icp-sdk/core/agent").HttpAgentOptions; actorOptions?: import("@icp-sdk/core/agent").ActorConfig}} [options] | |
| * @return {import("@icp-sdk/core/agent").ActorSubclass<import("./storage.did.js")._SERVICE>} | |
| */ |
🤖 Prompt for AI Agents
In cli/declarations/storage/index.js around lines 7 to 12, the JSDoc and
implementation currently omit support for an `agent` option and ignore it at
runtime; update the JSDoc to include options.agent (typed to
import("@icp-sdk/core/agent").HttpAgent or similar) and modify the factory
implementation to accept an `agent` parameter: if `agent` is provided, use it to
create the actor; if both `agent` and `agentOptions` are provided, emit a
console.warn indicating `agent` takes precedence (for backward compatibility),
and only fall back to constructing a new agent from `agentOptions` when `agent`
is not supplied. Ensure types in JSDoc match the .d.ts and main/index.js
behavior and keep the existing actorOptions handling unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/commands/replica.ts (1)
241-262: Remove unsafe any-cast, normalize canisterId, and pass BigInt cyclesaddCycles typically requires bigint (and in legacy pic-ic a Principal). Normalize ID once and avoid any-cast.
- type PocketIcSetupCanister = PocketIcMops['setupCanister'] & PocketIc['setupCanister']; - let {canisterId, actor} = await (this.pocketIc.setupCanister as PocketIcSetupCanister)({ - wasm, - idlFactory, - }); + type PocketIcSetupCanister = PocketIcMops['setupCanister'] & PocketIc['setupCanister']; + const {canisterId, actor} = await (this.pocketIc.setupCanister as PocketIcSetupCanister)({ + wasm, + idlFactory, + }); + const idText = typeof canisterId === 'string' ? canisterId : canisterId.toText(); - await this.pocketIc.addCycles(canisterId as any, 1_000_000_000_000); + await this.pocketIc.addCycles( + // pic-js-mops accepts string; pic-ic 4.x accepts Principal + (typeof canisterId === 'string' ? canisterId : canisterId), + 1_000_000_000_000n + ); this.canisters[name] = { cwd, - canisterId: canisterId.toText(), + canisterId: idText, actor, stream: new PassThrough(), };If needed for 4.x, import Principal and convert:
import {Principal} from '@icp-sdk/core/principal'; // then pass Principal.fromText(idText) for addCycles in the 4.x path.
🧹 Nitpick comments (2)
cli/commands/replica.ts (2)
29-30: Consider a discriminant to ease type narrowing for pocket-ic flavorsThe unions are fine. Add a discriminant (e.g., pocketIcFlavor: 'mops' | 'legacy') during start() to avoid casts later (deploy/log handling).
export class Replica { ... - pocketIcServer ?: PocketIcServer | PocketIcServerMops; - pocketIc ?: PocketIc | PocketIcMops; + pocketIcServer ?: PocketIcServer | PocketIcServerMops; + pocketIc ?: PocketIc | PocketIcMops; + pocketIcFlavor?: 'mops' | 'legacy';And set it in the 9.x vs 4.x branches.
91-93: Version gate is too strict; allow 4.x and handle missing config via binary introspectionToday this hard-fails when mops.toml omits pocket-ic or if a 4.0.x patch appears.
- if (config.toolchain?.['pocket-ic'] !== '4.0.0' && !config.toolchain?.['pocket-ic']?.startsWith('9.')) { - console.error('Current Mops CLI only supports pocket-ic 9.x.x and 4.0.0'); + const configured = config.toolchain?.['pocket-ic']; + if (configured && !/^9\./.test(configured) && !/^4\./.test(configured)) { + console.error('Current Mops CLI only supports pocket-ic 9.x.x and 4.x'); process.exit(1); }Optional: if configured is falsy, detect via
${pocketIcBin} --versionand branch accordingly.#!/bin/bash "$POCKET_IC_BIN" --version || pocket-ic --version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/bun.lockis excluded by!**/*.lockcli/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
cli/commands/replica.ts(4 hunks)cli/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/package.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: node 20, moc 0.14.13, ./cli/dist
🔇 Additional comments (3)
cli/commands/replica.ts (3)
8-12: Confirm @icp-sdk/core import surfaces and HttpAgent factory APIValidate that Actor and HttpAgent.create are exported from @icp-sdk/core/agent in your pinned version. Some builds expose new HttpAgent(...) instead of a create factory.
Run:
#!/bin/bash npm ls @icp-sdk/core rg -nP "from '@icp-sdk/core/agent'" -C2 --type ts rg -nP '\bHttpAgent\.create\s*\(' --type tsOr check docs:
Does @icp-sdk/core v4.0.2 export Actor and an HttpAgent.create factory from @icp-sdk/core/agent? Cite official docs.
96-105: Validate Mops server API parity (options, .getUrl(), .serverProcess presence)Double-check start options (showRuntimeLogs, showCanisterLogs, ttl units) and that PocketIcServerMops exposes getUrl() and serverProcess used by the log handler.
#!/bin/bash rg -nP 'class\s+PocketIcServer' "$(npm root)"/pic-js-mops/** -C3 || true rg -nP 'getUrl\s*\(' "$(npm root)"/pic-js-mops/** -C1 || true rg -nP 'serverProcess' "$(npm root)"/pic-js-mops/** -C1 || true
106-115: Parity check with legacy pic-ic 4.xEnsure the option names and ttl units match pic-ic 4.x expectations; mismatches can silently disable logs or TTL.
#!/bin/bash rg -nP 'class\s+PocketIcServer' "$(npm root)"/pic-ic/** -C3 || true rg -nP 'start\s*\(\s*\{' "$(npm root)"/pic-ic/** -C3 || true
Benchmark Resultsbench/1-buffer-vector-add.bench.mo
|
| 10 | 10000 | 1000000 | |
|---|---|---|---|
| Buffer | 9_493 |
5_687_530 |
525_783_824 |
| Vector | 13_461 |
4_378_548 |
417_864_434 |
Heap
| 10 | 10000 | 1000000 | |
|---|---|---|---|
| Buffer | 272 B |
272 B |
272 B |
| Vector | 272 B |
272 B |
272 B |
Garbage Collection
| 10 | 10000 | 1000000 | |
|---|---|---|---|
| Buffer | 1.09 KiB |
143.28 KiB |
12.02 MiB |
| Vector | 1.09 KiB |
45.65 KiB |
3.86 MiB |
bench/2-vector-buffer-add.bench.mo $({\color{green}-10.20\%})$
Add
Add items one-by-one
Instructions:
Heap:
Stable Memory:
Garbage Collection:
Instructions
| 10 | 10000 | 1000000 | |
|---|---|---|---|
| Vector | 13_461 |
4_378_902 |
417_886_028 |
| Buffer | 9_493 |
5_686_822 |
525_780_992 |
Heap
| 10 | 10000 | 1000000 | |
|---|---|---|---|
| Vector | 272 B |
272 B |
272 B |
| Buffer | 272 B |
272 B |
272 B |
Garbage Collection
| 10 | 10000 | 1000000 | |
|---|---|---|---|
| Vector | 1.09 KiB |
45.65 KiB |
3.86 MiB |
| Buffer | 1.09 KiB |
143.28 KiB |
12.02 MiB |
bench/array.bench.mo $({\color{green}-18.10\%})$
Array
arr arr
Instructions:
Heap:
Stable Memory:
Garbage Collection:
Instructions
| 100k x1 | reset1 | 100k x3 | reset2 | 100k x4 | reset3 | |
|---|---|---|---|---|---|---|
| Array | 13_502_032 |
3_271 |
27_003_206 |
3_745 |
54_004_063 |
4_219 |
Heap
| 100k x1 | reset1 | 100k x3 | reset2 | 100k x4 | reset3 | |
|---|---|---|---|---|---|---|
| Array | 390.9 KiB |
-390.37 KiB |
390.9 KiB |
-390.37 KiB |
390.9 KiB |
-390.37 KiB |
Garbage Collection
| 100k x1 | reset1 | 100k x3 | reset2 | 100k x4 | reset3 | |
|---|---|---|---|---|---|---|
| Array | 360 B |
390.97 KiB |
391 KiB |
390.97 KiB |
1.14 MiB |
390.97 KiB |
bench/removeLast.bench.mo $({\color{green}-10.90\%})$
Remove items using removeLast
Vector and buffer are initialized with 100k items and then 70k items are removed one-by-one.
Instructions:
Heap:
Stable Memory:
Garbage Collection:
Instructions
| remove 70k | |
|---|---|
| Vector | 27_707_652 |
| Buffer | 29_236_913 |
Heap
| remove 70k | |
|---|---|
| Vector | -136.8 KiB |
| Buffer | -269.76 KiB |
Garbage Collection
| remove 70k | |
|---|---|
| Vector | 139.45 KiB |
| Buffer | 540.43 KiB |
bench/stable-memory.bench.mo $({\color{green}-134.72\%})$
Stable Memory and Region
Grow Region and store blobs in it
Instructions:
Heap:
Stable Memory:
Garbage Collection:
Instructions
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 2_626_901 |
10_496_214 |
2_629 |
| 100 pages | 52_466_697 |
104_914_690 |
2_634 |
| 256 pages | 134_273_582 |
268_575_103 |
3_182 |
Heap
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 272 B |
272 B |
276 B |
| 100 pages | 272 B |
272 B |
272 B |
| 256 pages | 272 B |
272 B |
276 B |
Garbage Collection
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 208.34 KiB |
832.38 KiB |
336 B |
| 100 pages | 4.06 MiB |
8.13 MiB |
340 B |
| 256 pages | 10.4 MiB |
20.8 MiB |
340 B |
Stable Memory
| Region (fill 1/100) | Region (fill 1/50) | StableMemory | |
|---|---|---|---|
| 10 pages | 8 MiB |
8 MiB |
8 MiB |
| 100 pages | 8 MiB |
8 MiB |
0 B |
| 256 pages | 16 MiB |
16 MiB |
16 MiB |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/storage-actor.test.mo (5)
1-1: Remove unused importskip.It’s not referenced.
-import {test; suite; skip} "mo:test/async"; +import {test; suite} "mo:test/async";
2-5: Import Array to perform safe array checks later.Needed for robust owners checks below.
import Result "mo:base/Result"; import Blob "mo:base/Blob"; import Debug "mo:base/Debug"; import ExperimentalCycles "mo:base/ExperimentalCycles"; +import Array "mo:base/Array";
16-16: Drop noisy debug print.Keeps test output clean.
- Debug.print("lalalalalalala");
20-27: Strengthen negative assertions by matching the error variant.
Result.isErrhides the reason; asserting on#err _makes failures clearer.- await test("try to finish upload before upload start", func() : async () { - let res = await storage.finishUploads([fileId]); - assert Result.isErr(res); - }); + await test("try to finish upload before upload start", func() : async () { + let res = await storage.finishUploads([fileId]); + switch (res) { case (#err _) {}; case (#ok _) { assert false } }; + }); - await test("try to upload chunk before upload start", func() : async () { - assert Result.isErr(await storage.uploadChunk(fileId, 0, Blob.fromArray([]))); - }); + await test("try to upload chunk before upload start", func() : async () { + let res = await storage.uploadChunk(fileId, 0, Blob.fromArray([])); + switch (res) { case (#err _) {}; case (#ok _) { assert false } }; + });
29-37: Cover the happy path: upload a chunk before finishing.Right now the upload is finished without uploading any chunk; add a positive chunk upload to validate the main path.
await test("start upload", func() : async () { assert Result.isOk(await storage.startUpload({ id = fileId; path = "test/test.mo"; chunkCount = 1; owners = []; })); }); + + await test("upload a chunk after start", func() : async () { + assert Result.isOk(await storage.uploadChunk(fileId, 0, Blob.fromArray([1]))); + }); await test("finish upload", func() : async () { assert Result.isOk(await storage.finishUploads([fileId])); });Also applies to: 42-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/bun.lockis excluded by!**/*.lockcli/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
cli/package.json(3 hunks)test/storage-actor.test.mo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/package.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). (2)
- GitHub Check: Run Benchmarks and Comment
- GitHub Check: node 20, moc 0.14.13, ic-mops@1.0.0
🔇 Additional comments (2)
test/storage-actor.test.mo (2)
80-82: Add cycles before upgrade (some setups require it).Safer for pocket-ic v9/system upgrades.
await test("upgrade storage canister", func() : async () { + ExperimentalCycles.add<system>(1_000_000_000_000); storage := await (system Storage.Storage)(#upgrade(storage))(); });If upgrades in your env don’t require cycles, this is a no-op; otherwise it prevents transient failures.
10-13: Confirm test entrypoint naming
Tests in test/storage-actor.test.mo (importing mo:test/async) currently definepublic func runTests() : async () { … }Many Mocha-inspired or motoko test runners look for
public func run() : async ()as the suite entrypoint—if your harness doesn’t invoke runTests, these tests will be skipped. Verify the mo:test/async runner’s docs and, if needed, rename:
- public func runTests() : async () { + public func run() : async () {
| await test("get file meta", func() : async () { | ||
| let res = await storage.getFileMeta(fileId); | ||
| assert Result.isOk(res); | ||
| switch (res) { | ||
| case (#ok(fileMeta)) { | ||
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use array-size check; array equality is unreliable.
Comparing arrays with == is not robust in Motoko. Check length (or element-wise) instead.
switch (res) {
case (#ok(fileMeta)) {
assert fileMeta.path == "test/test.mo";
assert fileMeta.chunkCount == 1;
- assert fileMeta.owners == [];
+ assert Array.size(fileMeta.owners) == 0;
};
case (_) {};
};📝 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.
| await test("get file meta", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| await test("get file meta", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert Array.size(fileMeta.owners) == 0; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In test/storage-actor.test.mo around lines 62 to 73, the test compares
fileMeta.owners to [] using == which is unreliable for arrays in Motoko; replace
that equality check with a length check (e.g., assert
Array.size(fileMeta.owners) == 0) or an explicit element-wise check so the test
verifies the owners array is empty reliably.
| await test("get file meta after upgrade", func() : async () { | ||
| let res = await storage.getFileMeta(fileId); | ||
| assert Result.isOk(res); | ||
| switch (res) { | ||
| case (#ok(fileMeta)) { | ||
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeat owners check fix after upgrade.
Same reason as earlier.
switch (res) {
case (#ok(fileMeta)) {
assert fileMeta.path == "test/test.mo";
assert fileMeta.chunkCount == 1;
- assert fileMeta.owners == [];
+ assert Array.size(fileMeta.owners) == 0;
};
case (_) {};
};📝 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.
| await test("get file meta after upgrade", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert fileMeta.owners == []; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); | |
| await test("get file meta after upgrade", func() : async () { | |
| let res = await storage.getFileMeta(fileId); | |
| assert Result.isOk(res); | |
| switch (res) { | |
| case (#ok(fileMeta)) { | |
| assert fileMeta.path == "test/test.mo"; | |
| assert fileMeta.chunkCount == 1; | |
| assert Array.size(fileMeta.owners) == 0; | |
| }; | |
| case (_) {}; | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In test/storage-actor.test.mo around lines 84 to 95, the post-upgrade assertion
still expects fileMeta.owners == [] which is incorrect; update the test to
assert the owners match the expected owners used earlier in the test (e.g.,
replace the empty-array check with a comparison to the previously stored
expectedOwners/ownerId variable or the original owners list) so the ownership
info is verified consistently after upgrade.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/commands/bench-replica.ts (3)
7-7: Defer Mops flavor to runtime (type-only import).Avoid eagerly loading both PIC flavors. Keep types, load Mops impl only when needed.
-import {PocketIc as PocketIcMops, PocketIcServer as PocketIcServerMops} from 'pic-js-mops'; +import type {PocketIc as PocketIcMops, PocketIcServer as PocketIcServerMops} from 'pic-js-mops';
17-18: Add a discriminant to track which PIC API is active.This will let you avoid unsafe intersection casts later and branch cleanly.
pocketIcServer ?: PocketIcServer | PocketIcServerMops; pocketIc ?: PocketIc | PocketIcMops; + pocketIcMajor?: 4 | 9;
39-41: Harden version check (accept “9”, “9.0”, “v9.0.0”, etc.).String prefix “9.” misses common semver notations. Use a small regex and reuse the parsed version below.
- if (config.toolchain?.['pocket-ic'] !== '4.0.0' && !config.toolchain?.['pocket-ic']?.startsWith('9.')) { - console.error('Current Mops CLI only supports pocket-ic 9.x.x and 4.0.0'); + const picVersion = config.toolchain?.['pocket-ic'] ?? ''; + if (picVersion !== '4.0.0' && !/^v?9(\.|$)/.test(picVersion)) { + console.error('Current Mops CLI only supports pocket-ic 9.x and 4.0.0'); process.exit(1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/commands/bench-replica.ts(4 hunks)cli/commands/replica.ts(4 hunks)mops.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- mops.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/commands/replica.ts
⏰ 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: node 20, moc 0.14.13, ic-mops@1.0.0
🔇 Additional comments (1)
cli/commands/bench-replica.ts (1)
83-89: Request pic-js-mops v9.x type definitions
I can confirm pic-ic v4.0.0’s setupCanister(options: SetupCanisterOptions) ⇒ Promise<CanisterFixture> signature, but without the pic-js-mops v9.x source or its TypeScript declarations I can’t verify if its setupCanister signature/return differs. Please share the pic-js-mops v9.x repository link or paste the relevantsetupCanistertype definition (e.g. from itsindex.d.ts) so I can complete the API comparison.
| // pocket-ic 9.x.x | ||
| if (config.toolchain?.['pocket-ic']?.startsWith('9.')) { | ||
| this.pocketIcServer = await PocketIcServerMops.start({ | ||
| binPath: pocketIcBin, | ||
| }); | ||
| this.pocketIc = await PocketIcMops.create(this.pocketIcServer.getUrl()); | ||
| } | ||
| // pocket-ic 4.0.0 | ||
| else { | ||
| this.pocketIcServer = await PocketIcServer.start({ | ||
| binPath: pocketIcBin, | ||
| }); | ||
| this.pocketIc = await PocketIc.create(this.pocketIcServer.getUrl()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Close server on client init failure and load Mops flavor dynamically.
Prevents orphaned processes if create() throws and avoids hard dependency on pic-js-mops when using v4.
- // pocket-ic 9.x.x
- if (config.toolchain?.['pocket-ic']?.startsWith('9.')) {
- this.pocketIcServer = await PocketIcServerMops.start({
- binPath: pocketIcBin,
- });
- this.pocketIc = await PocketIcMops.create(this.pocketIcServer.getUrl());
- }
- // pocket-ic 4.0.0
- else {
- this.pocketIcServer = await PocketIcServer.start({
- binPath: pocketIcBin,
- });
- this.pocketIc = await PocketIc.create(this.pocketIcServer.getUrl());
- }
+ // pocket-ic 9.x
+ if (/^v?9(\.|$)/.test(picVersion)) {
+ const { PocketIc: PocketIcMops, PocketIcServer: PocketIcServerMops } = await import('pic-js-mops');
+ this.pocketIcServer = await PocketIcServerMops.start({ binPath: pocketIcBin });
+ this.pocketIcMajor = 9;
+ try {
+ this.pocketIc = await PocketIcMops.create(this.pocketIcServer.getUrl());
+ } catch (e) {
+ await this.pocketIcServer.stop().catch(() => {});
+ throw e;
+ }
+ }
+ // pocket-ic 4.0.0
+ else {
+ this.pocketIcServer = await PocketIcServer.start({ binPath: pocketIcBin });
+ this.pocketIcMajor = 4;
+ try {
+ this.pocketIc = await PocketIc.create(this.pocketIcServer.getUrl());
+ } catch (e) {
+ await this.pocketIcServer.stop().catch(() => {});
+ throw e;
+ }
+ }📝 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.
| // pocket-ic 9.x.x | |
| if (config.toolchain?.['pocket-ic']?.startsWith('9.')) { | |
| this.pocketIcServer = await PocketIcServerMops.start({ | |
| binPath: pocketIcBin, | |
| }); | |
| this.pocketIc = await PocketIcMops.create(this.pocketIcServer.getUrl()); | |
| } | |
| // pocket-ic 4.0.0 | |
| else { | |
| this.pocketIcServer = await PocketIcServer.start({ | |
| binPath: pocketIcBin, | |
| }); | |
| this.pocketIc = await PocketIc.create(this.pocketIcServer.getUrl()); | |
| } | |
| // pocket-ic 9.x | |
| if (/^v?9(\.|$)/.test(picVersion)) { | |
| // Dynamically load the Mops flavor only for v9 | |
| const { PocketIc: PocketIcMops, PocketIcServer: PocketIcServerMops } = await import('pic-js-mops'); | |
| this.pocketIcServer = await PocketIcServerMops.start({ binPath: pocketIcBin }); | |
| this.pocketIcMajor = 9; | |
| try { | |
| this.pocketIc = await PocketIcMops.create(this.pocketIcServer.getUrl()); | |
| } catch (e) { | |
| // Ensure we always stop the server if client init fails | |
| await this.pocketIcServer.stop().catch(() => {}); | |
| throw e; | |
| } | |
| } | |
| // pocket-ic 4.0.0 | |
| else { | |
| this.pocketIcServer = await PocketIcServer.start({ binPath: pocketIcBin }); | |
| this.pocketIcMajor = 4; | |
| try { | |
| this.pocketIc = await PocketIc.create(this.pocketIcServer.getUrl()); | |
| } catch (e) { | |
| // Ensure we always stop the server if client init fails | |
| await this.pocketIcServer.stop().catch(() => {}); | |
| throw e; | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/commands/bench-replica.ts around lines 43–56, starting the pocket-ic
server then calling the client factory can leave an orphaned server if the
client create() throws and the current code also hard-depends on the Mops
package; wrap the server start + client create in a try/catch so that if
PocketIcMops.create or PocketIc.create throws you call
this.pocketIcServer?.stop() (or the server shutdown API) before rethrowing, and
instead of importing Mops statically require/import the Mops module dynamically
only when the version check selects the 9.x path (e.g. dynamic
import('pic-js-mops') vs import('pic-js') fallback) so the code doesn’t
hard-fail for v4 users.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/storage-actor2.test.mo (3)
11-12: Right-size minted cycles and document intent.1e12 cycles is excessive for a unit test and can mask resource issues. Parameterize and lower to what install/upgrade actually needs (tune as required under pocket‑ic v9).
- ExperimentalCycles.add<system>(1_000_000_000_000); + // Mint only what is needed for canister install/upgrade in tests. + let CYCLES_FOR_TEST : Nat = 100_000_000_000; + ExperimentalCycles.add<system>(CYCLES_FOR_TEST);
16-16: Remove stray debug print.This noisy log adds no assertion value.
- Debug.print("lalalalalalala");
42-44: Finishing upload without any chunk: make the invariant explicit.With
chunkCount = 1, finishing without uploading chunk 0 may pass today but is fragile across implementations. Upload a zero-length chunk before finishing.- await test("finish upload", func() : async () { - assert Result.isOk(await storage.finishUploads([fileId])); - }); + await test("finish upload", func() : async () { + // Ensure required chunk exists before finishing. + ignore await storage.uploadChunk(fileId, 0, Blob.fromArray([])); + assert Result.isOk(await storage.finishUploads([fileId])); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/storage-actor2.test.mo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: node 20, moc 0.14.13, ./cli/dist
🔇 Additional comments (2)
test/storage-actor2.test.mo (2)
80-82: Upgrade pattern: confirm compatibility across pocket‑ic 4.x and 9.x.
await (system Storage.Storage)(#upgrade(storage))()is subtle and has changed in past toolchains. Please verify this works under both runtime branches targeted in this PR; otherwise gate by version or introduce a helper.
10-10: Do not add arun()wrapper. The Mops/mo:testreplica-mode runner automatically invokes your publicrunTests()entrypoint.Likely an incorrect or invalid review comment.
| import {test; suite; skip} "mo:test/async"; | ||
| import Result "mo:base/Result"; | ||
| import Blob "mo:base/Blob"; | ||
| import Debug "mo:base/Debug"; | ||
| import ExperimentalCycles "mo:base/ExperimentalCycles"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import Array for size/equality helpers.
import Result "mo:base/Result";
import Blob "mo:base/Blob";
import Debug "mo:base/Debug";
import ExperimentalCycles "mo:base/ExperimentalCycles";
+import Array "mo:base/Array";📝 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.
| import {test; suite; skip} "mo:test/async"; | |
| import Result "mo:base/Result"; | |
| import Blob "mo:base/Blob"; | |
| import Debug "mo:base/Debug"; | |
| import ExperimentalCycles "mo:base/ExperimentalCycles"; | |
| import {test; suite; skip} "mo:test/async"; | |
| import Result "mo:base/Result"; | |
| import Blob "mo:base/Blob"; | |
| import Debug "mo:base/Debug"; | |
| import ExperimentalCycles "mo:base/ExperimentalCycles"; | |
| import Array "mo:base/Array"; |
🤖 Prompt for AI Agents
In test/storage-actor2.test.mo around lines 1 to 6, the file is missing the
import for Array which provides size/equality helpers used later; add an import
statement for Array from the appropriate module (e.g., import Array
"mo:base/Array";) alongside the other imports so the test can use Array.size /
Array.equals helpers without runtime errors.
| assert fileMeta.path == "test/test.mo"; | ||
| assert fileMeta.chunkCount == 1; | ||
| assert fileMeta.owners == []; | ||
| }; | ||
| case (_) {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid == on arrays; use Array.size/Array.equal.
Array equality with == is unreliable/unsupported; assert emptiness or structural equality explicitly.
- assert fileMeta.owners == [];
+ assert Array.size(fileMeta.owners) == 0;Apply the same change to the post-upgrade check.
Also applies to: 89-93
🤖 Prompt for AI Agents
In test/storage-actor2.test.mo around lines 67 to 71 (and similarly update the
post-upgrade check at lines 89 to 93), replace the use of `==` to compare arrays
(e.g., `fileMeta.owners == []`) with an explicit emptiness or structural check
such as `Array.size(fileMeta.owners) == 0` or `Array.equal(fileMeta.owners,
[])`; update both occurrences so the test asserts emptiness or uses Array.equal
for structural equality instead of `==`.
1 participant
Summary by CodeRabbit
New Features
Refactor
Chores
Tests