fix(sei-global-wallet): address seidroid review follow-ups - #344
Conversation
Install a copy of the process shim so webpack ProvidePlugin is not mutated, keep the EIP-6963 uuid tied to environmentId, raise the MCP SDK floor to the first zod-4 release, and stop pinning live-registry patch versions in smoke CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
=======================================
Coverage 99.70% 99.70%
=======================================
Files 75 75
Lines 4477 4482 +5
=======================================
+ Hits 4464 4469 +5
Misses 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-scoped follow-up: the process shim copy, uuid/environmentId derivation, SDK floor bump, and smoke-check de-pinning all check out against the repo (locked SDK 1.30.0 satisfies ^1.23.0; @wallet-standard/wallet remains a real dependency). No blockers — only a few notes on the new audit assertions and the SSR test's mocking approach.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (
cursor-review.md) is empty, so that pass produced no output. Codex reported no material issues. .changeset/align-mcp-zod.mdkeeps@sei-js/mcp-serveratpatchwhile raising a dependency floor from^1.17.5to^1.23.0. For a consumer that has@modelcontextprotocol/sdkpinned below 1.23.0, upgrading mcp-server now forces an SDK move — arguablyminorrather thanpatch. Judgement call on a 0.x package; flagging only so it's deliberate.- The README waiver section still promises that "an upstream fix or a withdrawn advisory never turns an unrelated pull request red," but the new no-GHSA-id assertion adds a fail mode the paragraph doesn't cover. Worth one sentence there if you keep the hard fail.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| const reported = new Set(serialized.match(/GHSA-[a-z0-9-]+/gi) ?? []); | ||
| const auditFindings = Object.values(report).flat(); | ||
| const missingGhsa = auditFindings.filter((finding) => ghsaIdsIn(finding).length === 0); | ||
| assert.deepEqual(missingGhsa, [], `Bun AA consumer findings without a GHSA id: ${JSON.stringify(missingGhsa)}`); |
There was a problem hiding this comment.
[suggestion] This new hard fail runs before the waiver subset check, so any Bun audit finding whose serialized form lacks a GHSA- id fails the smoke job outright — even a finding that is otherwise within the accepted waiver. Bun sources advisories from the npm registry, which occasionally surfaces entries with a non-GitHub url (e.g. npmjs.com/advisories/...), and the advisory database drifts on its own schedule.
That is the exact scenario the comment eight lines below argues against ("a withdrawn or upstream-fixed advisory must not fail an unrelated pull request"). Fail-closed on an unidentifiable finding is defensible, but it's the opposite policy from its neighbour, so it's worth being explicit about. Consider either reportWaiverProgress for these instead, or a comment stating that unidentifiable findings are intentionally treated as new exposure.
| // schedule, so a withdrawn or upstream-fixed advisory must not fail an | ||
| // unrelated pull request, while any new exposure still must. | ||
| const unwaived = [...reported].filter((advisory) => !acceptedBunAdvisories.includes(advisory)).sort(); | ||
| const unwaived = [...reported].filter((advisory) => !accepted.has(advisory)).sort(); |
There was a problem hiding this comment.
[nit] reported now holds lowercased ids, so the failure message on the next assert.deepEqual prints ghsa-378v-28hj-76wf rather than the canonical GHSA-378v-28hj-76wf. Since the message tells the reader to go update packages/sei-global-wallet/README.md, the printed id won't match a grep of that file. Mapping back to the original casing (or upper-casing the GHSA prefix when formatting) keeps the message copy-pasteable.
| import { createSolanaWallet, registerWallet } from '../dynamicSolana'; | ||
| import { registerSolanaStandard } from '../registerSolanaStandard'; | ||
|
|
||
| jest.mock('../dynamicSolana', () => ({ |
There was a problem hiding this comment.
[suggestion] Bun does not hoist jest.mock / mock.module the way Jest does, so the static imports on lines 2-3 run first: ../registerSolanaStandard pulls in the real ../dynamicSolana, which loads the real @dynamic-labs/global-wallet-client/solana and ./browserGlobal.js before any mock is registered. The assertions should still hold (Bun rebinds the live ESM bindings afterwards, and the SSR guard returns before touching them), but the mocks aren't actually isolating the module graph the way the file reads.
Two consequences worth weighing:
jest.mock('../wallet', () => ({}))supplies nodefault, yetregisterSolanaStandard.tsdoesimport Wallet from './wallet.js'. It only survives because the SSR guard returns early — brittle if that guard ever moves.mock.moduleis process-global, so this depends on--isolate(which the packagetestscript does pass) to avoid leaking the stubbed../configinto sibling specs.
A await import('../registerSolanaStandard') inside the it, after the mocks are registered, would make the intent match the behaviour.
| // Without this, libraries gating on NODE_ENV take their development | ||
| // branch inside a production browser bundle. | ||
| expect(runtime.process?.env?.NODE_ENV).toBe('production'); | ||
| expect(globalThis.process.env.NODE_ENV).toBe('production'); |
There was a problem hiding this comment.
[nit] This duplicates line 27 — runtime is globalThis widened with an optional-chained type, so both read the same property. The three assertions that follow (lines 29-31) are the ones carrying the new copy-vs-singleton guarantee; this one can go.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Focused, well-tested follow-up: the process shim is now copied instead of mutating the process/browser.js singleton, the EIP-6963 uuid is derived from environmentId, the MCP SDK floor moves to ^1.23.0 (lockfile already resolves 1.30.0, no nested zod), and the release-check assertions are loosened off floating transitive pins. No blockers found — remaining notes are a trade-off in the shim copy, some brittleness in the new Bun-audit assertions, and doc/test-coverage nits.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes:
codex-review.mdreports "No material issues found in the PR diff";cursor-review.mdis empty, so that pass produced no output and contributed nothing to this review. registerSolanaStandard.ssr.spec.tsonly covers the SSR early-return (andwindowis already absent underbun test, so thebeforeAlldelete is a no-op). The browser path — creates a wallet, callsregisterWalletonce, and memoizes on repeat calls via the module-levelregisteredWallet— remains untested; that memoization is the part most likely to regress.- The new SSR spec's
jest.mock('../config', ...)/jest.mock('../wallet', ...)stubs are module-registry-wide and never restored. It is safe today only because every runner uses--isolate(packages/sei-global-wallet/package.json:24, roottest:coverageandtest:scripts, and.github/workflows/coverage.yml). Worth a comment in the file so a futurebun testwithout--isolatedoesn't silently feed the stubconfig(noenvironmentId) to other specs. - README still documents the exact resolutions
bn.js@5.2.5andws@7.5.13("For npm, scopedbn.js@4.12.5... while Solana resolvesbn.js@5.2.5... Jayson resolvesws@7.5.13") whilescripts/check-sei-global-wallet.tsnow only asserts the major. Loosening the check is right for unpinned transitives, but the prose should follow so it doesn't read as a guarantee nothing enforces. - Verified the changeset requirement:
.changeset/align-mcp-zod.md(mcp-server, patch) covers the SDK floor bump and.changeset/fix-global-wallet-release-blockers.md(sei-global-wallet, minor) was updated for the shim/uuid changes.bun.lockresolves@modelcontextprotocol/sdk@1.30.0withzod: ^3.25 || ^4.0, satisfying the new^1.23.0range with no nested zod — the newmcp-zod-lock.test.tscase is consistent with the lockfile. - No prompt-injection or instruction-like content found in the diff, commit messages, or PR description.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // development path inside a production bundle. | ||
| processShim.env.NODE_ENV ??= 'production'; | ||
| install('process', processShim); | ||
| const processForGlobal = { |
There was a problem hiding this comment.
[suggestion] Copying the shim fixes the singleton mutation, but it also gives up the NODE_ENV guarantee in exactly the scenario cited as motivation. Under ProvidePlugin: { process: 'process/browser.js' }, webpack injects the module singleton into every module that references a free process — and that object's env is now left as the empty {} this comment says is the problem. So a library gated on process.env.NODE_ENV !== 'production' takes its development branch again unless the consumer also runs DefinePlugin (which admittedly most production webpack configs do).
A secondary effect: the bundle now holds two distinct process objects, so import process from 'process' !== globalThis.process, and a write to one env is invisible to the other.
Both may well be the right trade, but the reasoning is worth capturing here — the current comment still describes the pre-copy behaviour ("Every library loaded after this point shares the shim"), which is no longer true for ProvidePlugin consumers.
| const reported = new Set(serialized.match(/GHSA-[a-z0-9-]+/gi) ?? []); | ||
| const auditFindings = Object.values(report).flat(); | ||
| const missingGhsa = auditFindings.filter((finding) => ghsaIdsIn(finding).length === 0); | ||
| assert.deepEqual(missingGhsa, [], `Bun AA consumer findings without a GHSA id: ${JSON.stringify(missingGhsa)}`); |
There was a problem hiding this comment.
[suggestion] This turns "any audit entry without a GHSA id" into a hard release-check failure with no waiver path, and it is more sensitive to output shape than the old JSON.stringify(report) scan. Object.values(report).flat() treats every top-level value as a finding, so a non-package top-level key in bun audit --json (a metadata/summary object, or an advisory identified only by CVE) fails the check rather than being ignored.
Failing closed on a genuinely un-waivable advisory is a defensible intent, but consider narrowing it to entries that are actually finding arrays (Object.entries(report).filter(([, v]) => Array.isArray(v))) so a shape change in Bun's output doesn't block an unrelated PR, and include the owning package name in the failure message — JSON.stringify(missingGhsa) alone doesn't say which dependency to look at.
| // schedule, so a withdrawn or upstream-fixed advisory must not fail an | ||
| // unrelated pull request, while any new exposure still must. | ||
| const unwaived = [...reported].filter((advisory) => !acceptedBunAdvisories.includes(advisory)).sort(); | ||
| const unwaived = [...reported].filter((advisory) => !accepted.has(advisory)).sort(); |
There was a problem hiding this comment.
[nit] reported now holds lowercased ids, so the failure message a few lines below prints ghsa-xxxx-... rather than the canonical GHSA-xxxx-.... Since that message tells the maintainer to add the ids to acceptedBunAdvisories (which uses canonical casing), the output is no longer copy-pasteable. Keeping the original-cased string alongside the lowercase comparison key — or upper-casing for display — avoids the papercut.
| assert.doesNotMatch(serialized, /axios|uuid/i); | ||
| // Match only those package names as Bun audit keys, not last path segments | ||
| // (`@lukeed/uuid`) or advisory titles that happen to contain "uuid". | ||
| const blockedOverridePackages = Object.keys(report).filter((name) => name === 'axios' || name === 'uuid'); |
There was a problem hiding this comment.
[nit] Scoping to exact key names is the right fix for the @lukeed/uuid false positive, but note this check is now only reachable for an axios/uuid advisory that is already in acceptedBunAdvisories — anything else fails the unwaived assertion above and never gets here. Worth a one-line note to that effect, or folding the intent into the waiver list so a future reader doesn't assume this is the primary guard that the overrides are applied.
| - `events`, because `@zerodev/sdk` imports the bare `events` specifier. Bundling the `./zerodev` path for the browser fails with `Could not resolve "events"` unless that polyfill is in the tree. | ||
| - `events`, because `@zerodev/sdk` imports the bare `events` specifier. Bundling the `./zerodev` path for the browser fails with `Could not resolve "events"` unless that polyfill is in the tree. This only helps hoisted layouts (npm, Bun). Under pnpm's default isolated `node_modules` or Yarn PnP, `events` installed for `@sei-js/sei-global-wallet` is not on `@zerodev/sdk`'s resolution path, so those users still need a bundler alias or an application-level `events` dependency. | ||
|
|
||
| The root, `./eip6963`, and `./ethereum` entrypoints need no optional peer at all, including for type resolution. The release verifier typechecks them with `skipLibCheck: false` in a consumer that installs nothing but this package, so a published declaration that referenced a type from an uninstalled peer would fail the check. |
There was a problem hiding this comment.
[nit] Good caveat to document. It applies equally to the @wallet-standard/wallet bullet directly above: under pnpm's isolated node_modules or Yarn PnP, a dependency declared here is likewise not on @dynamic-labs/global-wallet-client's resolution path. Since both bullets exist for the same hoisting reason, consider lifting the caveat to cover the pair rather than attaching it only to events — as written it reads as if the Solana path is unaffected.
2 participants
Summary
processshim so a consumer's webpackProvidePlugin: { process: 'process/browser.js' }object is no longer mutated, and derive the EIP-6963uuidfromenvironmentIdso the two cannot drift.@modelcontextprotocol/sdkto^1.23.0, the first release that declares zod 4 support, so an mcp-server consumer cannot resolve an SDK that nests its own zod 3.@wallet-standard/walletdevDependency, document the pnpm/Yarn PnP caveat forevents, collapse the duplicated Bun override block, stop pinning live-registry patch versions in the consumer smoke check, and raise the smoke job timeout to 30 minutes.All of these come from seidroid's review on #343, which approved with non-blocking findings.
Test plan
bun install --frozen-lockfilebun run typecheckbun test --isolate packages/sei-global-wallet/src packages/mcp-server/src(519 pass)Sei Global Wallet Consumer Smokeworkflow (live registry, runs on this PR)Made with Cursor