Skip to content

chore: deploy as npm package - #3

Merged
thomgabriel merged 8 commits into
masterfrom
dev
Oct 28, 2025
Merged

chore: deploy as npm package#3
thomgabriel merged 8 commits into
masterfrom
dev

Conversation

@thomgabriel

Copy link
Copy Markdown
Member

No description provided.

@claude

claude Bot commented Oct 20, 2025

Copy link
Copy Markdown

PR Review: Deploy as npm package

Thanks for working on making this a publishable npm package! Here's my review:

Positive Changes

  1. Good package scope: Using @pollum-io/vitalfi-programs provides proper namespacing
  2. Proper exports configuration: Modern exports field supports both TypeScript types and IDL
  3. Peer dependencies: Correctly specified to avoid version conflicts with consuming projects
  4. Build script: Added anchor build to the scripts

Critical Issues

1. Missing build artifacts in repository

  • The files array references target/types/vitalfi_vault.ts and target/idl/vitalfi_vault.json
  • These files are in .gitignore (line 3: target)
  • Impact: The package will be empty when published to npm

Recommendations:

  • Option A (Recommended): Add a .npmignore file that explicitly includes the build artifacts
  • Option B: Add prepublishOnly script to ensure build happens before publishing
  • Option C: Consider committing built artifacts if this is standard for Anchor projects

2. Entry point issues

  • main and types both point to .ts files: target/types/vitalfi_vault.ts
  • Problem: TypeScript source files are not typically used as package entry points
  • Expected: These should point to compiled .js files with .d.ts type definitions

Recommendation:

  • Verify that Anchor generates ready-to-use TypeScript files, OR
  • Add a compilation step to generate .js and .d.ts files
  • Update paths accordingly

3. Version conflict in dependencies

  • dependencies: @coral-xyz/anchor ^0.31.1
  • peerDependencies: @coral-xyz/anchor ^0.31.0

Issue: The peer dependency allows 0.31.0, but your package requires >=0.31.1

Recommendation: Align versions to ^0.31.1 in peerDependencies


Important Considerations

4. Missing package metadata

Consider adding repository, keywords, author, and publishConfig fields

5. No README or documentation

  • Add a README.md to the files array
  • Document installation and usage for npm consumers

6. Missing prepublish checks

Add validation before publishing to ensure lint, build, and tests pass


Testing Recommendations

Before merging, test the package locally with npm pack and verify imports work


Security Considerations

  • No sensitive data or credentials in changes
  • Appropriate use of peer dependencies
  • Ensure .npmignore is properly configured to avoid publishing sensitive files

Summary

This PR has the right structure but needs fixes before it can successfully publish:

  1. Critical: Configure .npmignore or commit build artifacts
  2. Critical: Verify entry point paths are correct for Anchor-generated files
  3. Important: Align peer dependency versions
  4. Nice to have: Add package metadata and documentation

Please address the critical issues and test with npm pack before publishing!

@claude

claude Bot commented Oct 20, 2025

Copy link
Copy Markdown

PR Review: Deploy as npm package

Summary

This PR adds npm packaging configuration to enable publishing the VitalFi Vault program IDL as an npm package. The approach is sound overall.

Key Findings

1. CRITICAL: Missing IDL file

The package references target/idl/vitalfi_vault.json which does not exist in the repository. This will cause publishing to fail.

  • Action: Ensure anchor build is run before publishing or add a prepublishOnly script

2. License Mismatch

  • package.json declares ISC but LICENSE file contains MIT text
  • Fix: Change package.json to MIT

3. Lockfile Conflicts

Both package-lock.json and yarn.lock are present, but Anchor.toml specifies yarn

  • Recommendation: Remove package-lock.json or switch to npm exclusively

4. Missing Metadata

package.json should include repository, keywords, author, homepage fields for better npm discoverability

5. Package Versioning

Consider adding CHANGELOG.md and using 0.1.0 for initial npm release

Security

Low risk - only distributes JSON IDL file with no executable code

Performance

Excellent - minimal package size

Test Coverage

Missing - consider adding validation tests for IDL file existence

Recommendation

Request changes - address license mismatch, missing IDL, and lockfile inconsistency before merging

@claude

claude Bot commented Oct 20, 2025

Copy link
Copy Markdown

Pull Request Review: NPM Package Configuration

Summary

This PR configures the repository to publish as an NPM package (@pollum-io/vitalfi-programs), making the Anchor IDL and TypeScript types available for external consumption. Overall, the changes are well-structured for package distribution, but there are several important considerations before publishing.


✅ Positive Changes

1. Well-Configured Package Metadata

  • Comprehensive package.json with proper repository links, keywords, and description
  • Clear entry points via main, types, and exports fields
  • Good use of peerDependencies to avoid version conflicts

2. Proper .npmignore Setup

  • Whitelist approach (ignore everything, then include specific files) is secure
  • Only publishes essential artifacts: IDL, types, README, and LICENSE

