fix(frontend)!: declare @icp-sdk/core as a peer dependency and stop bundling runtime deps by marc0olo · Pull Request #434 · dfinity/vetkeys

marc0olo · 2026-08-26T14:30:20Z

Closes #433.

Declares @icp-sdk/core as a peerDependency and stops bundling runtime dependencies into dist/. Released as 0.7.0 — breaking: consumers must now install @icp-sdk/core themselves.

Why both halves are needed

#433 asks for the peer change, which is correct — core is a shared singleton whose classes cross our public API (callers construct the HttpAgent they pass in), and as a plain dependency npm silently installed a second copy.

But the peer alone would have been cosmetic. vite.config.ts set no rollupOptions.external, and Vite lib mode externalises nothing by default, so the tarball already inlined a private 289 kB copy of core's agent — in every install, invisible to npm. Moving core to a peer and rebuilding produces a byte-identical bundle. So this PR does both.

Changes

@icp-sdk/core → peerDependencies (^5.0.0 || ^6.0.0), kept in devDependencies for build/test.
Externalise runtime deps in the Vite build. dist/lib: ~430 kB → ~56 kB.
@noble/curves / @noble/hashes → dependencies. They are imported by shipped code, so declaring them as dev dependencies was wrong — it only worked because the build inlined them.
New CI job pinning core to the peer-range floor and running the full suite against it.

idb-keyval stays a regular dependency: no idb-keyval value crosses the public API, and IndexedDB is keyed by (origin, dbName, storeName), so two copies address the same store.

Peer range spans both majors

We use only Actor.createActor and the candid IDL, unchanged across v5 and v6 — which is why the v5→v6 bump in #432 needed zero source changes. Typechecked against core 5.0.0, 5.2.1, 5.4.0, 6.0.0 and 6.1.0: all clean.

This matters: a peer of ^6.1.0 next to auth@^5 is a hard ERESOLVE, which would turn "silently broken" into "cannot install" until the rest of the SDK moves to core 6. The dual range fixes the hazard without that. It should also let icskills drop its @icp-sdk/vetkeys@^0.5 pin — worth confirming against dfinity/icskills#368 before closing.

Verification

All CI green, including the new frontend_ic_vetkeys_core_floor job — full canister suite (4 files, 45 tests) against a live replica on core 5.0.0, so both ends of the peer range are tested.

Note: #433's stated mechanism is overstated

Recording this so we do not carry an inaccurate rationale forward. #433 claims instanceof checks fail and Principals get mis-encoded across copies. Neither holds: core has no instanceof HttpAgent/instanceof Agent anywhere, candid's PrincipalClass.covariant tests x._isPrincipal, and Principal.isPrincipal has an explicit structural fallback.

The genuine cross-copy defects are narrower — actor.js does e instanceof RejectError on errors thrown by the consumer's agent, so error enrichment is silently skipped (degraded diagnostics, not corruption); v6's Actor passes effectiveTarget: to a v5 agent that only reads effectiveCanisterId, silently ignored (benign here, but fragile); and Principals returned by vetKeys were built by the bundled core, so a consumer's instanceof Principal failed.

So the case rests on packaging correctness and ~290 kB of dead weight, not an acute bug biting users today.

Release

After merge: git tag npm/0.7.0 && git push origin npm/0.7.0, then the docs workflow.

🤖 Generated with Claude Code

…time deps Closes #433. `@icp-sdk/core` is a shared-singleton library whose classes cross this package's public API — `DefaultKeyManagerClient` and `DefaultEncryptedMapsClient` take a caller-constructed `HttpAgent`. Declared as a plain dependency, npm was free to install a second copy of core nested under `@icp-sdk/vetkeys` with no warning, putting two `HttpAgent` / `Principal` identities in one application. It is now a peerDependency, so that conflict surfaces at install time, matching `@icp-sdk/auth`, `@icp-sdk/signer` and `@icp-sdk/canisters`. The peer alone would not have fixed anything. `vite.config.ts` set no `rollupOptions.external`, and Vite lib mode externalises nothing by default, so the published tarball already inlined a private 289 kB copy of core's agent — present in every install regardless of what npm resolved, and invisible to it. Moving core to a peer without externalising produces a byte-identical bundle. Both halves are needed, so all runtime dependencies are now external. The peer range spans both majors (`^5.0.0 || ^6.0.0`): this package uses only `Actor.createActor` and the candid `IDL`, unchanged across core v5 and v6, so it can sit in an application still on core v5 rather than forcing the whole SDK to move in lockstep. A new CI job runs the full canister suite against the floor of that range, so both ends stay tested. Also moves `@noble/curves` and `@noble/hashes` from devDependencies to dependencies: they are imported by shipped code, which only worked because the build inlined them. `idb-keyval` stays a regular dependency — no idb-keyval value crosses the public API, and IndexedDB is keyed by origin, database and store name. dist/lib shrinks from ~430 kB to ~56 kB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Updates the frontend/ic_vetkeys package packaging/build so @icp-sdk/core is no longer bundled into the published library output and is instead enforced as a peer dependency, preventing multiple-core copies in consumer dependency trees.

