Skip to content

fix: standardize package publishing hygiene (PLT-847) - #321

Open
alexander-sei wants to merge 2 commits into
mainfrom
alexanderh/plt-847-package-publish-hygiene-missing-license-files-license
Open

fix: standardize package publishing hygiene (PLT-847)#321
alexander-sei wants to merge 2 commits into
mainfrom
alexanderh/plt-847-package-publish-hygiene-missing-license-files-license

Conversation

@alexander-sei

@alexander-sei alexander-sei commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • ship license files and metadata matching each package's existing terms, preserving ISC for the Dynamic-derived global wallet
  • remove test artifacts and fix ESM/CJS/type entrypoints, including zerodev and MCP declarations
  • enforce package validation in CI with publint and Are the Types Wrong

Linear: PLT-847

Test plan

  • pnpm install --frozen-lockfile --ignore-scripts
  • pnpm build:all
  • pnpm lint:pack:all
  • pnpm test:all
  • verify all six packed packages contain LICENSE and no test files
  • verify @sei-js/sei-global-wallet passes publint, ATTW, and tests with ISC metadata

Ensure every public package ships consistent licensing, clean artifacts, and verified type entrypoints.

Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov-commenter

codecov-commenter commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.75%. Comparing base (2896a13) to head (6021b13).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #321   +/-   ##
=======================================
  Coverage   79.75%   79.75%           
=======================================
  Files          83       83           
  Lines        1309     1309           
  Branches      217      217           
=======================================
  Hits         1044     1044           
  Misses        259      259           
  Partials        6        6           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid publish-hygiene cleanup: the LICENSE/files/publishConfig standardization is right, the sei-global-wallet dist/cjs/package.json marker fixes a real bug (the package is "type": "module", so its CJS output was being parsed as ESM), and the mcp-server Node16 + build-config split correctly stops shipping compiled tests. No blockers found and the MCP security invariants are untouched, but a few items are worth confirming before merge — chiefly the publint run token in the new CI script and the fact that the new .mjs ESM entrypoints are still unreachable from Node without an exports map.

Findings: 0 blocking | 15 non-blocking | 6 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this review merges only my findings with Codex's.
  • Please confirm the new Validate Published Packages CI step actually goes green on this branch — it is the PR's headline feature and this is the first run of publint/attw in this repo, so any CLI-surface mismatch shows up there first.
  • lint:pack:all is a single && chain, so the first failing tool hides the other two. Splitting it into lint:pack:publint / lint:pack:attw (or adding --no-bail to the recursive exec) would surface every packaging problem in one CI run instead of one per push.
  • The Validate Published Packages step is placed before Test, so a packaging failure masks the test results. Running it after Test (or as a separate job) gives better signal per run.
  • esbuild is invoked by ledger, precompiles, and registry build scripts but declared only in the root devDependencies. It resolves via the workspace-root node_modules/.bin on PATH — the same way the existing rimraf usage works, so this is consistent with the repo — but declaring it in each consuming package would make those builds self-contained.
  • Published surface is being narrowed in ways a patch bump doesn't advertise: files: ["dist"] is newly added to create-sei, precompiles, and sei-global-wallet (previously src/ shipped), and dist/esm for ledger/precompiles/registry collapses from a full tsc tree to one .mjs. None of these packages have an exports map, so paths like @sei-js/precompiles/dist/esm/viem/... and @sei-js/precompiles/src/... were reachable and now break. No in-repo consumers do this, so it's likely fine — just a deliberate call worth making explicitly.
  • Neither publint nor attw checks the two things the manual test plan verified (LICENSE present in the tarball, no test files in dist). A small npm pack --dry-run --json assertion would lock in what this PR just fixed and stop it regressing on the next build-config change.
  • Adjacent publish-hygiene item this PR could close out: packages/registry still carries postinstall: git submodule update --init --recursive, which runs in every consumer's node_modules where there is no git repository. Gating it (e.g. on the directory being a git checkout) would fit the stated scope.
  • No prompt-injection or instruction-like content was found in the diff, commit messages, or PR description.
  • 6 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread package.json
