feat: v1.0.0 revamp Turborepo, Bun bundler, unified output, CI automation#65
Merged
Merged
Conversation
Migrates from Lerna v6 to Turborepo v2 for faster, cached task orchestration while keeping Yarn workspaces for dependency management. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces per-package tsc builds with a shared Bun build script that produces dist/esm, dist/cjs, and dist/types for all packages. Adds proper exports maps to each package.json for dual CJS/ESM resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds simple-import-sort and unused-imports plugins to ESLint config. Standardizes lint scripts across all packages and updates root scripts for format checking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Changes npx jest to yarn jest across all packages to fix module resolution in Yarn workspaces. Skips evidence.test.ts since the evidencePrecompile module was never implemented. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces 5 separate publish-*.yml workflows with a single publish.yml that builds, tests, and publishes all packages on tag push. Updates test and lint workflows to use Bun for building. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThe pull request implements a comprehensive v1.0.0 restructuring of the monorepo. Key changes include: replacing Lerna with Turbo for orchestration, introducing Bun as the bundler via a new shared build script, consolidating five separate publish workflows into a single unified publish workflow, updating all package.json files with proper ESM/CJS exports and file whitelists, adding ESLint plugins for import sorting and unused import detection, updating CI/CD workflows to use immutable installs and Bun setup, and deleting the Lerna configuration. Numerous import reordering and spacing adjustments are applied across EVM precompile files for consistency. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/utils/package.json (1)
64-68:⚠️ Potential issue | 🟡 MinorRemove
npmandts-nodefromdependenciesin packages/utils/package.json.
npmshould never be a runtime dependency—it's the package manager itself.ts-nodeis a development tool and has no purpose in production code. These appear to be in the wrong section. Movets-nodetodevDependenciesif needed for local development, or remove both entirely since they're not used in the package's build scripts, source code, or tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/package.json` around lines 64 - 68, Remove "npm" and "ts-node" from the "dependencies" object in package.json for the packages/utils package; delete both entries and, if "ts-node" is needed for local development only, add it to "devDependencies" instead (do not add "npm" anywhere), ensuring only runtime deps like "@kiichain/kiijs-proto" remain in "dependencies".packages/rwa/package.json (1)
33-37:⚠️ Potential issue | 🟠 Major
publishConfig.directoryconfiguration breaks package resolution.The
"directory": "dist"setting tells npm to publish thedist/directory's contents as the package root. However, theexportspaths reference./dist/esm/index.js, which would resolve todist/dist/esm/index.jsafter publishing—a path that doesn't exist.This issue is not unique to this package; identical misconfigurations exist in
packages/proto/package.jsonandpackages/lcd/package.json.Recommended fix: Remove
"directory": "dist"to publish from the package root (aligns withpackages/utils/andpackages/evm/patterns).Suggested fix
"publishConfig": { "access": "public", - "registry": "https://registry.npmjs.org", - "directory": "dist" + "registry": "https://registry.npmjs.org" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rwa/package.json` around lines 33 - 37, Remove the incorrect publishConfig.directory entry that points to "dist" so npm will publish from the package root; specifically edit the publishConfig block in packages/rwa/package.json to delete the "directory": "dist" key (and apply the same fix to packages/proto/package.json and packages/lcd/package.json) so the package exports (e.g., paths referencing ./dist/esm/index.js) resolve correctly after publishing.
🧹 Nitpick comments (10)
packages/rwa/tsconfig.json (1)
9-11: Stale comment: output is for type declarations, not JavaScript.The comment says "Output directory for compiled JavaScript" but with Bun handling JS bundling to
dist/esmanddist/cjs, thisoutDiris only used for type declarations. Consider updating the comment:📝 Suggested comment fix
"declaration": true, // Generate declaration files - "outDir": "./dist/types", // Output directory for compiled JavaScript + "outDir": "./dist/types", // Output directory for declaration files "rootDir": "./src" // Root directory of TypeScript source files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rwa/tsconfig.json` around lines 9 - 11, Update the stale comment next to the tsconfig.json "outDir" setting to accurately reflect its purpose for emitting TypeScript declaration files rather than compiled JavaScript; locate the "declaration" and "outDir" entries in tsconfig.json and change the comment "Output directory for compiled JavaScript" to something like "Output directory for generated .d.ts type declarations (JS bundling handled by Bun to dist/esm and dist/cjs)" so it correctly documents the role of outDir and its relationship to declaration: true.scripts/build.ts (2)
36-55: Consider adding error handling for build commands.If
bun buildortscfails,execSyncthrows an error that may not provide clear context. Wrapping in try-catch with explicit error messages would improve debugging:🛡️ Proposed error handling
+function runCommand(command: string, label: string) { + try { + execSync(command, { cwd, stdio: 'inherit' }); + } catch (error) { + console.error(`Failed during: ${label}`); + process.exit(1); + } +} // Build ESM with Bun console.log(' → ESM bundle'); -execSync( +runCommand( `bun build ./src/index.ts --outdir ./dist/esm --target=node --format=esm --splitting --sourcemap=external ${externalFlags}`, - { cwd, stdio: 'inherit' }, + 'ESM bundle', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.ts` around lines 36 - 55, Wrap each execSync invocation in try-catch blocks around the three build steps (the ESM/CJS/Type declarations sections that log "→ ESM bundle", "→ CJS bundle", and "→ Type declarations") so failures are caught; on catch, call console.error (or processLogger if available) with a clear message including which step failed (e.g., "Failed to build ESM bundle"), include the caught error details (error.message and any error.output/stderr if present), and then exit with a non‑zero status (process.exit(1)) or rethrow to ensure the script fails deterministically.
12-14: Usingrequire()in an ESM context.The root
package.jsonhas"type": "module", making this file ESM. While Bun supportsrequire()in ESM, this is non-standard. Consider using ESM-compatible imports for broader compatibility:♻️ ESM-compatible alternative
-const cwd = process.cwd(); -const pkg = require(resolve(cwd, 'package.json')); -const entrypoint = resolve(cwd, 'src/index.ts'); +import { readFileSync } from 'fs'; + +const cwd = process.cwd(); +const pkg = JSON.parse(readFileSync(resolve(cwd, 'package.json'), 'utf-8')); +const entrypoint = resolve(cwd, 'src/index.ts');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.ts` around lines 12 - 14, The file is ESM but uses require(resolve(...)) to load package.json (pkg), which is non-standard; replace that with an ESM-compatible approach such as using top-level await import for JSON modules (e.g. await import(resolve(cwd, 'package.json'), { assert: { type: 'json' } }) and assign to pkg) or read and parse the file with fs.readFileSync/JSON.parse(resolve(cwd, 'package.json')) instead; update the code that references pkg, keep cwd and entrypoint as-is, and remove the require(...) call so the script no longer relies on CommonJS require in an ESM module.eslint.config.js (2)
69-70: Note:scripts/*is excluded from linting.The new
scripts/build.tswon't be linted. If you want the build script to follow the same code quality standards, consider removing this ignore pattern or adding a separate config for scripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 69 - 70, The ESLint ignore pattern 'scripts/*' currently excludes your new build script from linting; remove the 'scripts/*' entry from the ignore array (or replace it with a narrower pattern) in eslint.config.js or add a separate ESLint config for scripts so 'scripts/build.ts' is included and follows project lint rules; search for the ignore array containing 'scripts/*' and update it or add an overrides block targeting 'scripts/*.ts' to enable linting for that file.
30-43: Consider the implications of disabling TypeScript safety rules.Disabling
@typescript-eslint/no-unsafe-assignmentand@typescript-eslint/no-unsafe-callreduces type safety checks. If this is intentional to accommodate existing code patterns, consider adding a comment explaining the rationale.The unused imports and import sorting rules are well-configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 30 - 43, The config currently turns off TypeScript safety rules '@typescript-eslint/no-unsafe-assignment' and '@typescript-eslint/no-unsafe-call' without explanation; add a brief comment above those rule entries explaining the intentional rationale (e.g., legacy JS interop, staged migration to stricter checks) or, better, replace the blanket 'off' with a targeted override or gradual strategy (enable in new/TS files or use eslint overrides for specific directories/files) so the intent is documented and maintainers know why these rules are relaxed.package.json (1)
64-64: Consider pinning Turbo to a more specific version range.
"turbo": "^2"allows any 2.x version. For reproducible builds, consider a narrower range like"^2.0.0"or the specific version tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 64, Update the turbo dependency version string in package.json to a narrower pinned range (e.g., replace "turbo": "^2" with a specific tested version or a tighter range such as "turbo": "^2.0.0" or "turbo": "2.12.0") so that builds are reproducible; locate the "turbo" entry in package.json and modify its value to the chosen exact or constrained version and then run your package manager (npm/yarn/pnpm) to refresh lockfile.packages/evm/tests/evidence.test.ts (1)
4-5: Orphaned ESLint disable comment and unused timeout.The
@typescript-eslint/no-require-importsdisable comment has no correspondingrequire()call in this file (likely leftover from removed imports). Additionally,jest.setTimeout(60_000)is unused since the tests are skipped and contain only synchronous assertions.Consider removing both to reduce noise.
Suggested cleanup
// Skipped: evidencePrecompile module has not been implemented yet. // Re-enable this test suite once src/ethers/evidencePrecompile.ts exists. -// eslint-disable-next-line `@typescript-eslint/no-require-imports` -jest.setTimeout(60_000); - describe.skip('Evidence Precompile Tests', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evm/tests/evidence.test.ts` around lines 4 - 5, Remove the orphaned ESLint disable comment and the unused jest.setTimeout call: delete the leading "// eslint-disable-next-line `@typescript-eslint/no-require-imports`" comment and the "jest.setTimeout(60_000);" statement from the test file (referencing the jest.setTimeout usage) since there is no require() and the tests are skipped/synchronous, so both are dead/noise.packages/evm/package.json (1)
55-56: Consider whetherdotenvshould be a peerDependency.
dotenvis typically adevDependencysince it's used for local development/testing, not at runtime by library consumers. Requiring consumers to install it as a peer may be unnecessary unless the library's public API depends on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evm/package.json` around lines 55 - 56, The package currently lists "dotenv" under peerDependencies; change this so consumers aren't forced to install it unless the library actually requires dotenv at runtime. If dotenv is only used for local development or tests, move "dotenv" from peerDependencies to devDependencies in packages/evm/package.json (update the "peerDependencies" block to remove "dotenv" and add it under "devDependencies"); if the library truly relies on dotenv at runtime, instead document that requirement and keep it as a regular dependency in "dependencies" rather than a peerDependency..github/workflows/publish.yml (1)
47-70: Sequential publish steps with no error isolation.If any package fails to publish (e.g., version already exists, network issue), subsequent packages won't be published. This may be intentional for atomicity, but consider:
- Adding
continue-on-error: trueif partial publishes are acceptable- Adding a pre-check step to verify versions don't exist on npm before attempting publish
Also, the publish order (proto → lcd → utils → evm → rwa) appears correct based on likely dependency relationships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 47 - 70, The publish steps for the workflow (steps named "Publish `@kiichain/kiijs-proto`", "Publish `@kiichain/kiijs-lcd`", "Publish `@kiichain/kiijs-utils`", "Publish `@kiichain/kiijs-evm`", "Publish `@kiichain/kiijs-rwa`") run sequentially and will abort the remaining publishes if one fails; update the workflow to either add continue-on-error: true to the individual publish steps if partial publishes are acceptable, or insert a pre-check step (e.g., a script that queries npm for the package@version for each package under packages/proto, packages/lcd, packages/utils, packages/evm, packages/rwa) and bail with a clear error before attempting publishes so failures are isolated and reported cleanly..github/workflows/test.yml (1)
30-31: Redundant build step—Turbo already handles the dependency.Per
turbo.json,test:cideclares"dependsOn": ["build"], so Turbo will automatically run the build task before tests. The explicityarn buildstep duplicates this work unless Turbo caching kicks in.You can either:
- Remove this step and rely on Turbo's dependency graph (simpler, leverages caching).
- Keep it for explicitness but be aware of the minor overhead on cache misses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 30 - 31, The GitHub Actions workflow includes an explicit "Build packages" step that runs `yarn build`, which is redundant because your Turbo pipeline declares `test:ci` with `"dependsOn": ["build"]` in `turbo.json`; remove the `- name: Build packages` / `run: yarn build` step from the workflow so Turbo's `test:ci` dependency handles the build (or if you intentionally want the extra step, keep it knowingly as a duplicate and accept the overhead).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/lint.yml:
- Line 27: CI currently runs "yarn install" without enforcing lockfile
immutability; update the workflow step that runs the install (the run: yarn
install step) to enforce the lockfile by adding the appropriate flag for your
Yarn version: add --frozen-lockfile for Yarn Classic (v1) or --immutable for
Yarn Berry/v4 (or detect the project’s yarn version and use the matching flag)
so CI fails if the lockfile would change and keeps builds reproducible.
In @.github/workflows/publish.yml:
- Around line 40-45: The publish workflow's "Run tests" step currently runs yarn
test and injects TEST_MNEMONIC/TEST_PRIVATE_KEY/TEST_HEX_ADDRESS which causes
proto integration tests to fail if those GitHub secrets are not configured;
either add those secrets to Actions (TEST_MNEMONIC, TEST_PRIVATE_KEY,
TEST_HEX_ADDRESS) or change the step command from yarn test to a unit-only test
target (e.g., yarn test:unit or your CI-specific test script) so proto
integration tests are not executed during publish, or modify the proto tests to
skip when the TEST_MNEMONIC secret is absent (use a conditional describe.skip
based on process.env.TEST_MNEMONIC).
In @.github/workflows/test.yml:
- Line 28: Replace the plain "yarn install" CI step with an immutable install
that enforces the lockfile (i.e., add the --immutable flag to the yarn install
invocation) so the test workflow fails if the lockfile is out of sync, matching
the behavior used in lint.yml; update the existing "yarn install" step
accordingly to use the immutable install option.
In `@package.json`:
- Line 42: package.json defines a "codegen" script that runs "turbo run codegen"
but turbo.json has no "codegen" pipeline task; add a "codegen" entry to
turbo.json's "pipeline" section named "codegen" that runs the repository-level
code generation command used by your packages (the same command invoked by the
package.json "codegen" script), and configure sensible fields like "outputs"
(for generated artifacts) and "dependsOn" if it should run before/after other
tasks (e.g., build). This will ensure running npm run codegen triggers the turbo
task defined for code generation.
In `@packages/evm/tests/evidence.test.ts`:
- Around line 7-10: The file has an extra blank line after the describe.skip
declaration causing a Prettier violation; remove the empty line between
describe.skip('Evidence Precompile Tests', () => { and the first it('should get
all evidence with pagination', () => { so the it block sits directly beneath the
describe block start, preserving existing indentation and test contents in the
same file where describe.skip and the it function are defined.
In `@packages/lcd/package.json`:
- Around line 7-21: The package.json currently points "main" and the "require"
export to ./dist/cjs/index.js but the repository root declares "type": "module",
so Node treats .js under dist/cjs as ESM; fix by updating the build or
packaging: either emit the CommonJS bundle with a .cjs extension (so change the
CJS output from dist/cjs/index.js to dist/cjs/index.cjs and update "main" and
the "require" export accordingly) or have the build emit a dist/cjs/package.json
containing {"type":"commonjs"} alongside the generated CJS files so Node will
load them as CommonJS; update the build script that produces the CJS artifact
and the package.json "main"/"exports" entries to match the chosen approach
(refer to "main", "exports", and the dist/cjs folder).
In `@turbo.json`:
- Around line 16-21: Add a "codegen" task to turbo.json tasks: create a
"codegen" entry that declares outputs for the generated source dirs from the
packages that run codegen (referencing the package names proto, lcd, rwa and
their src/ directories) so Turbo can cache and track those artifacts; ensure the
task name matches the root package.json "codegen" script and include appropriate
outputs arrays listing each package's generated src (e.g., proto src, lcd src,
rwa src) so turbo will recognize and cache the generated files.
---
Outside diff comments:
In `@packages/rwa/package.json`:
- Around line 33-37: Remove the incorrect publishConfig.directory entry that
points to "dist" so npm will publish from the package root; specifically edit
the publishConfig block in packages/rwa/package.json to delete the "directory":
"dist" key (and apply the same fix to packages/proto/package.json and
packages/lcd/package.json) so the package exports (e.g., paths referencing
./dist/esm/index.js) resolve correctly after publishing.
In `@packages/utils/package.json`:
- Around line 64-68: Remove "npm" and "ts-node" from the "dependencies" object
in package.json for the packages/utils package; delete both entries and, if
"ts-node" is needed for local development only, add it to "devDependencies"
instead (do not add "npm" anywhere), ensuring only runtime deps like
"@kiichain/kiijs-proto" remain in "dependencies".
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 47-70: The publish steps for the workflow (steps named "Publish
`@kiichain/kiijs-proto`", "Publish `@kiichain/kiijs-lcd`", "Publish
`@kiichain/kiijs-utils`", "Publish `@kiichain/kiijs-evm`", "Publish
`@kiichain/kiijs-rwa`") run sequentially and will abort the remaining publishes if
one fails; update the workflow to either add continue-on-error: true to the
individual publish steps if partial publishes are acceptable, or insert a
pre-check step (e.g., a script that queries npm for the package@version for each
package under packages/proto, packages/lcd, packages/utils, packages/evm,
packages/rwa) and bail with a clear error before attempting publishes so
failures are isolated and reported cleanly.
In @.github/workflows/test.yml:
- Around line 30-31: The GitHub Actions workflow includes an explicit "Build
packages" step that runs `yarn build`, which is redundant because your Turbo
pipeline declares `test:ci` with `"dependsOn": ["build"]` in `turbo.json`;
remove the `- name: Build packages` / `run: yarn build` step from the workflow
so Turbo's `test:ci` dependency handles the build (or if you intentionally want
the extra step, keep it knowingly as a duplicate and accept the overhead).
In `@eslint.config.js`:
- Around line 69-70: The ESLint ignore pattern 'scripts/*' currently excludes
your new build script from linting; remove the 'scripts/*' entry from the ignore
array (or replace it with a narrower pattern) in eslint.config.js or add a
separate ESLint config for scripts so 'scripts/build.ts' is included and follows
project lint rules; search for the ignore array containing 'scripts/*' and
update it or add an overrides block targeting 'scripts/*.ts' to enable linting
for that file.
- Around line 30-43: The config currently turns off TypeScript safety rules
'@typescript-eslint/no-unsafe-assignment' and
'@typescript-eslint/no-unsafe-call' without explanation; add a brief comment
above those rule entries explaining the intentional rationale (e.g., legacy JS
interop, staged migration to stricter checks) or, better, replace the blanket
'off' with a targeted override or gradual strategy (enable in new/TS files or
use eslint overrides for specific directories/files) so the intent is documented
and maintainers know why these rules are relaxed.
In `@package.json`:
- Line 64: Update the turbo dependency version string in package.json to a
narrower pinned range (e.g., replace "turbo": "^2" with a specific tested
version or a tighter range such as "turbo": "^2.0.0" or "turbo": "2.12.0") so
that builds are reproducible; locate the "turbo" entry in package.json and
modify its value to the chosen exact or constrained version and then run your
package manager (npm/yarn/pnpm) to refresh lockfile.
In `@packages/evm/package.json`:
- Around line 55-56: The package currently lists "dotenv" under
peerDependencies; change this so consumers aren't forced to install it unless
the library actually requires dotenv at runtime. If dotenv is only used for
local development or tests, move "dotenv" from peerDependencies to
devDependencies in packages/evm/package.json (update the "peerDependencies"
block to remove "dotenv" and add it under "devDependencies"); if the library
truly relies on dotenv at runtime, instead document that requirement and keep it
as a regular dependency in "dependencies" rather than a peerDependency.
In `@packages/evm/tests/evidence.test.ts`:
- Around line 4-5: Remove the orphaned ESLint disable comment and the unused
jest.setTimeout call: delete the leading "// eslint-disable-next-line
`@typescript-eslint/no-require-imports`" comment and the
"jest.setTimeout(60_000);" statement from the test file (referencing the
jest.setTimeout usage) since there is no require() and the tests are
skipped/synchronous, so both are dead/noise.
In `@packages/rwa/tsconfig.json`:
- Around line 9-11: Update the stale comment next to the tsconfig.json "outDir"
setting to accurately reflect its purpose for emitting TypeScript declaration
files rather than compiled JavaScript; locate the "declaration" and "outDir"
entries in tsconfig.json and change the comment "Output directory for compiled
JavaScript" to something like "Output directory for generated .d.ts type
declarations (JS bundling handled by Bun to dist/esm and dist/cjs)" so it
correctly documents the role of outDir and its relationship to declaration:
true.
In `@scripts/build.ts`:
- Around line 36-55: Wrap each execSync invocation in try-catch blocks around
the three build steps (the ESM/CJS/Type declarations sections that log "→ ESM
bundle", "→ CJS bundle", and "→ Type declarations") so failures are caught; on
catch, call console.error (or processLogger if available) with a clear message
including which step failed (e.g., "Failed to build ESM bundle"), include the
caught error details (error.message and any error.output/stderr if present), and
then exit with a non‑zero status (process.exit(1)) or rethrow to ensure the
script fails deterministically.
- Around line 12-14: The file is ESM but uses require(resolve(...)) to load
package.json (pkg), which is non-standard; replace that with an ESM-compatible
approach such as using top-level await import for JSON modules (e.g. await
import(resolve(cwd, 'package.json'), { assert: { type: 'json' } }) and assign to
pkg) or read and parse the file with fs.readFileSync/JSON.parse(resolve(cwd,
'package.json')) instead; update the code that references pkg, keep cwd and
entrypoint as-is, and remove the require(...) call so the script no longer
relies on CommonJS require in an ESM module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0f789dd-6c6a-4786-a329-f3ea70a3e40c
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/lint.yml.github/workflows/publish-evm.yml.github/workflows/publish-lcd.yml.github/workflows/publish-proto.yml.github/workflows/publish-rwa.yml.github/workflows/publish-utils.yml.github/workflows/publish.yml.github/workflows/test.yml.gitignoreeslint.config.jslerna.jsonpackage.jsonpackages/evm/package.jsonpackages/evm/tests/evidence.test.tspackages/lcd/package.jsonpackages/lcd/tsconfig.declaration.jsonpackages/proto/package.jsonpackages/proto/tsconfig.declaration.jsonpackages/rwa/package.jsonpackages/rwa/tsconfig.declaration.jsonpackages/rwa/tsconfig.jsonpackages/utils/package.jsonscripts/build.tsturbo.json
💤 Files with no reviewable changes (6)
- lerna.json
- .github/workflows/publish-utils.yml
- .github/workflows/publish-proto.yml
- .github/workflows/publish-rwa.yml
- .github/workflows/publish-evm.yml
- .github/workflows/publish-lcd.yml
…ng proto lint Proto package lint now skips since files are generated. Sorted imports/exports across evm, rwa, and utils packages to satisfy simple-import-sort rules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix CJS resolution: build script now generates dist/cjs/package.json
with {"type": "commonjs"} so Node resolves .js as CJS correctly
- Remove publishConfig.directory from proto, lcd, rwa to prevent
double dist/ path resolution on npm publish
- Remove npm and ts-node from utils runtime dependencies
- Add codegen task to turbo.json
- Enforce lockfile in CI with yarn install --immutable
- Add continue-on-error to publish steps for error isolation
- Replace require() with fs.readFileSync in build script (ESM compat)
- Add try-catch error handling to build script
- Move dotenv from evm peerDependencies to devDependencies removal
- Clean up orphaned code in evidence.test.ts
- Fix stale comment in rwa/tsconfig.json
- Add explanatory comments for disabled ESLint rules
- Pin turbo version range to ^2.0.0
- Remove redundant build step from test.yml (turbo handles it)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lockfile was stale after turbo version pin change, causing yarn install --immutable to fail in CI. Also disable fail-fast so individual test failures don't cancel other package tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Hi @GiovaniGuizzo , The remaining CI failure in the Test Packages workflow is due to missing repository secrets. The EVM and other integration tests require the following secrets to be configured in the repo's Settings > Secrets and variables > Actions:
These secrets are not available to PRs from forks by design (GitHub security policy), so the tests fail with:
The Lint and Format and CodeQL checks are passing. Could a maintainer either:
Thanks! |
GiovaniGuizzo
left a comment
GiovaniGuizzo
left a comment
Collaborator
There was a problem hiding this comment.
Overall, the PR looks nice. I have a few comments to improve it a bit better.
The publish comment will take some joint effort. I will configure the NPM registry to run from publish.yml using OIDC, but I think there is some config to be done still in the workflow file.
Collaborator
Sure. We will run tests once merged into our main. I will fix anything that fails. |
Remove tracked install-state.gz and prevent it from being committed in the future. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add sourceMap: true to tsconfig.base.json (inherited by proto, lcd, utils, evm) and directly to packages/rwa/tsconfig.json which has its own config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ensure optionalDependencies are also treated as external during bundling, preventing them from being incorrectly inlined. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace execSync CLI calls with Bun.build() JS API for ESM and CJS bundling. Use Bun.spawnSync for tsc which has no JS API equivalent. This gives better error handling and avoids shell command string interpolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove global disabling of @typescript-eslint/no-unsafe-assignment and @typescript-eslint/no-unsafe-call. These were disabled for proto-generated types but affected all packages. Proto files are already excluded via the ignores pattern, so the global disable was unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move registry configuration from workflow inline commands to .yarnrc.yml and .npmrc files - Add id-token: write permission for npm OIDC provenance - Use npm publish --provenance instead of yarn npm publish - Remove the inline "Configure registry" step - Use NODE_AUTH_TOKEN (setup-node convention) with NPM_TOKEN secret The workflow is now ready for OIDC once the npm token secret is replaced with the OIDC trust policy on the npm side. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add scripts/tsconfig.json with ES2022 module/target to support top-level await, and a minimal Bun type declaration file for the Bun.build() and Bun.spawnSync() APIs used in the build script. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The root tsconfig uses module: commonjs which doesn't support top-level await. Wrap the async Bun.build() calls in an async function with .then()/.catch() to avoid the TS1378 error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/evm/package.json (1)
60-68:⚠️ Potential issue | 🟡 Minor
dotenvremoved from peerDependencies but still used by tests — add to devDependencies.
packages/evmimports'dotenv/config'in 10 test files andjest.config.ts, butdotenvis not declared in this package'sdevDependenciesor anywhere in the workspace root. Test execution will fail when resolving thedotenvimport, particularly in CI with--immutableinstalls or strict package management.Proposed fix
"devDependencies": { "@types/jest": "^29.5.14", "@wagmi/cli": "^2.7.0", + "dotenv": "^16.4.5", "ethers": "^6.0.0", "jest": "^29.7.0", "ts-jest": "^29.3.1", "viem": "2.x", "wagmi": "^2.18.2" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evm/package.json` around lines 60 - 68, The package is missing dotenv in its devDependencies but several test files and jest.config.ts import 'dotenv/config', causing test resolution failures; fix by adding "dotenv" (appropriate version, e.g., latest or a pinned semver) to the "devDependencies" section of packages/evm's package.json so the test runner and jest.config.ts can resolve the import during CI/immutable installs, then run a local install/test to verify imports in the test files and jest.config.ts resolve correctly.
🧹 Nitpick comments (2)
packages/evm/package.json (1)
32-40: Scripts update looks good; minor consistency nit.
build,test,test:ci, andlintall align with the shared pattern used acrossutils/lcd/proto/rwa. One small inconsistency: most scripts here useyarn(yarn jest) whilelint/prebuild/docsstill usenpx. Not a blocker, but standardizing onyarn(e.g.,yarn eslint src/,yarn rimraf dist) would better leverage workspace resolution and be consistent with the Jest invocation fix mentioned in the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evm/package.json` around lines 32 - 40, The package.json scripts are inconsistent: replace npx usages with yarn to match other packages; update the "prebuild" script to "yarn rimraf dist", the "lint" script to "yarn eslint src/", and the "docs" script to "yarn typedoc --out docs" (leave "build", "test", and "test:ci" as-is); locate and edit these script entries in package.json to standardize on yarn.packages/proto/package.json (1)
49-51:testandtest:ciare identical — consider differentiating or dropping one.Both invoke
yarn jestwith no extra flags. If CI is expected to produce JUnit/coverage output or run non-interactively with specific flags (e.g.--ci --coverage --reporters=default),test:cishould diverge; otherwise one of them is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/proto/package.json` around lines 49 - 51, The package.json currently has duplicate scripts "test" and "test:ci" both running "yarn jest"; either remove the redundant "test:ci" script or update it to a CI-oriented invocation (e.g., add flags like --ci, --coverage, and a JUnit/coverage reporter) so it differs from "test"; modify the "test:ci" script entry accordingly in package.json to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/proto/package.json`:
- Around line 33-36: The package.json for packages/proto currently lacks a
"files" allowlist so publishing from the package root will include source/tests;
add a "files" array entry to package.json (same change applied to packages/lcd
and packages/rwa) listing only the intended published artifacts (e.g., built
dist folder, README.md, LICENSE, package.json and any typings) and remove or
ignore test/src assets; update the JSON key "files" at the top-level of the
manifest (near the existing "publishConfig") so npm will only include those
paths when running npm publish.
---
Outside diff comments:
In `@packages/evm/package.json`:
- Around line 60-68: The package is missing dotenv in its devDependencies but
several test files and jest.config.ts import 'dotenv/config', causing test
resolution failures; fix by adding "dotenv" (appropriate version, e.g., latest
or a pinned semver) to the "devDependencies" section of packages/evm's
package.json so the test runner and jest.config.ts can resolve the import during
CI/immutable installs, then run a local install/test to verify imports in the
test files and jest.config.ts resolve correctly.
---
Nitpick comments:
In `@packages/evm/package.json`:
- Around line 32-40: The package.json scripts are inconsistent: replace npx
usages with yarn to match other packages; update the "prebuild" script to "yarn
rimraf dist", the "lint" script to "yarn eslint src/", and the "docs" script to
"yarn typedoc --out docs" (leave "build", "test", and "test:ci" as-is); locate
and edit these script entries in package.json to standardize on yarn.
In `@packages/proto/package.json`:
- Around line 49-51: The package.json currently has duplicate scripts "test" and
"test:ci" both running "yarn jest"; either remove the redundant "test:ci" script
or update it to a CI-oriented invocation (e.g., add flags like --ci, --coverage,
and a JUnit/coverage reporter) so it differs from "test"; modify the "test:ci"
script entry accordingly in package.json to reflect the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b3b647f-e7d5-45c7-a4b0-92e818d4e62e
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (51)
.github/workflows/lint.yml.github/workflows/publish.yml.github/workflows/test.yml.gitignore.npmrc.yarnrc.ymleslint.config.jspackage.jsonpackages/evm/package.jsonpackages/evm/src/ethers/bankPrecompile.tspackages/evm/src/ethers/bech32Precompile.tspackages/evm/src/ethers/distributionPrecompile.tspackages/evm/src/ethers/governancePrecompile.tspackages/evm/src/ethers/ibcPrecompile.tspackages/evm/src/ethers/ics20Precompile.tspackages/evm/src/ethers/slashingPrecompile.tspackages/evm/src/ethers/stakingPrecompile.tspackages/evm/src/ethers/wasmPrecompile.tspackages/evm/src/viem/bankPrecompile.tspackages/evm/src/viem/bech32Precompile.tspackages/evm/src/viem/distributionPrecompile.tspackages/evm/src/viem/governancePrecompile.tspackages/evm/src/viem/ibcPrecompile.tspackages/evm/src/viem/ics20Precompile.tspackages/evm/src/viem/index.tspackages/evm/src/viem/slashingPrecompile.tspackages/evm/src/viem/stakingPrecompile.tspackages/evm/src/viem/wasmPrecompile.tspackages/evm/src/wagmi/bank.tspackages/evm/src/wagmi/bech32.tspackages/evm/src/wagmi/distribution.tspackages/evm/src/wagmi/governance.tspackages/evm/src/wagmi/ibc.tspackages/evm/src/wagmi/ics20.tspackages/evm/src/wagmi/slashing.tspackages/evm/src/wagmi/staking.tspackages/evm/src/wagmi/wasm.tspackages/evm/tests/evidence.test.tspackages/lcd/package.jsonpackages/proto/package.jsonpackages/rwa/package.jsonpackages/rwa/src/index.tspackages/rwa/tsconfig.jsonpackages/utils/package.jsonpackages/utils/src/cosmjs/signer.tspackages/utils/src/index.tsscripts/build.tsscripts/bun.d.tsscripts/tsconfig.jsontsconfig.base.jsonturbo.json
✅ Files skipped from review due to trivial changes (38)
- .gitignore
- packages/evm/src/viem/slashingPrecompile.ts
- packages/evm/src/viem/bech32Precompile.ts
- .npmrc
- tsconfig.base.json
- packages/evm/src/viem/bankPrecompile.ts
- packages/evm/src/viem/governancePrecompile.ts
- packages/evm/src/wagmi/slashing.ts
- packages/evm/src/viem/distributionPrecompile.ts
- packages/evm/src/wagmi/bech32.ts
- packages/evm/src/ethers/governancePrecompile.ts
- packages/evm/src/wagmi/distribution.ts
- packages/evm/src/viem/stakingPrecompile.ts
- packages/evm/src/wagmi/bank.ts
- packages/evm/src/ethers/stakingPrecompile.ts
- packages/evm/src/wagmi/ics20.ts
- packages/evm/src/ethers/slashingPrecompile.ts
- packages/evm/src/viem/ics20Precompile.ts
- packages/rwa/src/index.ts
- packages/evm/src/ethers/bankPrecompile.ts
- packages/rwa/tsconfig.json
- packages/evm/src/ethers/ics20Precompile.ts
- packages/evm/src/viem/index.ts
- packages/evm/src/ethers/bech32Precompile.ts
- packages/evm/src/wagmi/wasm.ts
- packages/evm/src/ethers/distributionPrecompile.ts
- packages/evm/src/ethers/wasmPrecompile.ts
- packages/evm/src/wagmi/ibc.ts
- packages/evm/src/viem/wasmPrecompile.ts
- packages/evm/src/wagmi/governance.ts
- packages/evm/src/viem/ibcPrecompile.ts
- .yarnrc.yml
- scripts/tsconfig.json
- packages/utils/src/cosmjs/signer.ts
- packages/evm/src/wagmi/staking.ts
- packages/evm/src/ethers/ibcPrecompile.ts
- packages/utils/src/index.ts
- scripts/bun.d.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/test.yml
- .github/workflows/lint.yml
- .github/workflows/publish.yml
- packages/utils/package.json
- eslint.config.js
- packages/lcd/package.json
- turbo.json
- packages/rwa/package.json
- scripts/build.ts
- package.json
- Add Bun setup step to lint workflow (required for build) - Remove npmAlwaysAuth from .yarnrc.yml and .npmrc to avoid auth errors when reading public packages in CI - Add "files": ["dist"] to all 5 packages to limit npm publish scope - Regenerate yarn.lock to match updated .yarnrc.yml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GiovaniGuizzo
left a comment
GiovaniGuizzo
left a comment
Collaborator
There was a problem hiding this comment.
Much better. I have a few other minor comments. We are almost there :)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/lcd/package.json (2)
10-21: Exports map looks good — CJS resolution concern from earlier review is resolved.The previous concern about
./dist/cjs/index.jsbeing parsed as ESM (due to root"type": "module") is addressed inscripts/build.ts, which now writesdist/cjs/package.jsonwith{"type": "commonjs"}alongside the CJS bundle. Since"files": ["dist"]publishes the entiredisttree, this override file will be included in the published tarball and Node will correctly resolverequire("@kiichain/kiijs-lcd")as CommonJS.One small optional enhancement: consider adding a
"./package.json"subpath to theexportsmap, since some tooling (bundlers, resolvers, postinstall scripts) still reads the manifest directly:♻️ Optional: expose package.json via exports
"exports": { ".": { "import": { "types": "./dist/types/index.d.ts", "default": "./dist/esm/index.js" }, "require": { "types": "./dist/types/index.d.ts", "default": "./dist/cjs/index.js" } - } + }, + "./package.json": "./package.json" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lcd/package.json` around lines 10 - 21, Add an explicit "./package.json" subpath to the package's "exports" map so tooling that reads the manifest via imports/resolution can access package metadata; update the exports object in packages/lcd/package.json (the "exports" entry where "." maps to import/require) to include a "./package.json": "./package.json" entry alongside the existing mappings, ensuring it coexists with the current import/require keys.
47-52: Consider adding aprepack/prepublishOnlyhook to guard against publishing staledist.With
"files": ["dist"]and no lifecycle script that builds before publish, a manualnpm publish/yarn npm publishrun without a precedingyarn buildwill either fail (nodist) or publish stale artifacts. The unified CI publish workflow likely builds explicitly, but adding a local safeguard makes the package publishable independently and prevents accidents:♻️ Suggested addition
"scripts": { "clean": "npx rimraf dist", "build": "bun run ../../scripts/build.ts", "codegen": "npx tsx scripts/codegen.ts", - "lint": "npx eslint src/" + "lint": "npx eslint src/", + "prepack": "yarn build" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lcd/package.json` around lines 47 - 52, Add a lifecycle hook to package.json to ensure dist is built before a local publish: create either a "prepack" or "prepublishOnly" entry under the scripts object that runs the existing "build" script (the one named "build": "bun run ../../scripts/build.ts") so publishing cannot proceed with missing or stale dist artifacts; update the scripts block to include "prepack" (or "prepublishOnly") referencing the "build" command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rwa/package.json`:
- Line 50: The package's "codegen" script currently invokes "npx ts-node
scripts/codegen.ts" but ts-node is not in devDependencies and other packages use
"tsx"; update the "codegen" script to use "tsx" (e.g., "tsx scripts/codegen.ts")
and add "tsx" to this package's devDependencies so the runner is declared and
consistent with packages/lcd; ensure you remove or stop referencing "ts-node" in
this package to avoid ambiguity.
---
Nitpick comments:
In `@packages/lcd/package.json`:
- Around line 10-21: Add an explicit "./package.json" subpath to the package's
"exports" map so tooling that reads the manifest via imports/resolution can
access package metadata; update the exports object in packages/lcd/package.json
(the "exports" entry where "." maps to import/require) to include a
"./package.json": "./package.json" entry alongside the existing mappings,
ensuring it coexists with the current import/require keys.
- Around line 47-52: Add a lifecycle hook to package.json to ensure dist is
built before a local publish: create either a "prepack" or "prepublishOnly"
entry under the scripts object that runs the existing "build" script (the one
named "build": "bun run ../../scripts/build.ts") so publishing cannot proceed
with missing or stale dist artifacts; update the scripts block to include
"prepack" (or "prepublishOnly") referencing the "build" command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38440285-2d48-46a8-8200-a686477b876f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/lint.yml.github/workflows/publish.yml.npmrc.yarnrc.ymlpackage.jsonpackages/evm/package.jsonpackages/lcd/package.jsonpackages/proto/package.jsonpackages/rwa/package.jsonpackages/rwa/tsconfig.jsonpackages/utils/package.jsonscripts/tsconfig.json
✅ Files skipped from review due to trivial changes (4)
- .npmrc
- .yarnrc.yml
- scripts/tsconfig.json
- packages/rwa/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/lint.yml
- .github/workflows/publish.yml
- package.json
- packages/utils/package.json
- packages/evm/package.json
- packages/proto/package.json
isonnymichael
force-pushed
the
feat/v1-revamp
branch
from
6ad1de6 to
2a4ecdc
Compare
GiovaniGuizzo
approved these changes
Apr 17, 2026
Collaborator
|
Approved! Nice work. Thanks for the contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Description
Summary
Comprehensive overhaul of the SDK build infrastructure addressing all 7 items from #56:
dist/esm,dist/cjs,dist/typesstructure with properexportsmaps in every packageformat:checkin CIpublish.ymlnpx→yarn), skipped unimplemented evidence testChanges by commit
build: replace Lerna with Turborepo for monorepo managementbuild: adopt Bun bundler with unified CJS/ESM/types output structurebuild: enforce linting and formatting standardstest: fix test runner and skip broken evidence testci: consolidate release automation with tag-triggered publishingTest results
TEST_MNEMONICenv (integration tests, unchanged behavior)Notes
evidence.test.tswas skipped (not deleted) sincesrc/ethers/evidencePrecompile.tswas never createdoven-sh/setup-bunin CITest plan
yarn buildcompletes for all 5 packagesyarn testpasses for rwa, utils, evmformat:checkcorrectlyimportandrequireCloses #56