Changes:

Move @icp-sdk/core from dependencies to peerDependencies (kept in devDependencies for build/test) and bump package version to 0.7.0.
Externalize runtime deps in Vite lib build to stop bundling @icp-sdk/core, @noble/*, and idb-keyval into dist/.
Add a CI job that pins @icp-sdk/core to the peer-range floor (5.0.0) and runs the vetkeys suite against it.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pnpm-lock.yaml	Reflects dependency moves (core to dev; noble to runtime deps) in the workspace lockfile.
frontend/ic_vetkeys/vite.config.ts	Adds Rollup externals to prevent bundling runtime dependencies into the library build output.
frontend/ic_vetkeys/package.json	Declares core as a peer dependency, moves `@noble/*` into dependencies, and bumps version to 0.7.0.
frontend/ic_vetkeys/CHANGELOG.md	Documents breaking packaging changes for the 0.7.0 release.
.github/workflows/frontend.yml	Adds a new CI job to test against the minimum supported `@icp-sdk/core` peer version.

Files not reviewed (1)

pnpm-lock.yaml: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Addresses review feedback on #434. The floor job skipped `pnpm run lint`, which made its own comment ("runs the same suite against it") inaccurate. It is also not redundant with the other jobs: linting here is type-aware (`recommendedTypeChecked` with `parserOptions.project`), so `no-unsafe-*` and friends read `@icp-sdk/core`'s typings and can legitimately differ between core versions in ways `tsc` alone does not catch. Verified clean against core 5.0.0 locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

marc0olo · 2026-08-26T14:42:50Z

Re: the Copilot review comment on .github/workflows/frontend.yml — good catch on both counts, fixed in 7c80bc9.

I had dropped pnpm run lint from the floor job deliberately, thinking it was redundant with the other two jobs since it lints identical source. That reasoning was wrong. Linting here is type-aware (recommendedTypeChecked with parserOptions.project), so rules like no-unsafe-assignment / no-unsafe-call / no-unsafe-member-access read @icp-sdk/core's typings. If an older core returned any where v6 returns a concrete type, those rules would fire on the floor and not on the ceiling — and tsc alone would not catch it, since it does not flag any propagation.

So lint was in fact the one part of the suite where the core version could change the result, which makes it the most valuable thing to run there, not the least.

Verified clean against core 5.0.0 locally before pushing. The job now runs build + test + lint, which also makes its own "runs the same suite against it" comment accurate.

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Files not reviewed (1)

pnpm-lock.yaml: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/frontend.yml:95

The core-version assertion hard-codes a ./node_modules/... path, which is brittle with pnpm’s symlinked layout and makes the check depend on filesystem structure rather than Node’s resolver. Prefer resolving the package via require() so the check keeps working if the node_modules layout changes.

          node -p "require('./node_modules/@icp-sdk/core/package.json').version" | tee /dev/stderr | grep -q '^5\.0\.0$'

The entry had grown into an explanation of why the packaging was wrong, which belongs in #433 and #434 rather than in release notes. Keeps the breaking changes, the supported peer range, the install command and the bundle-size delta; drops the singleton rationale, the range justification and the note about `idb-keyval` deliberately not changing. 46 lines -> 21. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

raymondk · 2026-08-26T17:34:18Z

          pnpm run test
          pnpm run lint
  frontend_ic_vetkeys_mac:
    runs-on: macos-15


Why is this useful to run on macs?
Perhaps we can avoid the extra cost and stick with the linux tests

Two github-actions check suites were created but never dispatched during the GitHub incident, leaving required checks unreported and the PR blocked. Re-requesting the suites returned 404 (they never started), so this empty commit fires a fresh `synchronize` event. Absorbed by the squash merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Clears **all 12 open Dependabot advisories** in one change. Supersedes #397, #380 and #409. ## Changes | Package | Was | Now | Alerts cleared | |---|---|---|---| | `openssl` | 0.10.73 | **0.10.81** | 8 — 5 high, 2 medium, 1 low | | `openssl-sys` | 0.9.109 | 0.9.117 | (transitive) | | `rustls-webpki` | 0.103.10 | **0.103.15** | 3 — 1 high, 2 low | | `rand` | 0.8.5 | **0.8.8** | 1 — low | Each alert was checked against its own patched version; all 12 verify as cleared. `openssl` and `rustls-webpki` land *newer* than the superseded PRs proposed (0.10.80 / 0.103.13). Three files, and the lockfile moves exactly four package versions — no transitive churn. ## Why not `rand` 0.10.2, as #409 proposed `rand` is the only one of the three that reaches shipped code — it is a **runtime** dependency of the published `ic-vetkeys` crate (plus `ic-vetkeys-test-utils`, `ic-vetkeys-manager-canister`, `ic-vetkeys-canisters-tests`). But the advisory ([GHSA-cq8v-f236-94qc](GHSA-cq8v-f236-94qc), **low**) is: ``` vulnerable range: >= 0.7.0, < 0.8.6 first patched: 0.8.6 ``` 0.8.6 is a **patch** release. #409 proposed `0.10.2` — two breaking majors past the fix — which would force a `rand` 0.8 → 0.10 API migration across four crates including the published crypto crate, plus a lockstep `rand_chacha` 0.3 → 0.10 move. That is a large, risky change to absorb for a low-severity advisory that a patch bump resolves. This PR moves the manifest floor to `0.8.6` and the lock resolves `0.8.8`. (The `rand 0.10.2` already in `Cargo.lock` is unrelated — it arrives via `quinn-proto`, transitively under the dev-only `reqwest`, and is untouched here.) ## Severity in context `openssl` and `rustls-webpki` account for 11 of the 12 alerts, including all 6 highs — but their real exposure is **CI-only**. Both reach the tree solely via `reqwest`, which sits under `[dev-dependencies]` next to `pocket-ic`, and the canisters compile to `wasm32-unknown-unknown` where neither can exist. Neither appears in the published crate's dependency list: ``` rand req=^0.8.5 kind=normal rand_chacha req=^0.3.1 kind=normal pocket-ic req=^15.0.0 kind=dev ``` GitHub reports `scope=runtime` for all of them, but that is inferred from `Cargo.lock`, which carries no dev/runtime split for transitive packages. The manifests are the authority here. ## No release required Published `ic-vetkeys@0.9.0` declares `rand = "^0.8.5"` — i.e. `>=0.8.5, <0.9.0`. **`0.8.8` already satisfies that**, so anyone building against 0.9.0 today resolves the fixed version automatically; nobody is pinned to the vulnerable 0.8.5 except via a stale local lockfile, which `cargo update -p rand` fixes without any action from us. `Cargo.lock` is not consumed by dependents of a library crate, and dev-dependencies never propagate — so the `openssl` / `rustls-webpki` half has zero consumer impact by construction. The manifest floor bump to `0.8.6` only takes effect when we next publish, and is belt-and-braces for the stale-lockfile case. **It can ride the next release rather than triggering one.** Nothing here affects `@icp-sdk/vetkeys` (npm) or the Motoko package. ## Verification Ran the backend CI commands verbatim: - `cargo build --release --target wasm32-unknown-unknown` for all four canister crates — **pass** - `cargo test` — **14 passed, 0 failed**, including the pocket-ic integration tests (`key_sharing_should_work`, `should_preserve_state_across_upgrade`, `should_get_accessible_shared_key_ids`, …) that exercise the crypto paths using `rand` - `cargo test --doc` — pass (the 4 `ignored` doc-tests carry pre-existing `ignore` annotations, unchanged here) - `cargo clippy -- -Dwarnings` — pass - `cargo fmt --check` — pass `rand` 0.8.5 → 0.8.8 is a patch bump inside 0.8, so no API change was expected; the green build and integration tests confirm it rather than assume it. ## Superseded - #397 — `openssl` → 0.10.80. Included here at 0.10.81. - #380 — `rustls-webpki` → 0.103.13. Included here at 0.103.15. - #409 — `rand` → 0.10.2. Replaced with the 0.8.x patch fix, per above. Also closed as obsolete while triaging: #337, #316, #315 (targeted `examples/`, removed in #377) and #406 (`vite` 7.3.5, already superseded by `^7.3.6` from #434). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Follow-up to the discussion on #434. Removes the three macOS CI jobs and the now-unused `provision-darwin.sh`. ## They have never caught anything Linux didn't Every completed run of the three workflows (140 runs, 2026-07-27 → 08-26): | Workflow | Runs | pass/pass | **fail/fail** | cancelled | **mac failed, linux passed** | |---|---|---|---|---|---| | `frontend.yml` | 60 | 51 | **3** | 4 | **0** | | `backend-rust.yml` | 40 | 35 | **2** | 3 | **0** | | `backend-motoko.yml` | 40 | 38 | **2** | 0 | **0** | All 7 real failures failed on **both** platforms. The two apparent frontend divergences were `success/cancelled` and `cancelled/success` — artifacts of `cancel-in-progress: true`, not signal. ## Coverage is preserved Each macOS job ran a command block **byte-identical** to its Linux counterpart. Verified by hashing the `run:` bodies of the non-provisioning steps before deleting: ``` frontend.yml linux 2518a70671d5 == mac 2518a70671d5 backend-rust.yml linux ff569b94f041 == mac ff569b94f041 backend-motoko.yml linux fa2141c56e44 == mac fa2141c56e44 ``` The only differences between each pair were `runs-on`, the provision script, and the step name. The surviving Linux jobs execute exactly what the deleted jobs did — nothing to port over. There is also little platform-specific surface to lose. The frontend package is portable TypeScript (`@noble/*` is pure JS, hashing goes through WebCrypto, IndexedDB is stubbed with `fake-indexeddb`); the Rust crates build to `wasm32-unknown-unknown`. The one genuinely platform-specific component under test was the `pocket-ic` binary, which exercises icp-cli's portability rather than this repo's. ## Cost macOS runners bill at **10×**. Measured over the last 5 runs of each: ``` frontend_ic_vetkeys_mac 2.3 min -> 23 billed min cargo-test-backend-darwin 3.3 min -> 33 billed min backend-motoko-tests-darwin 3.4 min -> 34 billed min ------- ------------ 9.0 min ~90 billed min per full CI pass ``` ## Also cleaned up `clippy.yml` and `backend-rust-canbench.yml` listed `provision-darwin.sh` in their `paths:` filters without ever running a macOS job. Those entries are removed along with the script. ## Origin, for the record The macOS jobs were never a considered decision. They trace to `803d2ca` (Feb 2025), the first frontend CI commit, which had `ubuntu-24.04` and `macos-15` side by side from line one, and reached `frontend.yml` via #61 — a "merge crates and packages" refactor whose PR body is `TODO description`. ## If we want some macOS coverage back This is 98 lines of pure deletion, so `git revert` restores it exactly. A middle option, if the concern is that a month is a short window for a rare platform regression: re-add the jobs on `push` to `main` plus a nightly `schedule` instead of on `pull_request`. That keeps a signal for toolchain drift (`provision-darwin.sh`, Homebrew, the icp-cli/pocket-ic darwin binaries) at a fraction of the cost, since most runs are PR runs. Happy to do that instead if preferred. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

marc0olo requested review from a team as code owners August 26, 2026 14:30

marc0olo mentioned this pull request Aug 26, 2026

@icp-sdk/vetkeys: declare @icp-sdk/core as a peerDependency, not a dependency #433

Closed

marc0olo requested a lite review from Copilot August 26, 2026 14:34

Copilot started reviewing on behalf of marc0olo August 26, 2026 14:34 View session

Copilot AI reviewed Aug 26, 2026

View reviewed changes

Comment thread .github/workflows/frontend.yml

marc0olo requested a lite review from Copilot August 26, 2026 14:46

Copilot started reviewing on behalf of marc0olo August 26, 2026 14:47 View session

Copilot AI reviewed Aug 26, 2026

View reviewed changes

raymondk approved these changes Aug 26, 2026

View reviewed changes

raymondk reviewed Aug 26, 2026

View reviewed changes

marc0olo merged commit 6cdb120 into main Aug 26, 2026
10 checks passed

marc0olo deleted the fix/core-peer-dependency branch August 26, 2026 19:04

This was referenced Aug 26, 2026

ci: drop the macOS jobs #435

Merged

chore(deps-dev): bump vite from 7.3.2 to 7.3.5 #406

Closed

fix(deps): clear all open Dependabot advisories #436

Merged

Navigation Menu

Uh oh!

fix(frontend)!: declare @icp-sdk/core as a peer dependency and stop bundling runtime deps - #434

fix(frontend)!: declare @icp-sdk/core as a peer dependency and stop bundling runtime deps#434
marc0olo merged 4 commits into
mainfrom
fix/core-peer-dependency

marc0olo commented Aug 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

marc0olo commented Aug 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

raymondk Aug 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

marc0olo commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why both halves are needed

Changes

Peer range spans both majors

Verification

Release

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

marc0olo commented Aug 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

raymondk Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marc0olo commented Aug 26, 2026 •

edited

Loading