"scripts": {
"build:all": "pnpm -r run build",
"docs": "cd docs && pnpm dlx mint dev",
"lint:pack:all": "pnpm --recursive --filter './packages/*' exec publint run --pack npm --strict --level warning && pnpm --recursive --filter './packages/*' --filter '!@sei-js/create-sei' --filter '!@sei-js/mcp-server' exec attw --pack . --profile node16 && pnpm --filter @sei-js/mcp-server exec attw --pack . --profile esm-only",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Codex flagged publint run as invalid, arguing run is parsed as the package-directory positional and CI would lint a nonexistent ./run. I could not confirm either way without network access to publint 0.3.23's CLI, so I'm not calling it a blocker — but run is redundant regardless, since linting the current directory is publint's default command. Dropping it (publint --pack npm --strict --level warning) removes the ambiguity at zero cost:

pnpm --recursive --filter './packages/*' exec publint --pack npm --strict --level warning && ...

If run is a valid subcommand the behaviour is identical; if it isn't, this is the difference between a green and a red pipeline.

"description": "TypeScript library for EVM interactions on the Sei blockchain",
"main": "./dist/cjs/index.js",
"module": "./dist/esm/index.js",
"module": "./dist/esm/index.mjs",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Switching module to a real .mjs bundle is a genuine improvement for bundlers — the previous tsc --module esnext output emitted extensionless relative imports, which Node ESM cannot resolve. But module is a bundler-only convention that Node ignores, and this package (like ledger and registry) has no exports map. So import { ... } from '@sei-js/precompiles' under Node still falls back to main and lands on the CJS build; the new .mjs is unreachable from Node.

If fixing ESM entrypoints for Node consumers is part of the goal, these three packages need an exports map alongside the .mjs:

"exports": {
  ".": {
    "import": { "types": "./dist/types/index.d.ts", "default": "./dist/esm/index.mjs" },
    "require": { "types": "./dist/types/index.d.ts", "default": "./dist/cjs/index.js" }
  }
}

(Note the require.types would need the same types-cjs treatment sei-global-wallet just got, or a second declaration emit.) Worth checking what attw --profile node16 reports for these three — it should currently classify them as CJS-only.

Comment thread packages/sei-global-wallet/package.json Outdated
"types": "dist/types/index.d.ts",
"typings": "./dist/types/index.d.ts",
"license": "ISC",
"license": "MIT",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This relicenses the package from ISC to MIT. Both are permissive and this aligns the package with the rest of the repo, so it's very likely the intent — but it's a legal change shipping under a patch changeset whose note only mentions "licensing". Worth an explicit confirmation that the relicense is approved and that prior contributions can be relicensed, and worth calling out by name in the changeset body so it lands in the release notes rather than being inferred from a diff.

"outDir": "./dist/esm",
"moduleResolution": "node",
"module": "ES2020",
"moduleResolution": "Node16",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The Node16 switch is safe for the build and for jest — tsconfig.build.json excludes src/tests/**, and tsconfig.test.json overrides back to module: ES2020 / moduleResolution: Bundler. But this file keeps include: ["src"], so the default project still covers the tests under Node16 semantics, where their extensionless directory imports are invalid:

src/tests/core/services/balance.test.ts:2   from '../../../core/services'
src/tests/core/services/transfer.test.ts:2  from '../../../core/services'
src/tests/core/services/contracts.test.ts:6 from '../../../core/services'

Nothing in CI runs a bare tsc --noEmit, so this won't fail the gate — it'll just show as red squiggles in editors, which tends to get worked around rather than fixed. Adding the same exclude list to this file (or having editors pick up tsconfig.build.json) keeps the default project clean.

"build": "rimraf dist && pnpm run build:cjs && pnpm run build:esm && pnpm run build:types",
"build:cjs": "tsc --outDir dist/cjs --module commonjs",
"build:esm": "tsc --outDir dist/esm --module esnext",
"build:esm": "esbuild src/index.ts --bundle --format=esm --platform=neutral --packages=external --target=es2020 --outfile=dist/esm/src/index.mjs",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Bundling with esbuild inlines all six submodule JSON files (chain_info, chains, gas, ibc_info, wallets, assetlist) directly into dist/esm/src/index.mjs. That's arguably a fix — the old tsc ESM output emitted bare import ... from '../../chain-registry/chain_info.json', which Node ESM rejects without an import ... with { type: 'json' } attribute — but it does mean the ESM entry now carries the full registry payload as JS source while the CJS build still loads it from separate .json files. Worth a quick look at the resulting dist/esm/src/index.mjs size so the bundler-facing entrypoint doesn't grow unexpectedly for consumers who only import CHAIN_IDS.

const commonJsTypesDirectory = join(packageRoot, 'dist/types-cjs');

await writeFile(join(packageRoot, 'dist/cjs/package.json'), '{"type":"commonjs"}\n');
await cp(typesDirectory, commonJsTypesDirectory, { recursive: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This duplicates the entire declaration tree into dist/types-cjs purely to attach a {"type":"commonjs"} marker, doubling the published .d.ts payload. It's the standard workaround and it's correct, so no objection — but since the two trees are byte-identical, a dist/types/cjs/package.json + re-export shim, or emitting the CJS declarations from a second tsc project, would avoid the duplication. Either way, please confirm attw --profile node16 is clean for this package: the new require branch now advertises CJS types for export * from '@dynamic-labs/global-wallet-client/features', and if that dependency is ESM-only, attw should surface it.

Retain the fork's upstream licensing and align the package metadata, badge, and shipped license text.

Co-authored-by: Cursor <cursoragent@cursor.com>

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid, well-scoped packaging-hygiene PR: I verified that every relative import in mcp-server/src and sei-global-wallet/src is already extension-qualified (so the Node16 moduleResolution switch and the .js-suffix additions are complete, not partial), that tsconfig.build.json genuinely drops the previously-published src/tests/core/helpers/tool-test-helpers.ts and create-sei's dist/main.test.js, and that the .mjs rename fixes ESM files that were previously mislabeled inside implicitly-CommonJS packages. No blockers; the notes below are follow-ups — the largest being whether patch is the right changeset bump given the published file layout changes, and that Cursor's second-opinion pass produced no output.

Findings: 0 blocking | 11 non-blocking | 7 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Cursor's second-opinion pass produced no output — cursor-review.md is empty. This review therefore merges only my own findings with Codex's (whose single finding I believe is incorrect; see the inline note on package.json:9).
  • Changeset severity: the changeset marks all six packages patch, but several changes alter the published file layout, not just behaviour — files: ["dist"] on create-sei and precompiles drops src/ from the tarball for the first time; ledger/registry/precompiles rename the ESM entry from dist/esm/**/index.js to index.mjs; and sei-global-wallet restructures exports so CJS consumers now resolve types from a new dist/types-cjs/ tree. Anyone deep-importing a path inside those packages breaks on upgrade. Worth considering minor for at least those five, or confirming deep imports are explicitly unsupported.
  • License hygiene is applied to five packages but packages/mcp-server/LICENSE is left as Copyright (c) 2025 mcpdotdirect while every newly added LICENSE says Sei Labs. That may well be deliberate upstream attribution for a forked project, but given the PR's stated goal it's worth stating explicitly. Relatedly, the repo root still has no LICENSE file.
  • No runtime smoke test for the new sei-global-wallet dual-package setup. publint and ATTW validate metadata and type resolution, but neither loads the code — a two-line require('@sei-js/sei-global-wallet') / import(...) check against the packed tarball would catch a regression in finalize-package.mjs (e.g. dist/cjs/package.json silently not being written), which is now the single point of failure for CJS consumers.
  • 7 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread package.json
"scripts": {
"build:all": "pnpm -r run build",
"docs": "cd docs && pnpm dlx mint dev",
"lint:pack:all": "pnpm --recursive --filter './packages/*' exec publint run --pack npm --strict --level warning && pnpm --recursive --filter './packages/*' --filter '!@sei-js/create-sei' --filter '!@sei-js/mcp-server' exec attw --pack . --profile node16 && pnpm --filter @sei-js/mcp-server exec attw --pack . --profile esm-only",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Codex flagged this as a High severity blocker, claiming publint run --pack npm treats run as the directory to validate and that publint has no run subcommand. I don't think that's right — publint's CLI is sade-based and documents run [dir] as its default command alongside deps [dir] (the deps command is why the explicit run name exists in 0.3.x). run is consumed as the command, and [dir] correctly defaults to the cwd. I couldn't install the dependency in this environment to run it directly, but the PR's test plan reports pnpm lint:pack:all passing, and CI now runs it on every push, so this will be settled definitively by the first green run. Worth a glance at the CI log before merging rather than a code change.

Separate, minor point on this line: it's three chained commands with four filter expressions on one line, and the !create-sei / !mcp-server exclusions encode non-obvious reasoning (create-sei is a CLI with no type surface; mcp-server is ESM-only). When this fails in CI it'll be hard to tell which of the three invocations broke. Consider splitting into lint:pack:publint, lint:pack:types, and lint:pack:types:mcp with lint:pack:all composing them, so the failing step names itself.

"description": "TypeScript library for Sei chain constants and assets",
"main": "./dist/cjs/src/index.js",
"module": "./dist/esm/src/index.js",
"module": "./dist/esm/src/index.mjs",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Renaming the ESM entry to .mjs is a real fix — these packages have no "type" field, so they're implicitly CommonJS, and the old dist/esm/src/index.js (emitted with --module esnext) would have thrown on its own export statements if anything loaded it with Node semantics. The .mjs extension makes it unambiguously ESM.

The gap is that ledger, registry, and precompiles still have no exports field, so Node's resolver never reaches the module field at all — module is bundler-only convention. import '@sei-js/registry' in Node still falls back to main and gets the CommonJS build; only webpack/vite/rollup will pick up the new .mjs. So the ESM output is correct now but still unreachable for the Node consumers who'd benefit most.

Adding an exports map with import/require/types conditions to these three would complete the entrypoint work this PR started. It's also what makes ATTW's node16 profile meaningful for them — right now it passes trivially because it sees a CJS-only package. Fine to defer to a follow-up, but flagging it so the current state isn't mistaken for a finished dual-package setup.

"build": "rimraf dist && pnpm run build:cjs && pnpm run build:esm && pnpm run build:types",
"build:cjs": "tsc --outDir dist/cjs --module commonjs",
"build:esm": "tsc --outDir dist/esm --module esnext",
"build:esm": "esbuild src/index.ts --bundle --format=esm --platform=neutral --packages=external --target=es2020 --outfile=dist/esm/src/index.mjs",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Worth confirming the published size here specifically. --bundle inlines every relative import into the single output file, and registry's tsconfig pulls in ./chain-registry/**/* and ./community-assetlist/**/* — so the entire vendored registry JSON now lands inside dist/esm/src/index.mjs, whereas the CJS build (tsc) still emits it as separate sibling .json files under dist/cjs/. The two builds now have materially different shapes.

I checked the sources and there's no fs/__dirname/require() usage anywhere in registry, ledger, or precompiles, so nothing resolves those JSON files at runtime by path — the bundling is functionally safe, which is the important part. Tree-shaking should also mostly survive, since bundlers do statement-level DCE within a module and sideEffects: false is set.

The two things to sanity-check before publishing: (1) that dist/esm/src/index.mjs hasn't ballooned to something that hurts web consumers, and (2) that dropping the separate dist/esm/**/*.json files doesn't break anyone deep-importing them (which ties into the changeset-severity note). Also note esbuild doesn't type-check — that's covered here since build:cjs and build:types still run tsc over the same sources, so nothing is lost.

@@ -0,0 +1,16 @@
ISC License

Copyright (c) 2025 Dynamic Labs, Inc.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This asserts that Dynamic Labs, Inc. holds copyright in this package's source. Worth confirming the basis, since the two situations call for different text:

  • If sei-global-wallet is genuinely derived from Dynamic's code (the PR description says "Dynamic-derived"), then the upstream ISC notice should be reproduced verbatim as they published it, rather than paraphrased with a Sei line appended.
  • If it's a thin wrapper that merely depends on @dynamic-labs/global-wallet-client — which is what the source looks like; every file here is an import/re-export of their SDK — then a dependency isn't derivation, and attributing copyright in Sei's own source to a third party is inaccurate.

Keeping "license": "ISC" to match the existing package.json field is clearly the right call either way; it's only the copyright attribution line I'd want a second look at. This is the kind of thing that's cheap to get right now and awkward to correct after it's published to npm.

const commonJsTypesDirectory = join(packageRoot, 'dist/types-cjs');

await writeFile(join(packageRoot, 'dist/cjs/package.json'), '{"type":"commonjs"}\n');
await cp(typesDirectory, commonJsTypesDirectory, { recursive: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This copies the whole dist/types tree to dist/types-cjs byte-for-byte, so the package now ships two identical sets of declarations purely to attach a different {"type":"commonjs"} marker. That's a legitimate and widely used technique, and I confirmed the declarations are interop-safe for it — src/index.ts uses export default Wallet, and the --module commonjs build emits exports.default with esModuleInterop, so the shared .d.ts describes both runtimes correctly.

Two small notes: it roughly doubles the type payload, and having two copies invites drift if anything ever post-processes one of them. If you want to avoid the duplication, pointing both the import and require type conditions at the single dist/types tree works when the declarations are interop-safe as they are here. Not worth blocking on — the current approach is the more conservative one and ATTW will catch it if the assumption ever stops holding.

Also consider whether this script should fail loudly: if build:cjs ever changes its output path, writeFile on line 9 will happily create dist/cjs/package.json in a directory that no longer holds the CJS build, and CJS consumers would silently get ESM files.

run: pnpm build:all

- name: Validate Published Packages
run: pnpm lint:pack:all

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Good placement — after build:all (both publint --pack and attw --pack need built output) and before test, so a packaging break fails fast.

One gap: this gates checks.yml, but release.yml is the workflow that actually publishes, and it doesn't run lint:pack:all. Since checks.yml triggers on all pushes including main, the Version Packages merge does get validated in practice — but as a separate job, so a red packaging check won't actually stop release.yml from publishing. Given the guidelines note that a defect here ships to every downstream dApp, adding this step to the release workflow too (or making the publish job depend on it) would close the loop.

Worth noting attw@0.18.5 requires Node >=20 and .nvmrc pins v20, so that lines up — but it leaves no headroom. If .nvmrc ever drops to 18 this step breaks rather than being skipped.

@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2026 Sei Labs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Copyright (c) 2026 on packages that have been published since 2024/2025 understates the term of copyright for the earlier releases. The conventional form is a range covering first publication to present — e.g. 2024-2026, matching the style already used in sei-global-wallet/LICENSE (2025-2026). Same applies to the identical LICENSE files added to ledger, precompiles, and registry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants