-
Notifications
You must be signed in to change notification settings - Fork 8
[LC-1472] Simplify Send Ergonomics #895
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
Conversation
🦋 Changeset detectedLatest commit: e389432 The changes in this PR will be included in the next version bump. This PR includes changesets to release 46 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for learncarddocs canceled.
|
|
👋 Hey there! It looks like you modified code, but didn't update the documentation in If this PR introduces new features, changes APIs, or modifies behavior that users or developers need to know about, please consider updating the docs. 🏄 Windsurf TipYou can ask Windsurf to help:
Windsurf will review your changes and suggest appropriate documentation updates based on what was modified. 📚 Documentation Guide
This is an automated reminder. If no docs are needed, feel free to ignore this message. |
✅ Deploy Preview for staging-learncardapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
✨ PR Review
The PR introduces a comprehensive ergonomic send method with proper error handling and security checks. The server-side implementation is well-structured with appropriate fallbacks and validation. However, there's a concerning type cast that bypasses type safety.
1 issues detected:
🧹 Maintainability - Unnecessary type assertion that bypasses type safety without clear justification 🛠️
Details: The code uses
as anyto bypass TypeScript's type checking when calling the boost.send.mutate method. Since SendBoostInput already includes an optional signedCredential field, this cast appears unnecessary and reduces type safety.
File:packages/plugins/learn-card-network/src/plugin.ts (1008-1008)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
@claude please review and provide a recommendation for merging |
|
Claude finished @Custard7's task —— View job PR Review: LC-1472 Simplify Send ErgonomicsI'll review this PR and provide a merge recommendation. Here's my analysis: Todo List:
🔍 Detailed Review✅ Architecture & DesignThe PR introduces a well-architected ergonomic send system:
✅ Implementation QualityStrong Points:
Areas for Improvement:
✅ Test CoverageExcellent test coverage across multiple layers: Unit Tests (
Unit Tests (
E2E Tests (
✅ Security AssessmentSecurity implementation is solid:
|
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.
✨ PR Review
The PR successfully implements an ergonomic send method for the LearnCard Network with good extensibility through discriminated unions. The implementation handles both client-side and server-side credential issuance appropriately, with proper contract integration for consent flow scenarios.
1 issues detected:
🐞 Bug - Direct property assignment on potentially null/undefined credentialSubject will cause runtime errors 🛠️
Details: The code directly assigns to boost.credentialSubject.id without checking if credentialSubject is null or undefined. While Array.isArray() returns false for null/undefined values, the subsequent property assignment will throw a runtime error if credentialSubject is falsy.
File:packages/plugins/learn-card-network/src/plugin.ts (997-997)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
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.
✨ PR Review
The PR introduces a well-designed ergonomic send method with comprehensive error handling and contract integration. The discriminated union pattern for extensibility is a good architectural choice.
1 issues detected:
🐞 Bug - Spreading null or undefined values causes runtime errors 🛠️
Details: The code spreads the credentialSubject object without checking if it's null or undefined. If credentialSubject is null/undefined, the spread operator will throw a TypeError at runtime.
File:services/learn-card-network/brain-service/src/routes/boosts.ts (675-675)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
🥷 Code experts: Custard7 Custard7, TaylorBeeston have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
@TaylorBeeston I got a bunch of errors when running the e2e tests. I fixed some that didn't sync over from the skill frameworks PR, but there were a bunch of others. No pic bc docker's super slow (/stuck?), but I'll add one when I can get it to run |
Custard7
left a comment
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.
All tests pass, and working for me - excellent implementation ✅ 🚀
Co-authored-by: gitstream-cm[bot] <111687743+gitstream-cm[bot]@users.noreply.github.com>
Overview
🎟 Relevant Jira Issues
LC-1472 Simplify read/write to consent flow contract API pattern
📚 What is the context and goal of this PR?
One of the challenges with integration right now is that it's just plain a pain to send credentials
through a contract! Especially if you're using a signing authority. This PR aims to create a single,
ergonomic send method that handles sending a credential gracefully for every possible scenario. This
means like if you want to send through a contract, you can, if you want to send a single VC, you can,
if you want to send a boost, you can, etc. without having to think through a lot of the steps involved,
just supply what you have! I also made this extensible using a discriminated union so that in the future
we can use for sending other things like VPs, etc!
🥴 TL; RL:
Adds a new generic
sendmethod that provides an ergonomic way to send credentials in the LearnCard Network. It handles boost template resolution, credential issuance (client-side when available, otherwise via signing authority), and consent flow integration automatically.sendmethod for ergonomic credential sendingRELATED_TOrelationship between new boosts and contracts💡 Feature Breakdown (screenshots & videos encouraged!)
New
sendAPI:Key features:
recipientaccepts either profileId or DIDissueCredentialis available in the LearnCard instance🛠 Important tradeoffs made:
z.discriminatedUnionas input validators, so we use separate validators:SendBoostInputValidator(ZodObject - TRPC compatible)SendInputValidator(discriminated union - for type extensibility)typefield is required (no default) due to discriminated union constraints🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
pnpm exec nx build typescd services/learn-card-network/brain-service && pnpm test -- --run -t "send" --no-coveragecd tests/e2e && pnpm test:e2e -- --run -t "Send"learnCard.invoke.send({ type: 'boost', recipient: '<profileId>', templateUri: '<boostUri>' })📱 🖥 Which devices would you like help testing on?
N/A - Backend/SDK changes only
🧪 Code Coverage
boosts.spec.tsfor send methodconsentflow.spec.tsfor contract integrationergonomic-send-boost.spec.tsDocumentation
📜 Gitbook
Documentation needed for the new
sendmethod API.📊 Storybook
N/A
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Add ergonomic
sendmethod to simplify boost credential issuance and delivery with automatic signing authority fallback and consent flow integration.Main changes:
sendmethod supporting template-based or on-the-fly boost credential creation with auto-issuanceRELATED_TOrelationship between newly created boosts and contracts for better traceabilitysendmethod usage and contract integration patternsGenerated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how