fix(sei-global-wallet): harden consumer integrations - #343
Conversation
Align Dynamic peers, make browser and EIP-6963 behavior reliable, and verify the explicit upstream security waiver in real npm and Bun consumers. 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 #343 +/- ##
==========================================
+ Coverage 99.69% 99.70% +0.01%
==========================================
Files 64 75 +11
Lines 4293 4477 +184
==========================================
+ Hits 4280 4464 +184
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:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR does a lot of genuinely good work — the local EIP-6963 announce/re-announce implementation, SSR guards, idempotent Solana registration, and the new consumer smoke harness are all solid improvements. But it also converts several widely-used optional peer dependencies (viem, @solana/web3.js, @zerodev/sdk, @dynamic-labs/ethereum-aa) from ranges to exact pins and hard-pins Dynamic to a release with known-vulnerable transitives, both of which are breaking for downstream installs and are released as a patch.
Findings: 4 blocking | 14 non-blocking | 10 posted inline
Blockers
- Semver: tightening optional peer ranges to exact versions is a breaking change for consumers, but the changeset requests a
patch. Per REVIEW_GUIDELINES §3 a published-package dependency change needs a changeset (present ✅), but the bump level should reflect that existing installs will now fail to resolve — this should be a major (or at minimum a minor with an explicit migration note). - Cursor's second-opinion pass (
cursor-review.md) is empty — that review produced no output, so only Codex's single finding was available to merge with mine. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
browserGlobal.tshas no unit test. The changeset and README both claim the shim installs "only missing values and preserves anything the consumer already set," but nothing insrc/lib/__tests__/exercises that.check-edge-native.mjsonly covers the absent case (it deletes both globals first); add a spec that pre-setsglobalThis.global/globalThis.processto sentinels and asserts they survive the import.- The generated consumer typecheck (
scripts/check-sei-global-wallet.ts:309-320) setsskipLibCheck: true, which disables checking of all.d.tsfiles. Since@wallet-standard/basemoved from a hard dependency to an optional peer and is referenced by emitted declarations, this is exactly the failure mode the typecheck would otherwise catch. Consider a second pass withskipLibCheck: false. - The new
sei-global-wallet-smoke.ymlonly triggers onpull_requestandworkflow_dispatch. Since its stated purpose is gating a release, it will never run onpushtomainor on the auto-generated "Version Packages" PR merge — the very commit that publishes. Addingpush: branches: [main]would close that gap. "sideEffects": truein package.json is a no-op: an absentsideEffectsfield already means "assume side effects everywhere." If the intent is to document which modules have side effects while keeping the rest tree-shakeable, use an array (e.g.["./dist/eip6963.js", "./dist/solana.js", "./dist/lib/browserGlobal.js"]).@wallet-standard/walletis no longer imported anywhere insrc/(only@wallet-standard/base, and only as a type). Keeping it inpeerDependencies/peerDependenciesMetais now vestigial and can be dropped.- No prompt-injection or instruction-like content was found in the diff, commit messages, or PR description.
- 8 suggestion(s)/nit(s) flagged inline on specific lines.
| "@zerodev/sdk": "5.4.36", | ||
| "viem": "^2.7.12" | ||
| "@zerodev/sdk": "5.5.7", | ||
| "viem": "2.45.3", |
There was a problem hiding this comment.
[blocker] Exact-pinning viem as a peer dependency will break installs for most consumers.
This goes from ^2.7.12 to exactly 2.45.3. peerDependenciesMeta.optional only means npm won't auto-install it and won't error when it's absent — if the consumer already has viem (which nearly every wagmi/RainbowKit/ConnectKit dApp does) at any version other than 2.45.3, npm 7+ fails the install with ERESOLVE could not resolve / conflicting peer dependency. Since 2.45.3 is older than what most apps are on, this is a forced downgrade, not a floor.
The same applies to the other newly-exact peers in this block: @dynamic-labs/ethereum-aa: 4.96.3 (L43), @solana/web3.js: 1.98.1 (L45), @zerodev/sdk: 5.5.7 (L49), and zksync-sso: 0.2.0 (L51).
The stated reason for the viem pin (newer viem/Ox pulling node:worker_threads into Vite resolution) is a consumer bundler-resolution problem; a library peer range shouldn't be the mechanism that dictates it. A bounded range that excludes the known-bad versions — e.g. >=2.7.12 <2.55.0, or ^2.45.3 if the issue is actually fixed forward — keeps the constraint expressible without making every non-matching consumer install fail. Applying the same treatment to the other four would keep this a non-breaking release.
| "dependencies": { | ||
| "@dynamic-labs/global-wallet-client": "^4.60.1", | ||
| "@wallet-standard/wallet": "^1.1.0" | ||
| "@dynamic-labs/global-wallet-client": "4.96.3", |
There was a problem hiding this comment.
[blocker] Exact-pinning Dynamic prevents consumers from ever receiving the upstream fix. (This is Codex's finding, with one addition.)
Codex is right that 4.96.3 knowingly ships vulnerable axios@1.16.0 and uuid@11.1.0, and that a library's overrides do not propagate — so every downstream npm audit fails unless the app author reads the README and copies the override block by hand.
The part worth adding: the previous ^4.60.1 at least let consumers pick up 4.96.4+ the moment Dynamic corrects those transitive pins. The exact 4.96.3 removes that escape hatch entirely — unblocking consumers then requires a new @sei-js/sei-global-wallet release and a coordinated upgrade across every downstream dApp.
Given the whole waiver is explicitly described as temporary, a caret or a tilde (~4.96.3) would preserve the reproducibility this PR is after while letting the fix flow through automatically. If exactness is truly required for the metafile/lockfile assertions in scripts/check-sei-global-wallet.ts, that's an argument for pinning in the repo lockfile, not in the published manifest.
| }); | ||
| } | ||
|
|
||
| if (typeof runtime.process === 'undefined') { |
There was a problem hiding this comment.
[suggestion] Installing a global process shim has app-wide blast radius, not just Dynamic's.
This module is loaded as an import side effect of every entrypoint, including the one-liner import '@sei-js/sei-global-wallet/eip6963' the README tells apps to put at startup. In a real browser or edge runtime process is genuinely undefined, so this installs process/browser.js globally — which every subsequently-loaded library then sees.
The concrete hazard: process/browser.js sets process.env = {}, so process.env.NODE_ENV is undefined. Libraries that gate on typeof process !== 'undefined' && process.env.NODE_ENV !== 'production' will take their development branch (extra warnings, unoptimized paths) in a production bundle. The global = globalThis alias above has the mirror problem: libraries that sniff Node via typeof global !== 'undefined' (buffer, readable-stream, bn.js and friends all do variants of this) will misdetect the environment.
The "don't clobber consumer values" guard is good and handles the case where someone already shimmed. The gap is the case where nothing is defined on purpose. A couple of options that keep the fix but shrink the radius: set NODE_ENV: 'production' on the shim's env, or install a minimal object with only the fields Dynamic actually reads rather than the full browserify shim.
|
|
||
| // The pinned source ends in `</svg>\n`; preserve that terminal byte in the | ||
| // base64 data URI instead of relying on editor treatment of a multiline asset. | ||
| config.walletIcon = `${config.walletIcon.slice(0, -2)}o=` as DataURIImage; |
There was a problem hiding this comment.
[suggestion] I verified this is correct — the literal ends in Pg== (2 bytes, >), and slice(0, -2) + 'o=' yields Pgo= (2 bytes, > \n). So the terminal newline does land, and the sha256 in the spec will match.
That said, this is very hard to audit: a reader has to hand-decode base64 to confirm the constant is what the comment claims, and it silently depends on the literal above ending with exactly two = padding chars. Any future re-encode of the icon changes that alignment and this line corrupts the data URI instead of failing loudly.
Since you already know the exact target bytes, encoding the trailing newline directly into the literal on L22 and deleting this line removes the coupling entirely. It also lets config stay immutable — right now the exported object is mutated after definition, which is easy to miss when reading config.ts top-down.
| Object.entries(unwaivedLock.packages) | ||
| .filter(([location]) => location === `node_modules/${name}` || location.endsWith(`/node_modules/${name}`)) | ||
| .map(([, metadata]) => metadata.version); | ||
| assert(lockedVersions('axios').includes('1.16.0')); |
There was a problem hiding this comment.
[suggestion] This asserts that the vulnerable versions are present, so the check goes red precisely when the underlying problem gets fixed upstream. The moment Dynamic corrects its transitive pins, this line (and L589's uuid companion, plus assert.notEqual(result.exitCode, 0, ...) at L488) fails and someone has to debug a CI failure that actually represents good news.
Inverting these to tolerate improvement — assert the audit is clean or that the only findings are within the accepted set — would make the job self-healing. If you want to keep the affirmative signal that the waiver is still needed, console.warn on "waiver no longer required" reads much better than a hard failure.
| ); | ||
| const serialized = JSON.stringify(report); | ||
| const advisories = [...new Set(serialized.match(/GHSA-[a-z0-9-]+/gi) ?? [])].sort(); | ||
| assert.deepEqual(advisories, [...acceptedBunAdvisories].sort()); |
There was a problem hiding this comment.
[suggestion] assert.deepEqual(advisories, [...acceptedBunAdvisories].sort()) requires the advisory set to match exactly — no more, no fewer. This couples a PR-blocking check to the GitHub Advisory Database, which changes independently of this repo. A brand-new advisory published against any dep in the Bun tree turns an unrelated PR red, and so does an advisory being withdrawn.
A subset assertion ("every reported advisory is in the accepted set") gives you the same protection against new exposure without failing on the withdrawal case. The README already promises "the verifier fails if Bun reports any new, missing, or different advisory", so this would need a wording tweak there too.
| await assertBrowserMetafile(join(npmConsumerDir, 'edge-meta.json'), npmLock); | ||
| await runEdgeBundle(join(npmConsumerDir, 'edge-esbuild.js')); | ||
| const viteBuild = await run(['npx', '--no-install', 'vite', 'build'], npmConsumerDir); | ||
| assert.doesNotMatch( |
There was a problem hiding this comment.
[nit] This only inspects viteBuild.stderr. Vite/Rollup routes plugin warnings — including the "...has been externalized for browser compatibility" message this regex is hunting for — through its own logger, which writes to stdout in most versions. Worth matching against ${viteBuild.stdout}${viteBuild.stderr} so the check can't silently pass.
| "@dynamic-labs/global-wallet-client": "^4.60.1", | ||
| "@wallet-standard/wallet": "^1.1.0" | ||
| "@dynamic-labs/global-wallet-client": "4.96.3", | ||
| "events": "3.3.0", |
There was a problem hiding this comment.
[suggestion] events isn't imported anywhere — I grepped packages/sei-global-wallet/src and scripts/check-sei-global-wallet.ts and found no reference. process is used (browserGlobal.ts imports process/browser.js), but events looks like it was added speculatively alongside it.
Unused runtime deps are worth removing rather than leaving in: this one ships to every consumer's install tree and adds a package to their audit surface for no benefit. If it's actually needed to satisfy a bundler resolution that the smoke test covers, a comment in the changeset explaining that would help, since it isn't discoverable from the source.
| // https://github.com/sei-protocol/sei-docs/blob/8710d6078058bdf67ec704b346f75ee9883e25ea/assets/brand/logo/sei-mark.svg | ||
| walletIcon: | ||
| 'data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMjU2IiBoZWlnaHQ9Ijk2IiB2aWV3Qm94PSIwIDAgMjU2IDk2IiBmaWxsPSJub25lIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPgo8ZyBjbGlwLXBhdGg9InVybCgjY2xpcDBfMzE5MV83MzI1KSI+CjxwYXRoIGZpbGwtcnVsZT0iZXZlbm9kZCIgY2xpcC1ydWxlPSJldmVub2RkIiBkPSJNNDcuNDYzNyA5NS4wODU4QzYxLjU5OTkgOTUuMDg1OCA3NC4zMDAyIDg4Ljk1ODMgODMuMDM1OCA3OS4yMjMxQzc4Ljk3MjIgNzUuNzAzNSA3Mi44NDgyIDc1LjQ4OTcgNjguNTIyIDc4Ljk2ODVMNjcuNjk1NSA3OS42MzMyQzU5Ljc3NDQgODYuMDAyOSA0OC4yMzI0IDg1LjA2MjIgNDEuNDU1IDc3LjQ5NDZDMzcuNzU4NiA3My4zNjcxIDMxLjQyODEgNzIuOTQ1OCAyNy4yMTM2IDc2LjU0NjZMMTcuNjg0NSA4NC42ODc5QzI1Ljg0NjEgOTEuMTk0OSAzNi4xOTg5IDk1LjA4NTggNDcuNDYzNyA5NS4wODU4Wk02Mi45MzE0IDcyLjA2MzlDNzAuNDc2MSA2NS45OTY3IDgxLjA4MDkgNjYuMjE0NCA4OC4zMzcyIDcyLjA3NDJDOTIuNjc0NSA2NC45MTIzIDk1LjE2OTkgNTYuNTE4MSA5NS4xNjk5IDQ3LjU0MjhDOTUuMTY5OSAzNy41Njk5IDkyLjA4ODkgMjguMzE0MyA4Ni44MjMzIDIwLjY2OTdDODMuNDI5IDE5Ljk0OTMgNzkuNzQ5NyAyMC43OTA3IDc2Ljk1NjggMjMuMjU3Mkw3Ni4xNjE5IDIzLjk1OTJDNjguNTQ0MSAzMC42ODY2IDU2Ljk3MSAzMC4yNzc3IDQ5Ljg1MDMgMjMuMDI5OUM0NS45NjY2IDE5LjA3NjkgMzkuNjIzNCAxOC45NDcxIDM1LjU4MDIgMjIuNzM3OUwyNC40OTEyIDMzLjEzNDZMMTguMzkwOCAyNi42NzI3TDI5LjQ4IDE2LjI3NTlDMzcuMDUzOCA5LjE3NDk0IDQ4LjkzNjUgOS40MTgxIDU2LjIxMTUgMTYuODIzQzYwLjAxMjcgMjAuNjkyMiA2Ni4xOTA5IDIwLjkxMDQgNzAuMjU3NSAxNy4zMTkxTDcxLjA1MjMgMTYuNjE3MUM3My41MjA3IDE0LjQzNzEgNzYuMzc2OCAxMi45Njc1IDc5LjM2NyAxMi4xOTQ2QzcwLjkxNTIgNC42MTM2MiA1OS43MzAxIDAgNDcuNDYzNyAwQzIzLjI0NjEgMCAzLjI0MzMyIDE3Ljk4MzQgMC4xNjc1MzYgNDEuMjgxMUM3LjUyMzM3IDM3Ljc2MzQgMTYuNTk2NyAzOS4xMTcgMjIuNTk0NCA0NS4yMzc4QzI2LjM4MDcgNDkuMTAxOSAzMi40NzczIDQ5LjUwOTcgMzYuNzQ3OSA0Ni4xODQ3TDQyLjk0ODEgNDEuMzU3NUM1MC43NCAzNS4yOTA4IDYxLjc1NTEgMzUuNTg0NCA2OS4yMTA1IDQyLjA1NzlMODEuMjgxNSA1Mi41Mzg0TDc1LjQzMzMgNTkuMjI3OEw2My4zNjI1IDQ4Ljc0NzFDNTkuMTIzMSA0NS4wNjYzIDUyLjg1OTggNDQuODk5MiA0OC40Mjg3IDQ4LjM0OUw0Mi4yMjg4IDUzLjE3NjFDMzQuMzgyNSA1OS4yODUgMjMuMTgxMyA1OC41MzU3IDE2LjIyNSA1MS40MzYzQzEyLjE3OTggNDcuMzA4MSA1LjU2Mzg4IDQ3LjE2NCAxLjM0MTY5IDUxLjExMjRMMCA1Mi4zNjY5QzEuMDA0MDkgNjIuMjk3IDUuMDc0MzMgNzEuMzI1MSAxMS4yNTI5IDc4LjQ5NjhMMjEuNDE5MiA2OS44MTA5QzI5LjMxMzkgNjMuMDY1OCA0MS4xNzI2IDYzLjg1NTMgNDguMDk3IDcxLjU4NjlDNTEuNzE1IDc1LjYyNjggNTcuODc2MyA3Ni4xMjg5IDYyLjEwNDkgNzIuNzI4NUw2Mi45MzE0IDcyLjA2MzlaIiBmaWxsPSIjOUUxRjE5Ii8+CjxwYXRoIGQ9Ik0xMjkuNTMzIDYyLjc5MDlDMTI5LjUzMyA3Mi43Mzg4IDEzNy4zNjkgNzkuMjYzOSAxNDkuNzEyIDc5LjI2MzlDMTYyLjE2NCA3OS4yNjM5IDE3MC43NSA3Mi45NTI4IDE3MC43NSA2My4wMDQ3QzE3MC43NSA1NS4xOTU5IDE2Ni4zNDkgNTEuMTMxMiAxNTcuNDQxIDQ4Ljk5MThMMTQ4LjIxIDQ2Ljc0NTNDMTQzLjA1NyA0NS40NjE4IDE0MC40ODIgNDMuMTA4NSAxNDAuNDgyIDM5LjU3ODZDMTQwLjQ4MiAzNC43NjQ5IDE0NC4wMjMgMzEuOTgzOCAxNTAuMzU2IDMxLjk4MzhDMTU2LjQ3NSAzMS45ODM4IDE2MC4wMTcgMzUuMjk5NyAxNjAuMjMyIDQwLjU0MTJIMTY5Ljg5M0MxNjkuNjc3IDMwLjU5MzEgMTYyLjE2NCAyNC4xNzUgMTUwLjY3OCAyNC4xNzVDMTM5LjA4NSAyNC4xNzUgMTMwLjkyOSAzMC4yNzIxIDEzMC45MjkgMzkuODk5M0MxMzAuOTI5IDQ3LjYwMTIgMTM1LjMzIDUyLjIwMDkgMTQ0LjY2NyA1NC40NDcxTDE1My44OTkgNTYuNTg2NUMxNTkuMzczIDU3Ljk3NzIgMTYxLjA5IDYwLjExNjUgMTYxLjA5IDYzLjY0NjZDMTYxLjA5IDY4LjQ2MDEgMTU3LjAxMSA3MS4zNDgzIDE0OS45MjcgNzEuMzQ4M0MxNDMuMjcyIDcxLjM0ODMgMTM5LjE5NCA2OC4xMzkyIDEzOS4xOTQgNjIuNzkwOUgxMjkuNTMzWiIgZmlsbD0iIzlFMUYxOSIvPgo8cGF0aCBkPSJNMjA2LjczMiA3OS4yNjM5QzIxOS43MjEgNzkuMjYzOSAyMjguNTIyIDcyLjk1MjggMjMxLjA5OCA2MS43MjEySDIyMS42NTNDMjE5LjkzNiA2Ny43MTEyIDIxNC43ODQgNzAuOTIwMyAyMDYuOTQ3IDcwLjkyMDNDMTk2Ljk2NCA3MC45MjAzIDE5MS4xNjkgNjQuODIzIDE5MC40MTggNTMuODA1NEwyMzAuOTkxIDUzLjY5ODNWNDkuOTU0NUMyMzAuOTkxIDM0LjQ0NCAyMjEuMjI0IDI0LjE3NSAyMDYuMzA0IDI0LjE3NUMxOTEuMDYyIDI0LjE3NSAxODAuNjUgMzUuNTEzNyAxODAuNjUgNTEuODhDMTgwLjY1IDY4LjEzOTIgMTkxLjI3NyA3OS4yNjM5IDIwNi43MzIgNzkuMjYzOVpNMjA2LjMwNCAzMi42MjU1QzIxNS4yMTMgMzIuNjI1NSAyMjAuOTAyIDM4LjQwMTkgMjIwLjkwMiA0Ny4wNjYzSDE5MC42MzNDMTkxLjkyMSAzNy44NjcgMTk3LjUwMiAzMi42MjU1IDIwNi4zMDQgMzIuNjI1NVoiIGZpbGw9IiM5RTFGMTkiLz4KPHBhdGggZD0iTTI0OS4zNDUgMTMuMjY0MkMyNTIuOTk0IDEzLjI2NDIgMjU2IDEwLjI2OSAyNTYgNi42MzIwN0MyNTYgMi44ODgxNyAyNTIuOTk0IDAgMjQ5LjM0NSAwQzI0NS42OTUgMCAyNDIuNjg5IDIuODg4MTcgMjQyLjY4OSA2LjYzMjA3QzI0Mi42ODkgMTAuMjY5IDI0NS42OTUgMTMuMjY0MiAyNDkuMzQ1IDEzLjI2NDJaTTI0NC40MDggNzcuOTgwM0gyNTQuNDk3VjI1LjY3MjVIMjQ0LjQwOFY3Ny45ODAzWiIgZmlsbD0iIzlFMUYxOSIvPgo8L2c+CjxkZWZzPgo8Y2xpcFBhdGggaWQ9ImNsaXAwXzMxOTFfNzMyNSI+CjxyZWN0IHdpZHRoPSIyNTYiIGhlaWdodD0iOTYiIGZpbGw9IndoaXRlIi8+CjwvY2xpcFBhdGg+CjwvZGVmcz4KPC9zdmc+Cg==', | ||
| 'data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNDAiIGhlaWdodD0iMjQwIiB2aWV3Qm94PSIwIDAgMjMuOTk2NSAyMy45OTY1IiBmaWxsPSIjMDAwMDAwIj48dGl0bGU+U2VpIG1hcms8L3RpdGxlPjxwYXRoIGQ9Ik0xMS45OTY5IDIzLjk5NjVIMTEuOTk2NUwxMS45OTY3IDIzLjk5NjVMMTEuOTk2OSAyMy45OTY1WiIvPjxwYXRoIGQ9Ik0xNi4wODUgMTkuNTI1NkMxNi44MDYxIDE4Ljg2ODkgMTcuOTA5MiAxOC44NjkgMTguNjMwMyAxOS41MjU4TDE4LjYzMDUgMTkuNTI1OUwyMC4wOTEgMjAuODU1OEMxNy45NTcgMjIuODA2NSAxNS4xMTU4IDIzLjk5NjUgMTEuOTk2NyAyMy45OTY1QzguODg4MjQgMjMuOTk2NCA2LjA1NTkxIDIyLjgxNDcgMy45MjQ0OCAyMC44NzZMNS40MDcyIDE5LjUyNTlMNS40MDczNCAxOS41MjU4QzYuMTI4NDQgMTguODY5IDcuMjMxNDcgMTguODY4OSA3Ljk1MjY1IDE5LjUyNTZMOS4yNTA4OCAyMC43MDg3TDkuMjUxMDkgMjAuNzA4OUMxMC44MTk1IDIyLjEzNzQgMTMuMjE4MiAyMi4xMzc0IDE0Ljc4NjYgMjAuNzA4OUwxNC43ODY4IDIwLjcwODdMMTYuMDg1IDE5LjUyNTZMMTYuMDg1IDE5LjUyNTZaIi8+PHBhdGggZD0iTTExLjk5NjUgMEMxNS4xMjE0IDAgMTcuOTY3MiAxLjE5NDIxIDIwLjEwMjUgMy4xNTEwNUMyMC4wNDEyIDMuMjAwNzUgMTkuOTgxIDMuMjUyNDUgMTkuOTIyIDMuMzA2MTlMMTkuOTIxOCAzLjMwNjM2TDE4LjYyMzYgNC40ODk0NEMxNy45MDI1IDUuMTQ2MDUgMTYuNzk5NiA1LjE0NjA1IDE2LjA3ODUgNC40ODk0NEwxNC43ODAzIDMuMzA2MzZMMTQuNzggMy4zMDYxOUwxNC43NDMxIDMuMjcyOTZDMTMuMTc0MiAxLjg3NzcyIDEwLjgwMDcgMS44ODg4MyA5LjI0NDU0IDMuMzA2MjJMNS45MjU2NiA2LjMyODMyTDcuNDIwMzggNy45NjkzOUwxMC43Mzk0IDQuOTQ3MTVMMTAuNzM5NSA0Ljk0NzA0QzExLjQ2MDcgNC4yOTAyNSAxMi41NjM3IDQuMjkwMTYgMTMuMjg0OSA0Ljk0NjgzTDE0LjU4MzEgNi4xMjk5MUwxNC41ODMzIDYuMTMwMTJDMTYuMTUxNyA3LjU1ODY2IDE4LjU1MDQgNy41NTg2NiAyMC4xMTg4IDYuMTMwMTJMMjAuMTE5IDYuMTI5OTRMMjEuNDE3MiA0Ljk0Njg3TDIxLjQxNzIgNC45NDY4M0MyMS40NzUzIDQuODkzOTEgMjEuNTM1OSA0Ljg0NTMgMjEuNTk4NiA0LjgwMDkxQzIzLjEwNDMgNi44MDU5MiAyMy45OTY1IDkuMjk3OSAyMy45OTY1IDExLjk5ODNDMjMuOTk2NSAxNC43MDY5IDIzLjA5ODggMTcuMjA1OCAyMS41ODQ3IDE5LjIxMzlMMjAuMTI1MyAxNy44ODVMMjAuMTI1MyAxNy44ODQ5QzE4LjU1NjkgMTYuNDU2NCAxNi4xNTgyIDE2LjQ1NjQgMTQuNTg5OCAxNy44ODQ5TDE0LjU4OTYgMTcuODg1MUwxMy4yOTE0IDE5LjA2ODJDMTIuNTcwMyAxOS43MjQ4IDExLjQ2NzQgMTkuNzI0OCAxMC43NDYzIDE5LjA2ODJMOS40NDgwNSAxNy44ODUxTDkuNDQ3ODQgMTcuODg0OUw5LjQxMDkyIDE3Ljg1MTdDNy44NDIgMTYuNDU2NSA1LjQ2ODQ5IDE2LjQ2NzYgMy45MTIzMyAxNy44ODVMMi40MjY2NCAxOS4yMzc4QzEuMzgxMjcgMTcuODU4NCAwLjYyNzM2NSAxNi4yNDYxIDAuMjU2NTM2IDE0LjQ5MjRMMi43MzQ1OCAxMi4yMzZMMi43MzQ3MiAxMi4yMzU5QzMuNDU1ODYgMTEuNTc5IDQuNTU4NjEgMTEuNTc5IDUuMjc5NzggMTIuMjM1OEw2LjU3ODM3IDEzLjQxODhMNi41Nzg0NyAxMy40MTg5QzguMTQ2ODEgMTQuODQ3NCAxMC41NDU1IDE0Ljg0NzYgMTIuMTEzNyAxMy40MTg4TDEzLjQxMjIgMTIuMjM1OEMxNC4xMzM0IDExLjU3OSAxNS4yMzY1IDExLjU3OSAxNS45NTc3IDEyLjIzNTlMMTUuOTU3OCAxMi4yMzZMMTkuMjc2OSAxNS4yNTgyTDIwLjc3MTYgMTMuNjE3MUwxNy40NTI3IDEwLjU5NUwxNy40NTI3IDEwLjU5NUMxNS44ODQzIDkuMTY2NDYgMTMuNDg1NiA5LjE2NjQ2IDExLjkxNzIgMTAuNTk1TDExLjkxNzEgMTAuNTk1MUwxMC42MTg1IDExLjc3ODJMMTAuNjE4NSAxMS43NzgyQzkuODk3NzkgMTIuNDM0OCA4Ljc5NDc2IDEyLjQzNSA4LjA3MzUyIDExLjc3ODJMNi43NzQ5MyAxMC41OTUxTDYuNzc0ODYgMTAuNTk1TDYuNzM3OTUgMTAuNTYxOEM1LjE2OTAzIDkuMTY2NTQgMi43OTU4NyA5LjE3NzY1IDEuMjM5NzEgMTAuNTk1TDAgMTEuNzIzOUMwLjE0NTg3NiA1LjIyNDM4IDUuNDYwNzEgNy4wNzkxM2UtMDYgMTEuOTk2NSAwWiIvPjwvc3ZnPg==', |
There was a problem hiding this comment.
[nit] The new mark is a solid #000000 fill on a transparent background, replacing the #9E1F19 wordmark. Per REVIEW_GUIDELINES §2 I'm not disputing the asset itself — the source is cited and the sha256 is pinned, which is exactly the right treatment.
The usability question is separate: EIP-6963 icons render in wallet-selection modals, and RainbowKit / ConnectKit / Web3Modal all default to dark surfaces. A pure-black transparent SVG will be close to invisible there. Most wallets ship an icon with its own background fill for this reason. Worth checking against a dark modal before release; if the brand kit has a variant with a background plate, that would be the safer pick.
| @@ -0,0 +1,48 @@ | |||
| --- | |||
| "@sei-js/sei-global-wallet": patch | |||
There was a problem hiding this comment.
[suggestion] patch understates this. The package is at 1.4.1, so semver applies, and tightening optional peer ranges to exact versions (see the package.json comment) means consumers who upgrade within a patch range can hit ERESOLVE on their next install. A dependency-resolution break is the kind of thing a patch bump specifically promises won't happen.
If the exact peer pins stay, this should be major. If they're relaxed to bounded ranges, minor fits the new exports (registerEIP6963Provider, unregisterEIP6963Provider, eip6963ProviderInfo, registerSolanaStandard) nicely.
Keep the optional ZeroDev entrypoint out of workspace tests so typecheck no longer loads the AA type graph, and scan hoisted Dynamic 4.96.1 copies in npm lockfiles. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A large, well-tested hardening pass on @sei-js/sei-global-wallet (browser/edge global shims, EIP-6963 re-announcement, idempotent Solana registration, a real consumer smoke harness). Two things block: the viem peer range is pinned to an exact version that is stricter than Dynamic's own ^2.45.3 contract and will cause ERESOLVE for downstream dApps, and the peer-contract narrowing is released as a patch.
Findings: 2 blocking | 13 non-blocking | 9 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
cursor-review.mdis empty — the Cursor second-opinion pass produced no output, so this review reflects only Codex's single finding plus my own.- The README and the changeset carry near-identical multi-paragraph waiver text (overrides blocks, GHSA list, viem/Vite rationale). Changesets become release notes; consider keeping the operational detail in the README and reducing the changeset to a short summary plus a link, so the two can't drift.
scripts/check-sei-global-wallet.tsgates the smoke workflow on livenpm install/npm audit/bun auditagainst the public registry and advisory DB, plusesbuildandvitebuilds. That will be slow and intermittently red for reasons unrelated to the PR that triggers it. Consider running the audit-assertion portion on a schedule and keeping the PR-triggered job to build/pack/entrypoint checks.SEI_GLOBAL_WALLET_FAST_CHECK=1(scripts/check-sei-global-wallet.ts:37) skips the unwaived npm consumer, the waived npm consumer, and the entire Bun consumer path. It is not mentioned in the README's "Release verification" section or in the workflow — worth documenting so nobody assumes a greentest:sei-global-wallet-releasealways covered Bun.@wallet-standard/baseis imported type-only inconfig.tsandregisterSolanaStandard.tsbut is only an optional peer. If TypeScript ever pullsdist/lib/config.d.tsinto a consumer's program, an unresolved type import surfaces for anyone who didn't install it. The smoke harness'stypecheck.tscan't catch this because that consumer installs every peer and setsskipLibCheck: true.- No prompt-injection or instruction-like content was found in the diff, commit messages, or PR description.
- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| "@zerodev/sdk": "5.4.36", | ||
| "viem": "^2.7.12" | ||
| "@zerodev/sdk": "5.5.7", | ||
| "viem": "2.45.3", |
There was a problem hiding this comment.
[blocker] Exact viem peer pin is stricter than the contract it claims to mirror, and will break consumers.
The changeset says this release "mirror[s] its optional peer contract" and the README calls this "the compatible Viem floor", but Dynamic 4.96.3 declares "viem": "^2.45.3" (visible in the bun.lock hunk for @dynamic-labs/global-wallet-client). 2.45.3 is not a floor — it is an exact pin, and it is narrower than upstream.
Every other exact peer here (@dynamic-labs/ethereum-aa@4.96.3, @solana/web3.js@1.98.1, @zerodev/sdk@5.5.7, zksync-sso@0.2.0) does match Dynamic exactly. viem is the one deviation, and it's the one peer nearly every consumer already has: any app on wagmi/RainbowKit/ConnectKit will be on some viem 2.x newer than 2.45.3 and will hit ERESOLVE on npm install (optional peers still conflict once the package is actually present).
The stated motivation — 2.55.19 pulling Ox Tempo's node:worker_threads into Vite resolution — is a property of the harness's own bundle, not something that warrants forcing a global downgrade on every downstream dApp. Suggest "viem": "^2.45.3" here to match upstream, and keep the exact 2.45.3 in devDependencies (line 39) and in fullConsumerManifest so the smoke test still exercises the known-good graph.
| @@ -0,0 +1,48 @@ | |||
| --- | |||
| "@sei-js/sei-global-wallet": patch | |||
There was a problem hiding this comment.
[blocker] patch under-states this change for a published package.
This release narrows or relocates the peer contract in ways that can fail a consumer's install or type resolution:
viem:^2.7.12→2.45.3@dynamic-labs/ethereum-aa:^4.15.0→4.96.3@zerodev/sdk:5.4.36→5.5.7@solana/web3.js:^1.92.1→1.98.1zksync-sso@0.2.0added as a new (optional) peer@wallet-standard/walletmoved out ofdependenciesinto optionalpeerDependencies— correct, since nothing insrc/imports it, but it does stop being installed transitively
A consumer taking this as a patch bump via ^1.4.1 can get an install that no longer resolves. .changeset/config.json uses fixed: [] / linked: [], so bumping just this package to minor (or major) costs nothing elsewhere.
| "@dynamic-labs/global-wallet-client": "^4.60.1", | ||
| "@wallet-standard/wallet": "^1.1.0" | ||
| "@dynamic-labs/global-wallet-client": "4.96.3", | ||
| "events": "3.3.0", |
There was a problem hiding this comment.
[suggestion] events@3.3.0 is declared as a runtime dependency but nothing under packages/sei-global-wallet/src/ imports it (process/browser.js is the only shim actually imported, in browserGlobal.ts:1). Unless this is deliberately present so a bundler can resolve a transitive bare events specifier — in which case it needs a comment saying so, because it looks removable — it ships an unused package to every consumer. Please drop it or document why it's load-bearing.
| - 'bun.lock' | ||
| - 'package.json' | ||
| - 'packages/sei-global-wallet/**' | ||
| - 'scripts/check-sei-global-wallet.ts' |
There was a problem hiding this comment.
[suggestion] The job runs test:sei-global-wallet-release, which imports scripts/dynamic-package-lock.js (check-sei-global-wallet.ts:8) for the stale-Dynamic-4.96.1 graph assertion — but neither scripts/dynamic-package-lock.ts nor its test is in this path filter. A PR that changes the dependency-graph enforcement therefore skips the very check it modifies. Add - 'scripts/dynamic-package-lock*' alongside this entry.
(Raised by Codex; confirmed.)
| }); | ||
| } | ||
|
|
||
| if (typeof runtime.process === 'undefined') { |
There was a problem hiding this comment.
[suggestion] This installs a process shim onto the page's global object as a side effect of importing any entrypoint — dynamicClient, dynamicEthereum, dynamicFeatures, dynamicSolana, and dynamicZerodev all import './browserGlobal.js', so merely import '@sei-js/sei-global-wallet' mutates the consumer's global scope in a browser.
process/browser.js sets process.browser = true and process.env = {}. Any other library on the page that branches on typeof process !== 'undefined' to decide "I'm in Node" now takes the wrong branch, and reads of process.env.X that previously threw now silently yield undefined. The typeof === 'undefined' guard and writable: true are good mitigations, but they don't help a library that loads after this one.
If Dynamic only needs these on its own module scope, prefer scoping the shim (bundler define/inject documented for consumers, or a wrapper that passes them in). If a true global really is required, the README's "All entrypoints are ESM-only…" paragraph should state plainly that importing the package defines globalThis.process and globalThis.global when absent.
|
|
||
| // The pinned source ends in `</svg>\n`; preserve that terminal byte in the | ||
| // base64 data URI instead of relying on editor treatment of a multiline asset. | ||
| config.walletIcon = `${config.walletIcon.slice(0, -2)}o=` as DataURIImage; |
There was a problem hiding this comment.
[suggestion] I verified this is correct — the literal's final base64 group is Pg== (one byte, >), and slice(0, -2) + 'o=' rewrites it to Pgo= (> + \n) — but the correctness depends on an invariant that is invisible at the call site: the literal must decode to a length ≡ 1 (mod 3). Re-encode the SVG with any other trailing byte and this silently corrupts the icon instead of failing loudly.
Since the base64 is already a single-line string literal, the stated reason ("instead of relying on editor treatment of a multiline asset") doesn't apply. Just embed the final value ending in …c3ZnPgo= and delete the post-hoc mutation; the SHA-256 assertion in config.spec.ts already pins the result either way.
| }; | ||
|
|
||
| const assertAcceptedBunAudit = (result: ProcessResult) => { | ||
| assert.notEqual(result.exitCode, 0, 'Bun AA consumer audit unexpectedly reported a clean dependency tree'); |
There was a problem hiding this comment.
[suggestion] assert.notEqual(result.exitCode, 0, ...) plus the exact-set assertions below make the smoke job fail whenever the Bun advisory set changes in either direction — including when upstream finally fixes bn.js/ws (the good outcome) or when GitHub publishes any new advisory for the AA subtree. The failure will land on whichever unrelated wallet PR happens to run next, and the message ("unexpectedly reported a clean dependency tree") won't obviously read as "the waiver can be removed now."
At minimum, make the clean-tree message actionable ("advisories resolved upstream — drop the waiver from README.md and this script"). Better: assert the accepted set is a superset check with a warning on shrinkage, and move the strict equality to a scheduled job.
| @@ -0,0 +1,8 @@ | |||
| export const STALE_DYNAMIC_VERSION = '4.96.1'; | |||
There was a problem hiding this comment.
[nit] STALE_DYNAMIC_VERSION hardcodes 4.96.1, so this guard only ever detects that one release. As soon as Dynamic ships 4.96.4 or 4.97, a genuinely stale nested subtree passes silently. Consider asserting the inverse instead — every @dynamic-labs/* location resolves to the expected pinned version — which stays meaningful across upgrades.
| "module": "./dist/index.js", | ||
| "types": "./dist/index.d.ts", | ||
| "license": "ISC", | ||
| "sideEffects": true, |
There was a problem hiding this comment.
[nit] "sideEffects": true is equivalent to the previous state (field absent), so nothing regresses. But since only eip6963.js, solana.js, and lib/browserGlobal.js actually have side effects, an array — ["./dist/eip6963.js", "./dist/solana.js", "./dist/lib/browserGlobal.js"] — would let bundlers tree-shake the root entry for consumers who only pull a couple of named exports.
This release widens optional peer ranges and keeps compatible installs working without narrowing the contract, while preserving direct wallet-standard resolution and required transient runtime shims. It installs missing browser globals before Dynamic loads, adds EIP-6963 cleanup and provider metadata safeguards, and expands the release smoke checks to cover clean consumer installs, SSR, browser bundles, dependency graphs, and package contents. The patch also updates the README and changeset to document the temporary consumer override waiver and the verified Dynamic peer matrix.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8428369. Configure here.
| ownDiagnostics, | ||
| [], | ||
| `Published declarations do not resolve without optional peers installed:\n${ownDiagnostics.join('\n')}\n\nFull output:\n${output}` | ||
| ); |
There was a problem hiding this comment.
Typecheck ignores consumer failures
Medium Severity
assertDeclarationsResolveWithoutOptionalPeers runs tsc with failures allowed and only treats diagnostics whose path matches @sei-js/sei-global-wallet/dist/ as failures. Errors attributed to strict-types.ts itself—missing exports, unresolved subpaths, or other public-API type breaks—leave ownDiagnostics empty, so the assert passes and the release verifier can green-light broken consumer typings.
Reviewed by Cursor Bugbot for commit 8428369. Configure here.
There was a problem hiding this comment.
A large, well-documented hardening pass on @sei-js/sei-global-wallet (Dynamic 4.96.3 floor, Node-global shim, hand-rolled EIP-6963 announce/cleanup, memoized Solana registration, and an extensive consumer-install verifier plus CI workflow). No blocking correctness or security defects found — module-evaluation order for the global shim is correct, the wallet identity (rdns, uuid, environmentId, walletUrl) is unchanged, peer ranges only widen, and a minor changeset is present; the findings below are packaging, drift-resilience, and documentation nits.
Findings: 0 blocking | 15 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage was partial:
cursor-review.mdis empty (that pass produced no output) andcodex-review.mdreports "No material issues found", so this review is effectively the only detailed pass over the diff. - Release posture on the security waiver: the package ships with known-vulnerable transitive
axios/uuidand relies on every consumer hand-adding rootoverridesfrom the README. Realistically most downstream dApps won't, so the practical outcome is that they ship the vulnerable tree. The^4.96.3range does mean an upstream Dynamic fix propagates without a release here, which is the right mitigation — worth stating explicitly in the changeset that this is the intended path out, and worth confirming there is a tracking issue against Dynamic. - The new
Sei Global Wallet Consumer Smokeworkflow depends on live registry + advisory data on a daily cron. Two operational gaps: (a) no dependency caching and five sequentialnpm installs including@solana/web3.js,viem,@zerodev/sdk, plus esbuild and Vite builds, against a 20-minute timeout — likely tight on a cold runner; (b) scheduled-workflow failures onmainonly notify the last committer by default, so a drift failure can sit unnoticed. Consideractions/setup-nodecaching and an explicit failure notification. - Test gap:
registerSolanaStandard()'s SSR early return is never asserted.entrypoints.spec.tsimports../../solanawith nowindowand only checks that the export is a function; add an assertion that the call returnsundefinedand thatcreateSolanaWalletwas not invoked, so the guard can't silently regress. - README duplicates the identical Bun override block twice (once under "Complete Bun root overrides when the optional AA path is not enabled" and again for the Bun AA case). It reads as an oversight rather than an intentional restatement — consider collapsing to one block with a sentence noting Bun's block is the same in both cases.
- Behavioural note worth calling out in the changeset:
./ethereumand the root entrypoint were previously pure re-exports and now mutateglobalThison import vialib/browserGlobal.js. The README documents this under "Globals installed on import", but the changeset's summary doesn't, and it's the kind of thing a consumer debugging global pollution will look for in release notes first. - Cannot verify from the diff alone: the PR replaces Dynamic's
announceEip6963Provider()with a hand-rolledCustomEventdispatch. If that helper did anything beyond dispatching (analytics,window.ethereumshimming, Dynamic-side bookkeeping), dropping it is a silent behaviour change. Please confirm against@dynamic-labs/global-wallet-client@4.96.3's implementation — the new browser integration test only asserts the event shape, which wouldn't catch a missing side effect. - 8 suggestion(s)/nit(s) flagged inline on specific lines.
| "@solana/web3.js": "1.98.1", | ||
| "@wallet-standard/base": "^1.0.1", | ||
| "@wallet-standard/features": "^1.0.3", | ||
| "@wallet-standard/wallet": "^1.1.0", |
There was a problem hiding this comment.
[nit] @wallet-standard/wallet is now declared in both dependencies (line 28) and devDependencies. npm and Bun resolve the runtime entry and ignore this one, so it is a no-op today, but it is a drift hazard: bumping only this line silently changes nothing, and bumping only line 28 leaves a stale-looking dev range behind. Drop the devDependency entry.
| "@wallet-standard/wallet": "^1.1.0" | ||
| "@dynamic-labs/global-wallet-client": "^4.96.3", | ||
| "@wallet-standard/wallet": "^1.1.0", | ||
| "events": "3.3.0", |
There was a problem hiding this comment.
[suggestion] events is declared as a runtime dependency of this package purely so a different package (@zerodev/sdk) can resolve its bare events import — the README says as much. That only works under hoisted layouts. 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 the documented Could not resolve "events" failure still occurs. The verifier only exercises npm and Bun, both hoisted, so this gap is untested.
It also ships to every consumer of the root / ./eip6963 / ./ethereum entrypoints, none of which touch ZeroDev. Suggest documenting the pnpm/PnP caveat in the "Optional peer versions" section and pointing those users at a bundler alias, since the dependency can't be a general fix.
| // `process/browser.js` ships an empty `env`. Libraries that branch on | ||
| // `process.env.NODE_ENV !== 'production'` would otherwise take their | ||
| // development path inside a production bundle. | ||
| processShim.env.NODE_ENV ??= 'production'; |
There was a problem hiding this comment.
[suggestion] This mutates the process/browser.js module singleton, not a private copy. The install guard is typeof globalThis.process === 'undefined', but a consumer can be using process/browser.js locally — e.g. webpack's ProvidePlugin: { process: 'process/browser.js' }, which is the standard setup and does not define globalThis.process. In that configuration this branch is taken, and the consumer's own process object gets NODE_ENV forced to production even in a development build, so their libraries take production branches.
The README's guarantee ("neither is installed when the consumer or runtime already defines it") holds for the defineProperty on line 32, but not for this line — the mutation escapes the global check. Consider setting NODE_ENV on a shallow copy of the shim rather than the shared module object:
const shim = { ...processShim, env: { NODE_ENV: 'production', ...processShim.env } };
install('process', shim);| const viteFiles = (await readdir(join(npmConsumerDir, 'vite-dist'))).filter((path) => path.endsWith('.js')); | ||
| assert.equal(viteFiles.length, 1, `Expected one Vite bundle, found: ${viteFiles.join(', ')}`); | ||
| await runBrowserBundle(join(npmConsumerDir, 'vite-dist', viteFiles[0]), true); | ||
| assertAuditClean(parseJsonOutput<AuditReport>((await run(['npm', 'audit', '--json'], npmConsumerDir)).stdout), 'Full npm consumer'); |
There was a problem hiding this comment.
[suggestion] The full npm consumer asserts a hard total === 0 against the live advisory database. The Bun path deliberately tolerates drift (subset check, "reports rather than fails when one stops being reported") precisely so that "a withdrawn or upstream-fixed advisory must not fail an unrelated pull request" — but the reverse case isn't handled here: a newly published advisory anywhere in the viem/@solana/web3.js/@zerodev/sdk tree turns the daily cron and every wallet-touching PR red with no change to this repo.
Since this workflow is path-triggered on bun.lock and package.json, that will surface on changes unrelated to the wallet. Consider giving npm the same accepted-advisory-set treatment (empty set today), so a new finding produces an actionable diff against a declared baseline rather than an opaque failure.
|
|
||
| const report = parseJsonOutput<Record<string, Array<{ severity?: string; url?: string }>>>(result.stdout); | ||
| const serialized = JSON.stringify(report); | ||
| const reported = new Set(serialized.match(/GHSA-[a-z0-9-]+/gi) ?? []); |
There was a problem hiding this comment.
[nit] The regex is case-insensitive (/gi) but membership is tested case-sensitively against acceptedBunAdvisories (lowercase) via includes on line 588 and reported.has on line 596. If bun audit ever emits an ID with different casing, the same advisory would both fail the unwaived check and be reported as "fixed upstream". GHSA IDs are lowercase in practice, so this is theoretical — normalizing with .map((id) => id.toLowerCase()) here removes the coupling.
| Wallet | ||
| ); | ||
| registerWallet(wallet); | ||
| registeredWallet = wallet; |
There was a problem hiding this comment.
[nit] registeredWallet is memoized for the module lifetime with no way to clear it, which is asymmetric with the EIP-6963 path in this same PR (unregisterEIP6963Provider). A consumer that can tear down and re-register the EIP-6963 provider has no equivalent for wallet-standard, and HMR will keep handing back a wallet bound to a stale Wallet client. Consider an unregisterSolanaStandard() that resets this, for parity.
| rdns: 'io.sei.global-wallet' | ||
| rdns: 'io.sei.global-wallet', | ||
| // Stable UUIDv4 for the EIP-6963 provider identity. | ||
| uuid: '36b63d10-7ba6-49a3-9614-22f471b9283c' |
There was a problem hiding this comment.
[nit] Not a regression — this is the same value the previous code passed as uuid via config.environmentId — but the changeset bills it as a fix ("use stable UUIDv4 provider metadata"), and EIP-6963 actually specifies the opposite: uuid must "uniquely distinguish different EIP-1193 provider sessions ... during the lifetime of the page". A constant is common in the wild and low-risk, but it interacts with the new cleanup helpers: unregisterEIP6963Provider() followed by registerEIP6963Provider() creates a fresh createEIP1193Provider(Wallet) instance announced under the same uuid, which a discovery UI keying on uuid may treat as the old session. Worth either generating per-registration with crypto.randomUUID(), or rewording the changeset so it doesn't read as a spec-conformance fix.
| "module": "./dist/index.js", | ||
| "types": "./dist/index.d.ts", | ||
| "license": "ISC", | ||
| "sideEffects": true, |
There was a problem hiding this comment.
[nit] "sideEffects": true is correct and conservative, but it's also the bundler default when the field is absent, so it buys nothing while ruling out tree-shaking entirely. An array would be more useful: the side effects live in dist/eip6963.js, dist/solana.js, and dist/lib/browserGlobal.js, so listing those would let bundlers drop unused re-exports from dist/index.js for consumers that only use a subpath.
Superseded: latest AI review found no blocking issues.
Dynamic's lockfile refresh nested zod 4 under the MCP SDK while mcp-server stayed on zod 3, so tsc compared two recursive schema graphs until the runner's 4GB heap died. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Substantial, well-documented hardening of @sei-js/sei-global-wallet (browser/SSR globals, EIP-6963 lifecycle, packaging) plus a release-verification harness and CI workflow; I found no correctness or security blockers, but the new consumer smoke check pins several floating upstream versions that will drift, and the package-scoped process global shim is a broad cross-library side effect worth calling out. Changesets are present for both published packages, matching guideline §3.
Findings: 0 blocking | 12 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only Codex (which reported no material findings, and could not run tests) and my own pass. events@3.3.0andprocess@0.11.10are harddependencies, but neither is imported by the package's own source for the common paths —eventsexists only so@zerodev/sdk(an optional peer) resolves when bundling./zerodev. Every consumer, including ones that only use./eip6963, now installs both. Consider an optional peer foreventsso the cost falls on AA consumers only; the README already explains the constraint, so this is a packaging-hygiene note rather than a defect.- This release knowingly ships a dependency tree with vulnerable transitive
axios/uuidand relies on consumers copying rootoverridesout of the README. That is the right call given the upstream pins, and the changeset carries a[!WARNING], but most downstream dApps will never read it. Worth tracking the Dynamic fix as a follow-up issue so the waiver actually gets removed rather than becoming permanent — the verifier'sreportWaiverProgressoutput only surfaces in CI logs. - Scope: the PR title is
fix(sei-global-wallet)but it also bumps@sei-js/mcp-server'szod3→4 and addsscripts/mcp-zod-lock.*. It has its own changeset and a lockfile-guard test, and the rationale is sound (Dynamic pullszod@4.0.5, so an unaligned mcp-server produced a second copy) — just noting it for release-notes readers who won't expect an mcp-server change here. sei-global-wallet-smoke.ymlsetstimeout-minutes: 20for a job that does two cleannpm installs, a fullbun install, three esbuild bundles, a Vite build, and twonpm auditruns against the live registry. That is likely fine today but leaves little headroom; a registry slowdown will read as a hard failure rather than a timeout you can distinguish.packages/sei-global-wallet/src/lib/__tests__/browserIntegration.spec.tsimports the real../../solanaand../../eip6963, which instantiate the actual Dynamic client against a hand-rolledEventTargetwindow and aMemoryStorage. It is a genuine integration test living in the unit suite (bun test --isolate src); if Dynamic starts touchingdocumentornavigatorit will fail for reasons unrelated to this package. Consider noting the coupling, or moving it alongside the release verifier.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| assertNpmDynamicGraph(npmLock); | ||
| assert.equal(npmLock.packages['node_modules/ethjs-unit/node_modules/bn.js']?.version, '4.12.5'); | ||
| assert.equal(npmLock.packages['node_modules/number-to-bn/node_modules/bn.js']?.version, '4.12.5'); | ||
| assert.equal(npmLock.packages['node_modules/bn.js']?.version, '5.2.5'); |
There was a problem hiding this comment.
[suggestion] These assertions pin exact versions that this repo does not control and that are not held by any lockfile — the consumer dirs are installed fresh from the live registry.
node_modules/bn.jsresolves5.2.5from Solana's^5.xrange → abn.js@5.2.6release turns this red.node_modules/jayson/node_modules/wsresolves7.5.13from Jayson's^7.5.10→ same forws@7.5.14.- Same pattern at lines 813–817 for the Bun lock (
bn.js@5.2.5,jayson/ws@7.5.13).
Since this workflow gates PRs and runs on a daily cron, an unrelated upstream patch release will fail main and every open PR touching packages/sei-global-wallet/**. The lines that are pinned by your overrides (node_modules/ws = 8.21.0, the scoped bn.js = 4.12.5, viem's ws = 8.18.3 under the pinned viem@2.45.3) are stable and worth keeping exact. For the floating ones, asserting the intended invariant instead — major/minor line, or "Jayson stayed on ws 7 while the hoisted copy is ws 8" — keeps the check meaningful without the drift.
| // `process/browser.js` ships an empty `env`. Libraries that branch on | ||
| // `process.env.NODE_ENV !== 'production'` would otherwise take their | ||
| // development path inside a production bundle. | ||
| processShim.env.NODE_ENV ??= 'production'; |
There was a problem hiding this comment.
[suggestion] Installing a page-wide process with NODE_ENV=production is a large blast radius for a side effect of import '@sei-js/sei-global-wallet'. Two consequences beyond Dynamic:
typeof process !== 'undefined'is the most common Node-vs-browser sniff. After any entrypoint of this package loads, every library initialised afterwards seesprocessdefined. (process/browser.jshas noversions, so the stricterprocess.versions?.nodechecks are unaffected — the loose ones are not.)- A consumer whose dev bundle reads
NODE_ENVat runtime rather than through a bundlerdefinewill now take its production branch during development — losing React's dev warnings, for example.
The README documents both and the "don't overwrite what the consumer set" guard is the right shape, so this isn't a defect. But if Dynamic only needs these on its own module scope, scoping the shim there rather than to globalThis would avoid changing behaviour for unrelated libraries in the host app.
| "@solana/web3.js": "1.98.1", | ||
| "@wallet-standard/base": "^1.0.1", | ||
| "@wallet-standard/features": "^1.0.3", | ||
| "@wallet-standard/wallet": "^1.1.0", |
There was a problem hiding this comment.
[nit] @wallet-standard/wallet is already a runtime dependency at line 28, so this devDependencies entry is a no-op. Worse, the two ranges can silently diverge on a future bump and only the dependencies one will affect consumers. Suggest dropping this line — entrypoints.spec.ts already asserts the dependencies entry is the load-bearing one.
| "express": "^4.21.2", | ||
| "viem": "2.55.16", | ||
| "zod": "^3.24.2" | ||
| "zod": "^4.0.5" |
There was a problem hiding this comment.
[suggestion] The @modelcontextprotocol/sdk floor is still ^1.17.5 (line 36), which predates the SDK accepting zod 4 — the versions that do declare "zod": "^3.25 || ^4.0" are considerably later (your lockfile resolves 1.30.0). npm/bun will normally pick the newest 1.x, so this repo is fine and mcp-zod-lock.test.ts guards it. But a consumer who pins the SDK anywhere in 1.17.x–1.1x.x gets zod 4 hoisted for @sei-js/mcp-server and a nested zod 3 under the SDK; the instanceof ZodType checks in server.tool/server.prompt then fail against schemas built by the other copy. Raising the SDK floor to the first release that declares zod-4 support makes the pairing unresolvable rather than silently broken.
| rdns: 'io.sei.global-wallet' | ||
| rdns: 'io.sei.global-wallet', | ||
| // Stable UUIDv4 for the EIP-6963 provider identity. | ||
| uuid: '36b63d10-7ba6-49a3-9614-22f471b9283c' |
There was a problem hiding this comment.
[nit] Two small things on the new uuid:
- It is byte-identical to
environmentIdabove. That is unchanged behaviour (the old code passedconfig.environmentIdas the EIP-6963uuid), but as two independent literals they can now drift apart without anything noticing. A comment saying they are intentionally the same value — or deriving one from the other — would make the coupling explicit. - EIP-6963 specifies
uuidas a UUIDv4 that distinguishes provider sessions with otherwise-matching properties "during the lifetime of the page", i.e. generated per instance rather than baked in. A constant is what most wallets ship in practice and is harmless with a single provider instance, so I'd leave it — just flagging that "stable UUIDv4" in the changeset is a deliberate deviation from the spec's wording, not conformance to it.
| } | ||
|
|
||
| // The documented Axios and UUID overrides must still be taking effect. | ||
| assert.doesNotMatch(serialized, /axios|uuid/i); |
There was a problem hiding this comment.
[nit] Two loose edges in the waiver check, both low-probability:
- This regex scans the whole serialised report, so an advisory on an unrelated package whose name or title merely contains
uuid(@lukeed/uuid,uuid-parse, …) fails the run with a misleading "overrides stopped taking effect" reading. Checking foraxios/uuidas top-level keys of the parsed report would say exactly what you mean. reported(line 580) is built only fromGHSA-matches, so an advisory that surfaced without a GHSA id would be invisible to theunwaivedassertion and silently pass. Bun's audit output does carry GHSA URLs today, so this is defensive rather than a live gap — but a count check (reported.sizevs. the number of findings in the report) would close it cheaply.
2 participants
Summary
Test plan
bun install --frozen-lockfilebun run checkbun run buildbun run testbun run lint:pack:allbun run test:sei-global-wallet-releaseSecurity waiver
npm consumers use scoped patched transitive overrides and audit clean. Bun consumers preserve compatible BN/ws majors and explicitly accept only
GHSA-378v-28hj-76wf,GHSA-58qx-3vcg-4xpx, andGHSA-96hv-2xvq-fx4pfor the optional AA path until upstream pins are corrected.Made with Cursor