feat(sdk,graphql): deprecate pinThing mutation in favor of direct Pinata uploads

[simonas-notcat]

Summary

Migrates from backend-mediated IPFS pinning (via GraphQL API) to client-side Pinata uploads for improved performance and reliability. This removes the dependency on the GraphQL backend for IPFS operations, allowing SDK users to interact directly with Pinata.

Breaking Changes

• createAtomFromThing now requires pinataApiJWT in the config parameter
• batchCreateAtomsFromThings now requires pinataApiJWT in the config parameter
• Both functions now use CreateAtomConfigWithIpfs type instead of WriteConfig

Deprecations

• GraphQL: pinThing mutation is deprecated (use uploadJsonToPinata from SDK instead)
• SDK: pinThing function is deprecated (use uploadJsonToPinata instead)

Changes

• Updated SDK functions to use direct Pinata uploads via uploadJsonToPinata
• Added comprehensive deprecation notices to GraphQL README and mutation files
• Updated all SDK documentation and examples with new Pinata JWT requirement
• Modified nextjs-template example to demonstrate new configuration
• Added CHANGELOG entries for both SDK and GraphQL packages

Migration Guide

Before:

const atom = await createAtomFromThing(
  { walletClient, publicClient, address },
  { name: 'Example', url: 'https://example.com' }
)

After:

const atom = await createAtomFromThing(
  {
    walletClient,
    publicClient,
    address,
    pinataApiJWT: process.env.PINATA_API_JWT, // New required parameter
  },
  { name: 'Example', url: 'https://example.com' }
)

Test plan

• Verify createAtomFromThing works with Pinata JWT
• Verify batchCreateAtomsFromThings works with Pinata JWT
• Confirm proper error handling when Pinata JWT is missing
• Test nextjs-template example with updated configuration
• Verify all documentation links and examples are accurate
• Ensure backward compatibility warnings are visible in deprecated functions

🤖 Generated with Claude Code

[claude]

PR Review: Deprecate pinThing mutation in favor of direct Pinata uploads

Summary

This PR successfully migrates from backend-mediated IPFS pinning to client-side Pinata uploads. The changes are well-documented with proper deprecation notices and migration guides. Overall, this is a solid refactoring that improves architecture by reducing backend dependencies.

✅ Strengths

1. Clear Breaking Changes Documentation - Comprehensive migration guide, well-documented CHANGELOGs, proper deprecation notices
2. Consistent API Design - Both functions updated consistently with clean type definition
3. Good Error Handling - Example component checks for missing JWT, uploadJsonToPinata has try-catch
4. Documentation Quality - Extensive README updates with deprecation warnings

🔍 Issues & Recommendations

High Priority

1. Security: Exposed Pinata JWT in Client-Side Code ⚠️
Location: apps/nextjs-template/src/components/intuition/IntuitionCreateThingButton.tsx:20

Using NEXT_PUBLIC_* exposes the JWT to the browser, allowing users to extract and abuse your Pinata API key. This could lead to unauthorized IPFS uploads, quota abuse, and API key compromise.

Recommendation: Use a backend API route to proxy Pinata uploads securely, or use Pinata key scoping. Update docs to warn about this.

2. Type Safety: Missing URI Format Validation
Location: packages/sdk/src/core/create-atom-from-thing.ts:26

No validation that dataIpfs.IpfsHash is valid. Add validation before constructing the URI.

3. Inconsistent URI Construction
Different patterns between create-atom-from-thing.ts and create-atom-from-ipfs-upload.ts. Choose one pattern.

Medium Priority

4. Test Coverage Missing - No tests for createAtomFromThing and batchCreateAtomsFromThings

5. Unused Variable - Line 22 in packages/sdk/src/api/pin-thing.ts is a no-op

6. Sequential Uploads - Batch operation uploads sequentially (slow). Consider Promise.all for parallelization.

7. Documentation Typo - Extra quote in description string at IntuitionCreateThingButton.tsx:36

📊 Overall Assessment

• Code Quality: 7.5/10 (well-structured, needs test coverage)
• Security: 6/10 (critical JWT exposure issue)
• Documentation: 9/10 (excellent guides)
• Performance: 7/10 (sequential uploads)

✅ Recommendation

