-
Notifications
You must be signed in to change notification settings - Fork 8
[LC-1423] X-API Scoped by Contract #900
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: 7be34ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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.
|
✅ Deploy Preview for staging-learncardapp canceled.
|
|
🥷 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: ✨ Comment |
|
@claude please review and provide a recomendation on merging |
|
Claude finished @Custard7's task —— View job Code Review Complete ✅I've reviewed this PR for X-API contract scoping and here's my comprehensive assessment: Review Tasks
Code Review Findings✅ Strong Points
🔍 Addressing the Review Comment@TaylorBeeston regarding the VP context question: The implementation doesn't modify the VP context - it's much cleaner than that! Here's what actually happens:
Why use DidAuthVP type? The
🚦 Minor Considerations
📊 Security & Best Practices
Merge Recommendation: ✅ APPROVEThis PR is ready to merge based on:
The approach is sound - embedding the contract URI in the VP before signing ensures it's cryptographically protected and enables proper statement scoping for ConsentFlow workflows. |
smurflo2
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.
LGTM!
Overview
🎟 Relevant Jira Issues
[LC-1423] Port xAPI activity logs to the new ConsentFlow design
📚 What is the context and goal of this PR?
In order to query X-API statements scoped by contract, we need to tag them!
🥴 TL; RL:
Tags X-API statements with a contract URI by adding the contract URI to the VP JWT
💡 Feature Breakdown (screenshots & videos encouraged!)
🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
Easiest way is to just run the tests! =) The E2E tests will check the new JWT format and test correctly
tag statements
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Wrote E2E tests
Documentation
I had Claude update the docs =)
📝 Documentation Checklist
User-Facing Docs (
docs/→ docs.learncard.com)docs/tutorials/)docs/how-to-guides/)docs/sdks/)docs/core-concepts/)docs/apps/)Internal/AI Docs
Visual Documentation
- Mermaid diagram — Complex flow, state machine, or architecture
B[LearnCard] --> C[Network]