-
Notifications
You must be signed in to change notification settings - Fork 8
Release/0.12.1 #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release/0.12.1 #14
Conversation
SP-6529: EC certificates
* "@super-protocol/dto-js": "1.2.10" * "@super-protocol/sdk-js": "3.13.3"
Double GPU in orderCapacity fix
WalkthroughThe changes update package dependencies and versioning in the project configuration, and refactor certificate handling in the order report command. The new approach uses helper functions to formally construct and encode the certificate chain, replacing previous direct string concatenation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commands/ordersGetReport.ts (1)
39-46: Excellent refactoring to use formal certificate chain building.This is a significant improvement over simple string concatenation. The new approach properly:
- Parses certificates into PKI objects
- Builds a formal certificate chain
- Converts back to PEM format with proper encoding
However, consider adding error handling for the certificate operations, as
toPkiCerts,buildChain, andtoSchema().toBER()could potentially fail.Consider wrapping the certificate operations in a try-catch block:
- const pkiCerts = CertificatesHelper.toPkiCerts( - orderReport.certificate.concat(constants.SUPERPROTOCOL_CA), - ); - orderReport.certificate = CertificatesHelper.buildChain(pkiCerts[0], pkiCerts) - .map((certWithKeyIdent) => - CertificatesHelper.derToPem(certWithKeyIdent.cert.toSchema().toBER()), - ) - .join('\n'); + try { + const pkiCerts = CertificatesHelper.toPkiCerts( + orderReport.certificate.concat(constants.SUPERPROTOCOL_CA), + ); + orderReport.certificate = CertificatesHelper.buildChain(pkiCerts[0], pkiCerts) + .map((certWithKeyIdent) => + CertificatesHelper.derToPem(certWithKeyIdent.cert.toSchema().toBER()), + ) + .join('\n'); + } catch (error) { + Printer.error(`Failed to process certificate chain: ${error.message}`); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(2 hunks)src/commands/ordersGetReport.ts(2 hunks)
🔇 Additional comments (4)
package.json (2)
3-3: LGTM! Version bump follows semantic versioning.The version update from 0.11.10 to 0.12.1 correctly follows semantic versioning for a minor release.
38-39: LGTM! Dependency updates align with code changes.The SDK dependency updates enable the enhanced certificate handling functionality used in
ordersGetReport.ts. The version increments are appropriate and consistent with the new features being utilized.src/commands/ordersGetReport.ts (2)
3-8: LGTM! Clean import addition.The
CertificatesHelperimport is properly added to the existing import statement from the SDK.
42-42: Verify correct leaf certificate for chain building
The code on line 42 ofsrc/commands/ordersGetReport.tsuses the first element ofpkiCertsas the chain’s starting certificate:orderReport.certificate = CertificatesHelper.buildChain(pkiCerts[0], pkiCerts)• Confirm that
pkiCertsis always ordered with the end-entity (leaf) certificate at index 0.
• If the array order isn’t guaranteed, update the logic to select the leaf certificate dynamically (e.g. by matching subject/issuer or sorting by path length).
• Ensuring the correct starting certificate will prevent chain-building errors when intermediate or root certs appear first.
Summary by CodeRabbit
Chores
Bug Fixes