Approve with changes - Fix the NEXT_PUBLIC_PINATA_API_JWT exposure before merging, add input validation, consider parallelizing uploads, and add tests.

Great work on the migration and documentation!

🤖 Review generated with Claude Code

[claude]

PR Review: Migration to Direct Pinata Uploads

Summary

This PR successfully migrates from backend-mediated IPFS pinning to client-side Pinata uploads. The implementation is clean, well-documented, and maintains backward compatibility through clear deprecation notices.

✅ Strengths

1. Clear Breaking Change Communication

• Comprehensive migration guide in the PR description
• Well-documented deprecation notices in both code and documentation
• CHANGELOG entries properly capture the changes

2. Security Awareness

• Excellent security warning in IntuitionCreateThingButton.tsx:20-23 about exposing NEXT_PUBLIC_* environment variables
• Clear guidance to use backend API routes for production (line 22)

3. Type Safety

• Introduced CreateAtomConfigWithIpfs type that properly extends WriteConfig with pinataApiJWT
• Maintains strict TypeScript usage per project guidelines

4. Error Handling

• Proper validation of IPFS hash in createAtomFromThing.ts:27-29
• Comprehensive error messages in uploadJsonToPinata.ts:39-41
• Transaction validation in createAtomFromThing.ts:45-47

5. Performance Improvement

• batchCreateAtomsFromThings uses Promise.all for parallel uploads (line 47)
• Removes dependency on GraphQL backend, reducing latency

🔍 Issues & Concerns

1. Security: API Key Exposure (CRITICAL)

Location: apps/nextjs-template/src/components/intuition/IntuitionCreateThingButton.tsx:24

While the warning comment is excellent, the example code still demonstrates an insecure pattern. Consider:

// Current (insecure for production):
const pinataApiJWT = process.env.NEXT_PUBLIC_PINATA_API_JWT

// Better approach - add a comment suggesting:
// TODO: Replace with: const response = await fetch('/api/upload-to-pinata', { ... })

Recommendation: Either provide a secure implementation example or add a more prominent warning that this code should NOT be deployed to production as-is.

2. Missing Error Context in Batch Operations

Location: packages/sdk/src/core/batch-create-atoms-from-things.ts:37-45

When uploading multiple items in parallel, if one fails, the error doesn't indicate which item failed:

