Add AsterPay KYA Trust Score demo for ACK-ID integration#75
Open
petteri74dev wants to merge 15 commits intoagentcommercekit:mainfrom
Open
Add AsterPay KYA Trust Score demo for ACK-ID integration#75petteri74dev wants to merge 15 commits intoagentcommercekit:mainfrom
petteri74dev wants to merge 15 commits intoagentcommercekit:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughA new demo package for AsterPay KYA Trust Score integration is added, featuring JWT-to-W3C Verifiable Credential conversion, ACK-ID verification, and ERC-8183 IACPHook-style gating. The demo includes cryptographic operations, trust-score validation, and an end-to-end CLI workflow with configurable security thresholds. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
demos/asterpay-kya/vitest.config.ts (1)
4-6: Avoid a green test run with zero assertions.
passWithNoTests: truelets this package report success even if none of the new JWT/VC round-trip or gate-rejection paths are exercised. I'd add at least one colocated smoke test for create → convert → verify plus a blocked case, then drop this flag so regressions surface in CI. Based on learnings, test files must be co-located with source code as *.test.ts files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/asterpay-kya/vitest.config.ts` around lines 4 - 6, Update the Vitest config to stop allowing empty test runs by removing or setting test.passWithNoTests to false, and add at least one colocated smoke test file (a *.test.ts next to the related source) that exercises the create → convert → verify happy path and one gate-rejection/blocked case so CI fails on regressions; locate the test configuration under the test object in vitest.config.ts (property name passWithNoTests) and add tests co-located with the source implementing the round-trip and rejection scenarios.demos/asterpay-kya/src/index.ts (1)
235-235: MakeCOMPLYa real policy check, or rename it.The job advertises a 50 USDC budget, but
tierAuthorized = allPassedmeans this stage never inspectsvc.credentialSubject.tieror the budget. Right now the fifth shield is just a re-label of earlier gates, not a distinct tier-based authorization step.Also applies to: 296-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/asterpay-kya/src/index.ts` at line 235, COMPLY is currently a no-op because tierAuthorized is set to allPassed; either implement a real tier/budget check or rename the shield to reflect it's just repeating prior checks. Replace the allPassed assignment for tierAuthorized with a function that inspects vc.credentialSubject.tier and the job budget (e.g., verify vc.credentialSubject.tier >= requiredTier and that the credential holder is allowed the advertised 50 USDC budget), or if you don't want tier logic now, rename the COMPLY symbol to something like ALL_PASSED/REDUNDANT_COMPLIANCE and keep tierAuthorized = allPassed; update references to COMPLY and tierAuthorized accordingly so the intent is clear.demos/asterpay-kya/src/asterpay-kya-ack-id.ts (2)
195-219: Tight coupling between VC creation and JWT reconstruction.This function relies on the specific
jwsformat (header..signature) and non-standardoriginalPayloadfield created byconvertAsterPayKyaToVerifiableCredential. Consider adding a comment to document this coupling and that this reconstruction only works with VCs created by this module.📝 Suggested documentation
+/** + * Reconstructs the original KYA JWT from a synthetic VC. + * + * Note: This only works with VCs created by `convertAsterPayKyaToVerifiableCredential` + * as it relies on the non-standard `originalPayload` field in the proof. + */ export function convertVerifiableCredentialToAsterPayKya( vc: Verifiable<W3CCredential<AsterPayKyaCredentialSubject>>, ): JwtString {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/asterpay-kya/src/asterpay-kya-ack-id.ts` around lines 195 - 219, Document the tight coupling and limitations of convertVerifiableCredentialToAsterPayKya: add a clear comment near the function explaining it expects vc.proof.jws in the specific "header..signature" form and relies on a non-standard proof.originalPayload produced by convertAsterPayKyaToVerifiableCredential, and explicitly state this reconstruction is only guaranteed for VCs created by that module; also mention the thrown errors (missing jws, invalid format, missing originalPayload) so callers know the preconditions.
172-180: Consider making Coinbase KYC check configurable.The
coinbaseKyc.metcheck is a hard requirement. For flexibility, consider making this configurable via a parameter, similar tominTrustScore, allowing callers to decide which attestations are required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/asterpay-kya/src/asterpay-kya-ack-id.ts` around lines 172 - 180, The Coinbase KYC attestation is currently enforced unconditionally; update the verifier (the function in asterpay-kya-ack-id.ts that reads vc.credentialSubject and uses att = vc.credentialSubject.insumerAttestation) to accept a new optional parameter (e.g., requireCoinbaseKyc or requiredAttestations) with a sensible default (true to retain current behavior) and use that flag instead of hard-coding the check of att.coinbaseKyc.met; modify the conditional around att.coinbaseKyc.met to only return the failure object when the flag is set, and propagate this parameter through any callers (and tests) and document the new option similar to minTrustScore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/asterpay-kya/src/asterpay-kya-ack-id.ts`:
- Around line 144-148: The current issuer check only looks for the literal
string "did:web:api.asterpay.io" in trustedIssuers without validating the
credential's actual issuer; after calling
convertAsterPayKyaToVerifiableCredential and obtaining vc, replace the existing
check with logic that validates vc.issuer.id against the trustedIssuers list
(e.g., ensure vc?.issuer?.id exists and trustedIssuers.includes(vc.issuer.id))
and return invalid if it does not match; reference
convertAsterPayKyaToVerifiableCredential, vc, vc.issuer.id, and trustedIssuers
when making this change.
- Around line 89-96: The proof object currently uses type "JsonWebSignature2020"
with fields jws and originalPayload which is incompatible with the ACK verifier;
update the proof to use type "JwtProof2020" and replace the jws/originalPayload
fields with a single jwt field containing the full compact JWT
(header.payload.signature) so verifyProof
(packages/vc/src/verification/verify-proof.ts) can recognize it; keep created,
verificationMethod and proofPurpose unchanged and remove jws and originalPayload
from the proof object.
In `@demos/asterpay-kya/src/index.ts`:
- Around line 111-115: The catch blocks inside runDemo (the one that logs
errorMessage("Conversion failed") and the other similar catches at the spots
mentioned and around line 315) currently log and return, causing runDemo() to
resolve successfully on fatal errors; change them to rethrow the caught error
(e.g., throw error) after logging (or call process.exit(1) if you prefer an
immediate non-zero exit) so the top-level .catch(console.error) can observe the
failure; update the catch blocks in runDemo(), and the similar handlers
referenced (the conversion error catch and the other two catches) to either
rethrow the original error or call process.exit(1) instead of returning.
- Around line 289-293: The attestation gate is missing the country check: update
the attestPass boolean (used to decide COMPLY) to include
att.coinbaseCountry.met along with att.coinbaseKyc.met, att.gitcoinPassport.met,
and att.tokenBalance.met so a failed country check blocks approval; locate the
attestPass definition in index.ts (variable att from
vc.credentialSubject.insumerAttestation) and adjust it to require
att.coinbaseCountry.met, and ensure the log line that prints
KYC/Country/Passport/USDC reflects the enforced check.
In `@demos/asterpay-kya/src/kya-token.ts`:
- Around line 67-68: The mock entry with properties trustScore and tier is
inconsistent: trustScore is 82 but tier is "trusted"; update the mock in
kya-token.ts so the values align with the 4-tier mapping — either set tier to
"enterprise" to match trustScore: 82, or lower trustScore into the 50–74 range
if "trusted" is intended; edit the object that defines trustScore and tier to
make them consistent.
---
Nitpick comments:
In `@demos/asterpay-kya/src/asterpay-kya-ack-id.ts`:
- Around line 195-219: Document the tight coupling and limitations of
convertVerifiableCredentialToAsterPayKya: add a clear comment near the function
explaining it expects vc.proof.jws in the specific "header..signature" form and
relies on a non-standard proof.originalPayload produced by
convertAsterPayKyaToVerifiableCredential, and explicitly state this
reconstruction is only guaranteed for VCs created by that module; also mention
the thrown errors (missing jws, invalid format, missing originalPayload) so
callers know the preconditions.
- Around line 172-180: The Coinbase KYC attestation is currently enforced
unconditionally; update the verifier (the function in asterpay-kya-ack-id.ts
that reads vc.credentialSubject and uses att =
vc.credentialSubject.insumerAttestation) to accept a new optional parameter
(e.g., requireCoinbaseKyc or requiredAttestations) with a sensible default (true
to retain current behavior) and use that flag instead of hard-coding the check
of att.coinbaseKyc.met; modify the conditional around att.coinbaseKyc.met to
only return the failure object when the flag is set, and propagate this
parameter through any callers (and tests) and document the new option similar to
minTrustScore.
In `@demos/asterpay-kya/src/index.ts`:
- Line 235: COMPLY is currently a no-op because tierAuthorized is set to
allPassed; either implement a real tier/budget check or rename the shield to
reflect it's just repeating prior checks. Replace the allPassed assignment for
tierAuthorized with a function that inspects vc.credentialSubject.tier and the
job budget (e.g., verify vc.credentialSubject.tier >= requiredTier and that the
credential holder is allowed the advertised 50 USDC budget), or if you don't
want tier logic now, rename the COMPLY symbol to something like
ALL_PASSED/REDUNDANT_COMPLIANCE and keep tierAuthorized = allPassed; update
references to COMPLY and tierAuthorized accordingly so the intent is clear.
In `@demos/asterpay-kya/vitest.config.ts`:
- Around line 4-6: Update the Vitest config to stop allowing empty test runs by
removing or setting test.passWithNoTests to false, and add at least one
colocated smoke test file (a *.test.ts next to the related source) that
exercises the create → convert → verify happy path and one
gate-rejection/blocked case so CI fails on regressions; locate the test
configuration under the test object in vitest.config.ts (property name
passWithNoTests) and add tests co-located with the source implementing the
round-trip and rejection scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56a11b75-20db-4bf9-8f26-b3b75db16a43
📒 Files selected for processing (10)
demos/README.mddemos/asterpay-kya/README.mddemos/asterpay-kya/package.jsondemos/asterpay-kya/src/asterpay-kya-ack-id.tsdemos/asterpay-kya/src/index.tsdemos/asterpay-kya/src/jwk-keys.tsdemos/asterpay-kya/src/kya-token.tsdemos/asterpay-kya/tsconfig.jsondemos/asterpay-kya/vitest.config.tspackage.json
There was a problem hiding this comment.
🧹 Nitpick comments (2)
demos/asterpay-kya/src/asterpay-kya-ack-id.ts (1)
38-42: Unused variablejwtSignature.The
jwtSignatureis extracted but never used. This appears to be leftover code from the previousJsonWebSignature2020proof format that included ajwsfield.🧹 Proposed cleanup
const jwtParts = kyaToken.split(".") if (jwtParts.length !== 3) { throw new Error("Invalid JWT format") } - const jwtSignature = jwtParts[2]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/asterpay-kya/src/asterpay-kya-ack-id.ts` around lines 38 - 42, Remove the unused jwtSignature extraction: delete the jwtParts[2] assignment and, if you don't need the signature, also remove the jwtSignature variable and any related dead code; keep or adjust the existing JWT format validation around kyaToken/jwtParts as needed. Specifically, update the logic around the kyaToken variable and the jwtParts split (used in the current function) so there are no unused variables (jwtSignature) left in the scope and no references to the old JsonWebSignature2020 `jws` behavior.demos/asterpay-kya/src/index.ts (1)
257-257: Consider returning the VC from verification to avoid redundant conversion.The token is converted twice: once in
verifyAsterPayKyaAsAckId(line 242-247) and again here (line 257). For demo purposes this is acceptable, but if this pattern were used in production, consider havingverifyAsterPayKyaAsAckIdreturn the converted VC when valid to avoid the duplicate JWT verification overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/asterpay-kya/src/index.ts` at line 257, The verifyAsterPayKyaAsAckId flow currently verifies the JWT and then you re-run conversion with convertAsterPayKyaToVerifiableCredential, causing duplicate work; change verifyAsterPayKyaAsAckId to return the already-converted Verifiable Credential (or an object containing both verification result and vc) instead of only a boolean, update the call site here to accept that returned vc (use it instead of calling convertAsterPayKyaToVerifiableCredential again), and adjust any other callers of verifyAsterPayKyaAsAckId to handle the new return shape (preserve existing error/validation behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@demos/asterpay-kya/src/asterpay-kya-ack-id.ts`:
- Around line 38-42: Remove the unused jwtSignature extraction: delete the
jwtParts[2] assignment and, if you don't need the signature, also remove the
jwtSignature variable and any related dead code; keep or adjust the existing JWT
format validation around kyaToken/jwtParts as needed. Specifically, update the
logic around the kyaToken variable and the jwtParts split (used in the current
function) so there are no unused variables (jwtSignature) left in the scope and
no references to the old JsonWebSignature2020 `jws` behavior.
In `@demos/asterpay-kya/src/index.ts`:
- Line 257: The verifyAsterPayKyaAsAckId flow currently verifies the JWT and
then you re-run conversion with convertAsterPayKyaToVerifiableCredential,
causing duplicate work; change verifyAsterPayKyaAsAckId to return the
already-converted Verifiable Credential (or an object containing both
verification result and vc) instead of only a boolean, update the call site here
to accept that returned vc (use it instead of calling
convertAsterPayKyaToVerifiableCredential again), and adjust any other callers of
verifyAsterPayKyaAsAckId to handle the new return shape (preserve existing
error/validation behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab378682-3133-45c4-8293-2a1f4196b914
📒 Files selected for processing (2)
demos/asterpay-kya/src/asterpay-kya-ack-id.tsdemos/asterpay-kya/src/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What's included
Key features
Test plan
Context
This is a resubmission of #63 from a new account. The previous account experienced GitHub flagging issues unrelated to this contribution.
AI Disclosure: This PR was developed with assistance from Claude (Anthropic).
Summary by CodeRabbit
New Features
Documentation
Chores