3. Dependency Management

  • Moving Anchor to peerDependencies is correct - consumers should provide their own version
  • Updated to Anchor 0.32.x aligns with current ecosystem standards

4. Build Automation

  • prepublishOnly script ensures IDL/types are built before publishing
  • Prevents accidental publication of stale artifacts

⚠️ Critical Issues

1. Missing Build Artifacts in Repository 🔴

The package specifies files that won't exist in the repository: target/idl/vitalfi_vault.json and target/types/vitalfi_vault.ts

Problem: The target/ directory is git-ignored and won't be in the published branch by default.

Solutions:

  • Option A (Recommended): Use GitHub Actions to build and publish (anchor build then npm publish)
  • Option B: Commit built artifacts to a separate dist/ branch
  • Option C: Rely on prepublishOnly script (verify with npm pack --dry-run after anchor build)

Testing: Before publishing to NPM, run: anchor build then npm pack --dry-run. This will show exactly what files would be included.


2. Version Number Should Start at 0.1.0 🟡

Current version: 0.1.4

This suggests 3 prior versions exist, but this is the first NPM publication. Consider starting at 0.1.0 for initial release and using semantic versioning going forward.


3. Missing npm Package Scope Verification 🟡

Package name: @pollum-io/vitalfi-programs

Questions:

  1. Does the @pollum-io organization exist on NPM?
  2. Do you have publish permissions for this scope?
  3. Is the package name available?

Verify with: npm info @pollum-io/vitalfi-programs


🔒 Security Considerations

1. Good: .npmignore Prevents Source Code Leakage

Only IDL/types are published - program source code stays private. ✅

2. Consider: Add .npmrc to Repository

Prevent accidental publication to wrong registry

3. Verify: No Sensitive Data in IDL

The IDL is generated from the Anchor program and will be public. Ensure no private keys, seeds, internal URLs, or sensitive business logic documentation.


📦 Package.json Review

Well Done:

  • ✅ Proper module exports configuration
  • ✅ Peer dependencies instead of regular dependencies
  • ✅ Build scripts configured correctly
  • ✅ Comprehensive metadata and keywords

Minor Improvements:

1. Add Engines Field - Specify minimum Node.js version

2. Add publishConfig - Required for scoped packages to be public

3. Enhance exports with explicit types field


🧪 Testing Recommendations

Before Merging:

  1. Dry Run Publish: anchor build and npm pack to verify tarball contents
  2. Test Local Installation: Install the packed tarball in a test project
  3. Verify TypeScript Types: Ensure imports work correctly

📝 Documentation Additions Needed

1. Update README with NPM Installation

Add section with installation and usage examples showing how to import the IDL and types.

2. Add CHANGELOG.md

Document version history and breaking changes.


🔄 Yarn.lock Changes

The yarn.lock shows a major version update (likely Yarn 3+ format). This is fine but note:

  • ✅ Dependencies properly updated to Anchor 0.32.1
  • ✅ No suspicious new dependencies introduced
  • ⚠️ Large diff due to lockfile format change (-2487/+1271 lines)

🏁 Recommendation: Approve with Changes

Required Before Publishing:

  1. ✅ Verify @pollum-io NPM organization access
  2. ✅ Add publishConfig.access = public to package.json
  3. ✅ Test npm pack to ensure build artifacts are included
  4. ✅ Add NPM installation instructions to README

Suggested Before Publishing:

  1. Reset version to 0.1.0 for initial release
  2. Add engines field to package.json
  3. Add CHANGELOG.md
  4. Set up automated publishing via GitHub Actions

Post-Merge Actions:

  1. Create GitHub Release matching NPM version
  2. Test package installation from NPM registry
  3. Monitor for any installation issues from consumers

Great work on preparing this for NPM publication! The package structure is solid - just needs verification of the build artifact handling before going live. 🚀


Review generated for PR #3 - chore: deploy as npm package

@claude

claude Bot commented Oct 24, 2025

Copy link
Copy Markdown

Pull Request Review: Deploy as NPM Package

Summary

This PR configures the repository to publish the VitalFi Vault program IDL and TypeScript types as an npm package (@pollum-io/vitalfi-programs). The changes also include a small but critical bug fix in the close_vault instruction.


✅ Positive Changes

1. NPM Package Configuration (package.json)

  • Well-structured exports: The package correctly exposes both IDL JSON and TypeScript types through multiple entry points
  • Appropriate peerDependencies: Moving @coral-xyz/anchor and @solana/web3.js to peerDependencies prevents version conflicts
  • Good metadata: Keywords, repository info, and description are appropriate
  • Build automation: prepublishOnly script ensures types are generated before publishing

2. .npmignore Configuration

  • Minimal bundle: Correctly configured to only publish essential files (IDL, types, README, LICENSE)
  • Whitelist approach: Using * then negating specific files is safer than blacklisting

3. Critical Bug Fix in close_vault (instructions.rs:648-660)