const uploadPromises = data.map(async (item) => {
  const dataIpfs = await uploadJsonToPinata(config.pinataApiJWT, item)
  
  if (!dataIpfs.IpfsHash || dataIpfs.IpfsHash.trim() === '') {
    throw new Error('Invalid IPFS hash received from Pinata') // Which item failed?
  }
  
  return \`ipfs://\${dataIpfs.IpfsHash}\`
})

Recommendation: Add context to errors:

throw new Error(\`Invalid IPFS hash received from Pinata for item: \${JSON.stringify(item).slice(0, 100)}\`)

3. Inconsistent Error Parsing

Locations:

• create-atom-from-thing.ts:49 uses eventParseAtomCreated
• create-atom-from-ipfs-upload.ts:50 uses eventParseDeposited

Question: Why does createAtomFromThing parse AtomCreated events while createAtomFromIpfsUpload parses Deposited events? This seems inconsistent. Should they both use eventParseAtomCreated?

4. Test Coverage Gap

The existing test in packages/sdk/tests/external.test.ts only tests the happy path and appears to expect a hardcoded IPFS hash:

expect(data.IpfsHash).toEqual('QmYiJdC6KbkUuKVYeuQLm3DRbaok3h6yPyJjPqas8cotqx')

Missing test cases:

• Error handling when Pinata API returns non-200 response
• Error handling when IPFS hash is empty/invalid
• Testing createAtomFromThing with Pinata JWT
• Testing batchCreateAtomsFromThings with Pinata JWT
• Handling of partial failures in batch operations

5. Minor: Typo Fix

Location: apps/nextjs-template/src/components/intuition/IntuitionCreateThingButton.tsx:40

Good catch fixing the typo from 'A decentralized trust protocol to A decentralized trust protocol (removed extra quote).

6. Documentation Formatting

The GraphQL README.md has significant formatting changes (many are just spacing/line breaks), but the core deprecation message at line 1029 is clear.

📋 Recommendations

High Priority

1. Security Warning: Make the security implications more prominent in the example code
2. Error Context: Add item context to batch operation errors
3. Test Coverage: Add comprehensive error handling tests
4. Clarify Event Parsing: Document why different functions parse different events

Medium Priority

5. Rate Limiting: Consider adding retry logic or rate limiting guidance for Pinata API calls
6. Validation: Add JSON schema validation before uploading to catch malformed data early
7. Documentation: Add examples of the recommended secure backend proxy pattern

Low Priority

8. Type Safety: Consider making depositAmount required rather than optional with undefined behavior
9. Logging: Add debug logging for IPFS operations to help with troubleshooting

🧪 Test Plan Status

From the PR description, ensure these are tested:

• ✅ Verify `createAtomFromThing` works with Pinata JWT
• ✅ Verify `batchCreateAtomsFromThings` works with Pinata JWT
• ⚠️ Confirm proper error handling when Pinata JWT is missing (needs test)
• ⚠️ Test nextjs-template example with updated configuration (manual testing needed)
• ✅ Verify all documentation links and examples are accurate
• ✅ Ensure backward compatibility warnings are visible

📊 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5)
Security: ⭐⭐⭐ (3/5) - Example code demonstrates insecure pattern
Test Coverage: ⭐⭐⭐ (3/5) - Missing error handling tests
Documentation: ⭐⭐⭐⭐⭐ (5/5) - Excellent deprecation notices and migration guide

✅ Approval Recommendation

APPROVE with minor changes requested

This is a well-executed migration that improves performance and reduces backend dependencies. The breaking changes are clearly communicated, and the deprecation strategy is sound.

Address the security warning prominence and add error handling tests before merging to production.

---

Review generated with assistance from Claude Code

[claude]

Code Review: Migration to Direct Pinata Uploads

Summary

This PR migrates from backend-mediated IPFS pinning to client-side Pinata uploads. Overall, the implementation is solid and well-documented, with proper deprecation notices and comprehensive tests. However, there are several areas that need attention before merging.

---

🔴 Critical Issues

1. Security Exposure in Example App (apps/nextjs-template/src/components/intuition/IntuitionCreateThingButton.tsx:34)

const pinataApiJWT = process.env.NEXT_PUBLIC_PINATA_API_JWT

Issue: While there's a warning comment, the example code directly exposes the API key in the browser. This is dangerous because:

• Developers often copy example code directly
• The warning might be overlooked
• This could lead to production security incidents

Recommendation:

• Consider removing this example entirely, or
• Replace with a placeholder that forces developers to implement it properly:

// IMPORTANT: Do NOT use NEXT_PUBLIC_ environment variables for API keys
// Implement a server-side API route instead. See documentation for details.
throw new Error('This example requires implementing a secure server-side upload endpoint')

2. Missing Validation in Tests (packages/sdk/tests/external.test.ts:20-23)

expect(data.IpfsHash).toEqual(
  'QmYiJdC6KbkUuKVYeuQLm3DRbaok3h6yPyJjPqas8cotqx',
)

Issue: This test expects a hardcoded IPFS hash, which suggests it's hitting a real API. This will fail if:

• The same data returns a different hash
• Pinata is unavailable
• Rate limits are hit

Recommendation: Mock the uploadJsonToPinata function:

vi.mock('../src/external/upload-json-to-pinata', () => ({
  uploadJsonToPinata: vi.fn().mockResolvedValue({
    IpfsHash: 'QmYiJdC6KbkUuKVYeuQLm3DRbaok3h6yPyJjPqas8cotqx',
    // ... other fields
  })
}))

---

⚠️ High Priority Issues

3. Error Handling - Array Index Risk (packages/sdk/src/core/create-atom-from-thing.ts:54)

return {
  uri: uriRef,
  transactionHash: txHash,
  state: events[0].args,  // 🔴 Potential crash if empty
}

Issue: If events array is empty, this will throw an undefined error.

Recommendation:

if (!events || events.length === 0) {
  throw new Error('No AtomCreated events found in transaction')
}
return {
  uri: uriRef,
  transactionHash: txHash,
  state: events[0].args,
}

Same issue exists in:

• create-atom-from-ipfs-upload.ts:55

4. Inconsistent Error Messages (packages/sdk/src/api/pin-thing.ts:28)

console.error('Error fetching triple:', error)  // 🔴 Wrong entity type

Issue: Error message says "triple" but function is pinThing.

Recommendation: Update to 'Error pinning thing:'

5. Missing Rate Limiting Documentation

The migration introduces direct Pinata API calls from potentially many clients. The documentation should mention:

• Pinata rate limits (free tier: 100 requests/month)
• Best practices for batching
• Retry strategies
• Cost implications

---

💡 Medium Priority Suggestions

6. Type Safety Enhancement

The uploadJsonToPinata function accepts unknown for jsonData parameter. Consider adding validation:

export async function uploadJsonToPinata(
  apiToken: string,
  jsonData: unknown,
): Promise<PinataPinResponse> {
  // Validate that jsonData is JSON-serializable
  if (jsonData === undefined || typeof jsonData === 'function' || typeof jsonData === 'symbol') {
    throw new Error('Invalid JSON data: data must be JSON-serializable')
  }
  // ... rest of implementation
}

7. Parallel Upload Error Context (packages/sdk/src/core/batch-create-atoms-from-things.ts:37-55)

The error messages include itemPreview which is good, but consider also logging successful uploads count:

try {
  const uris = await Promise.all(uploadPromises)
} catch (error) {
  throw new Error(
    `Batch upload failed. Successfully uploaded: ${uploadPromises.length} items. ${error.message}`
  )
}

8. Deprecation Timeline Missing

The deprecation notices don't mention:

• When will the old API be removed?
• Is there a sunset date for the GraphQL pinThing mutation?
• Will there be a breaking major version bump?

Recommendation: Add to CHANGELOG:

### Deprecations
- **DEPRECATED (v3.0.0 removal planned):** The `pinThing` GraphQL mutation...

---

✅ Strengths

1. Excellent documentation: The security warning in the Next.js template is comprehensive
2. Good test coverage: Error cases are well covered in external.test.ts
3. Proper TypeScript types: CreateAtomConfigWithIpfs extends properly
4. Clear migration path: The PR description provides before/after examples
5. Parallel uploads: batchCreateAtomsFromThings efficiently parallelizes Pinata uploads
6. Comprehensive error messages: Good context in batch upload errors with item previews

---

📋 Test Plan Checklist Review

Looking at the test plan in the PR description:

• ✅ Test coverage exists for error handling
• ⚠️ Missing: Integration test showing createAtomFromThing with real config
• ⚠️ Missing: Test for batchCreateAtomsFromThings functionality
• ⚠️ Missing: Validation that deprecation warnings appear in console

---

🔒 Security Assessment

Overall: Medium Risk

1. ✅ No SQL injection vectors
2. ✅ No XSS vectors (IPFS hashes are validated)
3. ⚠️ API key exposure risk in example code (addressed above)
4. ✅ No obvious command injection
5. ⚠️ Potential DoS if rate limiting not documented
6. ✅ HTTPS used for Pinata API

---

📊 Performance Considerations

Positive:

• Parallel uploads in batch function ✅
• Removes backend as bottleneck ✅
• Direct to Pinata reduces latency ✅

Concerns:

• Client-side rate limiting not implemented
• No retry logic for failed uploads
• Browser environment limitations (no connection pooling)

---

Recommendations Summary

Must Fix Before Merge:

1. ✅ Fix or remove the insecure example code
2. ✅ Add array bounds checking in create-atom-from-thing.ts
3. ✅ Mock external API calls in tests
4. ✅ Fix error message in pin-thing.ts

Should Fix:

5. Add rate limiting documentation
6. Add deprecation timeline
7. Enhance type validation in uploadJsonToPinata

Nice to Have:

8. Add integration tests
9. Consider retry logic for transient failures
10. Document Pinata costs and limits

---

Conclusion

This is a well-structured migration that improves performance and reduces backend dependencies. The code quality is high, with good error handling and documentation. However, the security example and test mocking issues should be addressed before merging.

Recommendation: Request changes for critical issues, then approve.

---

Review conducted following TypeScript strict mode, functional programming principles, and Intuition Protocol conventions.