-
Notifications
You must be signed in to change notification settings - Fork 51
fix(sei-global-wallet): address seidroid review follow-ups #344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ describe('browser global shim with nothing defined', () => { | |
|
|
||
| try { | ||
| await import('../browserGlobal.js'); | ||
| const processModule = await import('process/browser.js'); | ||
| const processSingleton = processModule.default; | ||
|
|
||
| const runtime = globalThis as typeof globalThis & { | ||
| global?: unknown; | ||
|
|
@@ -23,6 +25,10 @@ describe('browser global shim with nothing defined', () => { | |
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] This duplicates line 27 — |
||
| expect(processSingleton.env.NODE_ENV).not.toBe('production'); | ||
| expect(globalThis.process).not.toBe(processSingleton); | ||
| expect(globalThis.process.env).not.toBe(processSingleton.env); | ||
| } finally { | ||
| if (originalGlobal) Object.defineProperty(globalThis, 'global', originalGlobal); | ||
| if (originalProcess) Object.defineProperty(globalThis, 'process', originalProcess); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { afterAll, beforeAll, describe, expect, it, jest } from 'bun:test'; | ||
| import { createSolanaWallet, registerWallet } from '../dynamicSolana'; | ||
| import { registerSolanaStandard } from '../registerSolanaStandard'; | ||
|
|
||
| jest.mock('../dynamicSolana', () => ({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Bun does not hoist Two consequences worth weighing:
A |
||
| createSolanaWallet: jest.fn(), | ||
| registerWallet: jest.fn() | ||
| })); | ||
|
|
||
| jest.mock('../wallet', () => ({})); | ||
| jest.mock('../config', () => ({ | ||
| config: { | ||
| walletIcon: 'test-icon', | ||
| walletName: 'SEI Wallet' | ||
| } | ||
| })); | ||
|
|
||
| describe('registerSolanaStandard during SSR', () => { | ||
| const originalWindow = Object.getOwnPropertyDescriptor(globalThis, 'window'); | ||
|
|
||
| beforeAll(() => { | ||
| Reflect.deleteProperty(globalThis, 'window'); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| if (originalWindow) Object.defineProperty(globalThis, 'window', originalWindow); | ||
| else Reflect.deleteProperty(globalThis, 'window'); | ||
| }); | ||
|
|
||
| it('returns undefined without creating or registering a wallet', () => { | ||
| expect(registerSolanaStandard()).toBeUndefined(); | ||
| expect(createSolanaWallet).not.toHaveBeenCalled(); | ||
| expect(registerWallet).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,10 @@ if (typeof runtime.process === 'undefined') { | |
| // `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'; | ||
| install('process', processShim); | ||
| const processForGlobal = { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Copying the shim fixes the singleton mutation, but it also gives up the A secondary effect: the bundle now holds two distinct 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. |
||
| ...processShim, | ||
| env: { ...processShim.env } | ||
| }; | ||
| processForGlobal.env.NODE_ENV ??= 'production'; | ||
| install('process', processForGlobal); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,13 @@ const assertSatisfiesDynamicRange = (version: string | undefined, label: string) | |
| return version; | ||
| }; | ||
|
|
||
| const assertMajor = (version: string | undefined, major: string, label: string) => { | ||
| assert(version, `${label} was not installed`); | ||
| assert(version.startsWith(`${major}.`), `${label} resolved ${version}, expected ${major}.x`); | ||
| }; | ||
|
|
||
| const ghsaIdsIn = (value: unknown) => JSON.stringify(value).match(/GHSA-[a-z0-9-]+/gi) ?? []; | ||
|
|
||
| const reportWaiverProgress = (message: string) => { | ||
| console.warn(`[waiver] ${message}`); | ||
| }; | ||
|
|
@@ -576,28 +583,39 @@ const assertAcceptedBunAudit = (result: ProcessResult) => { | |
| } | ||
|
|
||
| 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) ?? []); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] This new hard fail runs before the waiver subset check, so any Bun audit finding whose serialized form lacks a 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 Failing closed on a genuinely un-waivable advisory is a defensible intent, but consider narrowing it to entries that are actually finding arrays ( |
||
|
|
||
| const reported = new Set(auditFindings.flatMap((finding) => ghsaIdsIn(finding)).map((advisory) => advisory.toLowerCase())); | ||
| const accepted = new Set(acceptedBunAdvisories.map((advisory) => advisory.toLowerCase())); | ||
|
|
||
| // A subset check, not an exact set: the advisory database changes on its own | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] |
||
| assert.deepEqual( | ||
| unwaived, | ||
| [], | ||
| `Bun AA consumer reported advisories outside the accepted waiver: ${unwaived.join(', ')}. Assess them and update packages/sei-global-wallet/README.md before releasing.` | ||
| ); | ||
|
|
||
| const fixed = acceptedBunAdvisories.filter((advisory) => !reported.has(advisory)); | ||
| const fixed = acceptedBunAdvisories.filter((advisory) => !reported.has(advisory.toLowerCase())); | ||
| if (fixed.length > 0) { | ||
| reportWaiverProgress( | ||
| `Bun no longer reports ${fixed.join(', ')}. Narrow the waiver in packages/sei-global-wallet/README.md and acceptedBunAdvisories in this script.` | ||
| ); | ||
| } | ||
|
|
||
| // The documented Axios and UUID overrides must still be taking effect. | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Scoping to exact key names is the right fix for the |
||
| assert.deepEqual( | ||
| blockedOverridePackages, | ||
| [], | ||
| `Bun AA consumer still reports ${blockedOverridePackages.join(', ')}; the documented Axios and UUID overrides are not taking effect.` | ||
| ); | ||
| console.log( | ||
| `Bun AA consumer advisories, all within the waiver: ${Object.entries(report) | ||
| .map(([name, findings]) => `${name} (${findings.map(({ severity }) => severity).join(', ')})`) | ||
|
|
@@ -747,11 +765,11 @@ try { | |
| 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'); | ||
| assertMajor(npmLock.packages['node_modules/bn.js']?.version, '5', 'hoisted bn.js'); | ||
| assert.equal(npmLock.packages['node_modules/ws']?.version, '8.21.0'); | ||
| assert.equal(npmLock.packages['node_modules/viem']?.dependencies?.ws, '8.18.3'); | ||
| assert.equal(npmLock.packages['node_modules/jayson']?.dependencies?.ws, '^7.5.10'); | ||
| assert.equal(npmLock.packages['node_modules/jayson/node_modules/ws']?.version, '7.5.13'); | ||
| assertMajor(npmLock.packages['node_modules/jayson/node_modules/ws']?.version, '7', 'jayson nested ws'); | ||
| await run(['node', 'check-ssr.mjs'], npmConsumerDir); | ||
| await run(['node', 'check-edge-native.mjs'], npmConsumerDir); | ||
| await run(['node', 'check-local-aa.mjs'], npmConsumerDir); | ||
|
|
@@ -810,11 +828,11 @@ try { | |
| await run(['bun', 'install'], bunConsumerDir); | ||
| const bunLock = await readFile(join(bunConsumerDir, 'bun.lock'), 'utf8'); | ||
| assertBunDynamicGraph(bunLock); | ||
| assert.match(bunLock, /"bn\.js": \["bn\.js@5\.2\.5"/); | ||
| assert.match(bunLock, /"ethjs-unit\/bn\.js": \["bn\.js@4\.11\.6"/); | ||
| assert.match(bunLock, /"number-to-bn\/bn\.js": \["bn\.js@4\.11\.6"/); | ||
| assert.match(bunLock, /"jayson\/ws": \["ws@7\.5\.13"/); | ||
| assert.match(bunLock, /"ws": \["ws@8\.18\.3"/); | ||
| assert.match(bunLock, /"bn\.js": \["bn\.js@5\./); | ||
| assert.match(bunLock, /"ethjs-unit\/bn\.js": \["bn\.js@4\./); | ||
| assert.match(bunLock, /"number-to-bn\/bn\.js": \["bn\.js@4\./); | ||
| assert.match(bunLock, /"jayson\/ws": \["ws@7\./); | ||
| assert.match(bunLock, /"ws": \["ws@8\./); | ||
| await run(['bun', 'check-ssr.mjs'], bunConsumerDir); | ||
| await run(['bun', 'check-local-aa.mjs'], bunConsumerDir); | ||
| assertAcceptedBunAudit(await run(['bun', 'audit', '--json'], bunConsumerDir, true)); | ||
|
|
@@ -823,7 +841,7 @@ try { | |
| console.log( | ||
| fastCheck | ||
| ? 'Sei Global Wallet fast npm consumer checks passed.' | ||
| : `Sei Global Wallet consumer checks passed: npm scoped patched bn.js/ws8 while preserving Solana bn5/Jayson ws7 with a clean audit; Bun preserved compatible majors and accepted exactly ${acceptedBunAdvisories.join(', ')}.` | ||
| : 'Sei Global Wallet consumer checks passed: npm scoped patched bn.js/ws8 while preserving Solana bn5/Jayson ws7 with a clean audit; Bun preserved compatible majors within the accepted advisory waiver.' | ||
| ); | ||
| } finally { | ||
| await rm(temporaryRoot, { force: true, recursive: true }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Good caveat to document. It applies equally to the
@wallet-standard/walletbullet directly above: under pnpm's isolatednode_modulesor 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 toevents— as written it reads as if the Solana path is unaffected.