// Before: let vault = &ctx.accounts.vault;
// After:  let vault = &mut ctx.accounts.vault;

vault.status = VaultStatus::Closed;  // NEW: Sets status before closing

This is an important fix! The original code:

  • Didn't set the vault status to Closed before closing the account
  • Used an immutable reference, which would have prevented the status update anyway

Impact: Without this fix, the vault would be closed without properly transitioning to Closed status, potentially causing inconsistencies in event logs and off-chain indexing.

@claude

claude Bot commented Oct 24, 2025

Copy link
Copy Markdown

🔍 Issues & Concerns

CRITICAL: Missing Files for NPM Package ⚠️

The package.json declares these files:

"files": [
  "target/idl/vitalfi_vault.json",
  "target/types/vitalfi_vault.ts",
  "README.md",
  "LICENSE"
]

Problem: The target/ directory is in .gitignore, so these critical files won't be in the git repository. This means:

  1. Publishing will likely fail unless build artifacts are generated during prepublishOnly
  2. Users importing from GitHub won't have access to types/IDL
  3. CI/CD needs special handling to build before publish

Recommendations:

  • ✅ Ensure your CI/CD pipeline runs anchor build before npm publish
  • ✅ Consider committing generated files OR document that users must build from source
  • ✅ Add a .npmrc or publish script that verifies build artifacts exist

MODERATE: Dependency Version Bump ⚠️

- "@coral-xyz/anchor": "^0.31.1"
+ "@coral-xyz/anchor": "^0.32.1"

Concerns:

  • Anchor 0.32 includes breaking changes from 0.31
  • No mention in PR description about testing with new version
  • Could affect consumers who are still on 0.31

Recommendations:

  • ✅ Verify all tests pass with Anchor 0.32.1
  • ✅ Document breaking changes (if any) in README or CHANGELOG
  • ✅ Consider semver implications for your package version

MINOR: package-lock.json in .gitignore

+package-lock.json

Observation: The project uses Yarn (per Anchor.toml), so this is fine. However, yarn.lock should NOT be ignored to ensure reproducible builds.

Current state: yarn.lock is tracked ✅ (changes are in this PR)

@claude

claude Bot commented Oct 24, 2025

Copy link
Copy Markdown

🔒 Security Review

Solana Program Changes

The only Rust code change is the bug fix in close_vault. Security analysis:

Status transition is correct: Setting Closed before closing the account is proper state management

No new attack vectors: The change doesn't introduce new security issues

Maintains authorization: Still requires authority signature and proper status constraints

NPM Package Security

No executable scripts in package
Minimal published surface area (only IDL + types)
No postinstall hooks that could execute malicious code


🎯 Best Practices & Recommendations

1. Add a CHANGELOG.md

Document this version and the bug fix:

## [0.1.4] - 2024-XX-XX
### Added
- Published as npm package @pollum-io/vitalfi-programs
### Fixed
- close_vault now properly sets vault status to Closed before closing account

2. Version the Package Correctly

Current version: 0.1.4

  • If the Anchor upgrade includes breaking changes, consider bumping to 0.2.0
  • If just adding npm packaging, 0.1.4 is reasonable

3. Add Package Build Verification

Add to package.json scripts:

"prepack": "test -f target/idl/vitalfi_vault.json || (echo 'Run anchor build first' && exit 1)"

4. Document Usage in README

Add installation and usage instructions for npm users

5. Test the Package Locally

Before publishing:

npm pack
# Creates @pollum-io-vitalfi-programs-0.1.4.tgz
# Extract and verify contents

@claude

claude Bot commented Oct 24, 2025

Copy link
Copy Markdown

📊 Test Coverage

Observation: The PR doesn't modify test files, which is good since the changes are minimal. However:

Question: Do existing tests cover the close_vault status transition?

  • Tests should verify vault.status == VaultStatus::Closed after calling close_vault

✅ Approval Recommendation

Status: ✅ Approve with minor changes

This PR is well-structured and includes an important bug fix. The npm packaging setup is solid, but please address:

  1. MUST: Document how CI/CD will handle building before npm publish
  2. SHOULD: Verify tests pass with Anchor 0.32.1
  3. SHOULD: Add usage documentation to README
  4. NICE TO HAVE: Add CHANGELOG.md and prepack verification

📝 Summary Checklist

  • ✅ Code quality: Clean, minimal changes
  • ✅ Bug fix: Critical status update in close_vault
  • ✅ Security: No new vulnerabilities introduced
  • ⚠️ Documentation: Needs npm usage guide
  • ⚠️ Build process: Needs CI/CD verification
  • ✅ Testing: No test changes needed (existing coverage applies)
  • ✅ Package structure: Well-configured

Great work on preparing this for npm distribution! The bug fix is a valuable addition. 🎉


🤖 Generated with Claude Code

@thomgabriel
thomgabriel merged commit 6c9ac5d into master Oct 28, 2025
1 check passed
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.

1 participant