feat: bump Starknet JSON-RPC spec to v0.9.0-rc.2 #311
Conversation
|
Claude finished @kariy's task —— View job PR Review: Starknet JSON-RPC spec v0.9.0-rc.2 UpdateReview Checklist:
Review SummaryCompleted comprehensive review of RPC v0.9.0-rc.2 dependency updates. Key Findings:
Main Recommendations:
Security: Low risk - all dependencies from trusted sources Overall: Necessary update that appears technically sound but requires careful monitoring due to reliance on development branches and personal forks. Full detailed review has been submitted to the PR with specific recommendations for each concern identified. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive Review: Starknet JSON-RPC v0.9.0-rc.2 Update
Summary
This PR updates dependencies to align with Starknet JSON-RPC spec v0.9.0-rc.2. The changes primarily involve updating several core dependencies from published crates to Git-based development branches.
Detailed Analysis
✅ Positive Aspects:
- Coordinated Update: All related dependencies (starknet, katana, account_sdk, cainome) are updated together
- Specific Commits: Git dependencies use specific revisions/branches for reproducibility
- Clear Intent: The PR title clearly indicates the RPC spec version being targeted
⚠️ Concerns & Recommendations:
1. Dependency Stability (High Priority)
- Issue: Multiple dependencies switched from published crates to Git branches/revisions
starknet:"0.14.0"→ Git revisioncainome-cairo-serde:"0.2.0"→ Git branchaccount_sdk: Different Git repo/branch
- Risk: Reduced stability, potential for upstream changes, harder dependency management
- Recommendation: Add comments explaining why Git dependencies are needed and timeline for returning to published versions
2. Repository Ownership Change (Medium Priority)
- Issue:
account_sdkmoved fromcartridge-gg/controllertokariy/controller-rs - Risk: Personal fork dependency could become unmaintained
- Recommendation: Verify this is temporary and plan to return to the main repository
3. Development Branch Dependencies (Medium Priority)
- Issue: Multiple branches in active development (
rpc/v0.9,feat/rpc0.9,snip12-rpc0.9) - Risk: Breaking changes could be introduced upstream without notice
- Recommendation: Pin to specific commit hashes once branches stabilize
4. Testing Requirements (High Priority)
- Missing: No evidence of comprehensive testing with new dependencies
- Recommendation:
- Run full test suite with new dependencies
- Test RPC compatibility with external services
- Validate existing functionality still works
5. Documentation Gap
- Missing: No description of what changes users can expect
- Recommendation: Document any breaking changes or new features from RPC v0.9.0-rc.2
🔍 Files Requiring Attention:
Cargo.toml (Line 26):
starknet = { git = "https://github.com/xJonathanLEI/starknet-rs", rev = "993a708551f87bcfbf7fdf33f796b01f7521a634" }Consider adding a comment explaining why this Git dependency is necessary.
slot/Cargo.toml (Lines 30-31):
account_sdk = { git = "https://github.com/kariy/controller-rs", branch = "feat/rpc0.9" }
cainome-cairo-serde = { git = "https://github.com/kariy/cainome", branch = "snip12-rpc0.9" }Monitor these personal forks and plan migration path back to official repositories.
🔒 Security Assessment:
- Low Risk: All dependencies are from known, trusted sources
- Monitor: Personal forks should be reviewed for any unauthorized changes
- Validate: Ensure Git revisions haven't been tampered with
✅ Next Steps:
- Test thoroughly: Run full test suite and E2E tests
- Document changes: Add changelog entry for RPC spec update
- Monitor upstream: Track when stable versions become available
- Plan migration: Create timeline for returning to published crates
Overall Assessment:
This is a necessary update to support the new RPC spec, but the reliance on development branches and personal forks introduces some risk. The changes appear technically sound but require careful monitoring and testing.
|
Claude finished @kariy's task —— View job PR Review: Starknet JSON-RPC spec v0.9.0-rc.2 UpdateReview Checklist:
Review SummaryCompleted comprehensive review of RPC v0.9.0-rc.2 dependency updates. Key Findings:
Main Recommendations:
Security: Low risk - all dependencies from trusted sources Overall: Necessary update that appears technically sound but requires careful monitoring due to reliance on development branches and personal forks. Full detailed review has been submitted to the PR with specific recommendations for each concern identified. |
There was a problem hiding this comment.
Comprehensive Review: Starknet JSON-RPC v0.9.0-rc.2 Update
Summary
This PR updates dependencies to support Starknet JSON-RPC spec v0.9.0-rc.2. While the changes are necessary for RPC compatibility, several dependency management practices introduce risks that should be addressed.
🔴 Critical Issues
1. Personal Fork Dependencies (High Risk)
Files: slot/Cargo.toml:42-43
account_sdk = { git = "https://github.com/kariy/controller-rs", branch = "feat/rpc0.9" }
cainome-cairo-serde = { git = "https://github.com/kariy/cainome", branch = "feat/rpc0.9" }Issues:
- Using personal forks (
kariy/*) instead of official repositories - Personal forks may become unmaintained or diverge from upstream
- Introduces trust concerns - need to verify no unauthorized changes
- No clear migration path back to official repositories
Recommendations:
- Add TODO comments with timeline for reverting to official repos
- Document why these forks are necessary (what specific changes do they contain?)
- Consider contributing changes upstream to official repositories
2. Development Branch Dependencies (Medium-High Risk)
Files: Multiple Cargo.toml files
torii-cli = { git = "...", branch = "feat/rpc0.9" }
katana-primitives = { git = "...", branch = "rpc/v0.9" }
katana-cli = { git = "...", branch = "rpc/v0.9" }Issues:
- Multiple dependencies on active development branches
- Branches can receive breaking changes without notice
- Reduced build reproducibility
- Potential for upstream force-pushes or branch deletions
Recommendations:
- Pin to specific commit hashes once branches stabilize
- Monitor upstream repositories for stable releases
- Add comments explaining why git dependencies are necessary
🟡 Medium Priority Issues
3. Version Coordination Risk
Issue: Large version jump from stable releases to release candidates
starknetupdated to0.17.0-rc.2(likely from a much older stable version)- Multiple breaking changes likely included
- No migration guide or changelog referenced
Recommendations:
- Test thoroughly with existing codebase
- Review breaking changes in starknet 0.17.0-rc.2 changelog
- Update any deprecated API usage
4. Missing Test Coverage Documentation
Issue: No indication of testing performed with new dependencies
- Large dependency updates should include comprehensive testing
- E2E tests should verify RPC compatibility
- No mention of performance impact assessment
Recommendations:
- Run full test suite:
./scripts/e2e.sh - Verify RPC compatibility with external services
- Document any performance changes observed
🟢 Positive Aspects
✅ Coordinated Update: All related dependencies updated together
✅ Clear Intent: PR title clearly indicates RPC spec version target
✅ Existing Review: Previous comprehensive review already conducted
🔒 Security Assessment
- Low-Medium Risk: Dependencies from known sources but using personal forks
- Recommended Actions:
- Verify kariy's forks haven't introduced unauthorized changes
- Audit any new dependencies pulled in by version updates
- Monitor for security advisories on new dependency versions
📋 Action Items
Before Merge:
- Test thoroughly - Run
./scripts/e2e.shand full test suite - Document changes - Add comments explaining git dependency necessity
- Plan migration - Create timeline for returning to stable releases
- Verify forks - Audit personal fork changes vs upstream
Post-Merge Monitoring:
- Track upstream - Monitor official repos for stable v0.9.0 releases
- Performance watch - Monitor for any performance regressions
- Security updates - Watch for security advisories on new versions
Overall Assessment
This dependency update is necessary for RPC v0.9.0 support but introduces moderate risk through unstable dependency practices. The changes appear technically sound but require careful monitoring and testing before production deployment.
Recommendation: Approve with conditions - address dependency stability concerns and ensure comprehensive testing is completed.
|
Claude finished @kariy's task —— View job PR Review: Starknet JSON-RPC spec v0.9.0-rc.2 UpdateReview Checklist:
Comprehensive Code Review🔴 Critical Issues1. Personal Fork Dependencies (High Risk)File: account_sdk = { git = "https://github.com/kariy/controller-rs", branch = "feat/rpc0.9" }Issues:
Recommendation: Document why this fork is necessary and create migration plan. 2. Development Branch Dependencies (Medium-High Risk)Files: torii-cli = { git = "https://github.com/dojoengine/torii", branch = "feat/rpc0.9" }
katana-primitives = { git = "https://github.com/dojoengine/katana", branch = "rpc/v0.9" }
katana-cli = { git = "https://github.com/dojoengine/katana", branch = "rpc/v0.9" }Issues:
Recommendation: Pin to specific commit hashes once branches stabilize. 3. Release Candidate in Workspace (Medium Risk)File: starknet = "0.17.0-rc.2"Issues:
Recommendation: Monitor for stable release and upgrade promptly.
|
There was a problem hiding this comment.
Summary
Comprehensive review completed for the Starknet JSON-RPC v0.9.0-rc.2 dependency updates. This is a necessary update but introduces several stability and security concerns through the use of development branches and personal forks.
Key Findings:
⚠️ High Risk: Personal fork dependency (kariy/controller-rs) requires monitoring⚠️ Medium-High Risk: Multiple development branch dependencies reduce stability⚠️ Medium Risk: Release candidate version may introduce breaking changes
Primary Recommendations:
- Test thoroughly with
./scripts/e2e.shbefore merge - Document migration timeline for returning to stable dependencies
- Monitor upstream repositories for stable releases
- Verify personal fork hasn't introduced unauthorized changes
The update appears technically necessary for RPC compatibility but requires careful monitoring and comprehensive testing before production deployment.
|
Claude finished @kariy's task —— View job PR Review: Starknet JSON-RPC spec v0.9.0-rc.2 UpdateReview Checklist:
Comprehensive Code ReviewCompleted comprehensive review of RPC v0.9.0-rc.2 dependency updates. 🎉 Major Improvements IdentifiedThis PR has significantly improved since previous reviews, addressing critical security concerns: ✅ Key Fixes:
Remaining RecommendationsLow-Medium Priority:
Overall AssessmentExcellent progression! The dependency management has evolved from high-risk (personal forks, development branches) to low-risk, well-managed approach. This demonstrates strong improvement in software supply chain security practices. Recommendation: Ready for approval after comprehensive testing verification. Full detailed review submitted to PR with specific technical recommendations. |
There was a problem hiding this comment.
Comprehensive Code Review: Starknet JSON-RPC v0.9.0-rc.2 Update
🎉 Major Improvements Since Previous Reviews
This PR has significantly improved since earlier iterations, addressing the primary security and stability concerns:
✅ Security & Dependency Management Wins:
- Personal Fork Eliminated:
account_sdknow uses officialcartridge-gg/controller-rs(previously problematickariy/controller-rs) - Official Sources:
cainome-cairo-serdefrom officialcartridge-gg/cainome - Reproducible Builds: Both use specific commit hashes (
rev) instead of branches - Trust Chain Restored: All major dependencies from verified official sources
⚠️ Remaining Considerations (Manageable Risk)
1. Development Branch Dependencies (Medium Impact)
Files: cli/Cargo.toml:19-25
torii-cli,katana-primitives,katana-clistill use development branches- Risk: Potential for upstream changes affecting builds
- Mitigation: Pin to commit hashes when branches stabilize
2. Release Candidate Usage (Low-Medium Impact)
File: Cargo.toml:26 - starknet = "0.17.0-rc.2"
- Acceptable: Necessary for RPC v0.9.0 support
- Action: Upgrade to stable 0.17.0 when available
3. Testing Requirements (High Priority)
- Critical: Run comprehensive E2E tests (
./scripts/e2e.sh) - Validate: RPC compatibility, service deployments, authentication flow
- Evidence: No indication of testing performed with new dependencies
🔒 Security Assessment: LOW RISK
Previous high-risk personal fork dependencies eliminated. Current setup uses trusted sources with specific commit hashes, significantly improving supply chain security.
📋 Recommended Actions
Before Merge:
- Test thoroughly: Run
./scripts/e2e.shand full test suite - Verify functionality: Test Katana/Torii deployments and authentication
- Performance check: Verify no critical regressions
Post-Merge:
- Monitor upstream: Track stable releases for development branch dependencies
- Plan migration: Timeline for moving to published crates when available
Final Recommendation
Strong improvement! This PR successfully addresses the major concerns from previous reviews while maintaining necessary RPC v0.9.0 compatibility. The dependency management is now much more secure and maintainable.
Recommend approval after comprehensive testing verification.
1 participant
No description provided.