feat(ack-pay): add approval model types#96
Conversation
WalkthroughThe PR introduces a minimal approval model for ACK-Pay consisting of PaymentApprovalRequest and PaymentApprovalDecision types. These contracts are exported from the package, implemented with Valibot/Zod schemas, covered by tests, and documented in the HITL guide with examples. ChangesApproval Request and Decision Model
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05ddd97b3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * ACK-Pay does not prescribe an approval workflow. This type gives applications | ||
| * and demos a shared object shape for approval-required paths. | ||
| */ | ||
| export interface PaymentApprovalRequest { |
There was a problem hiding this comment.
Because this introduces new exported ACK-Pay model types, the repo’s schema contract requires matching Valibot and Zod v3/v4 schemas for new types. As written, applications receiving these approval objects over the wire can import only TypeScript interfaces, so runtime validation through @agentcommercekit/ack-pay/schemas/* is unavailable and inconsistent with existing payment request/receipt models; please add schemas for both PaymentApprovalRequest and PaymentApprovalDecision.
Useful? React with 👍 / 👎.
Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ack-pay/hitl.mdx`:
- Line 48: The example's expiresAt field is stale (currently
"2026-01-01T12:00:00.000Z"); update the expiresAt value in the sample to a
future ISO8601 timestamp (e.g., a year or more ahead) or replace it with a
clearly labeled placeholder like expiresAt: "<FUTURE_ISO_TIMESTAMP>" so readers
copying the snippet get a valid approval window; locate and change the expiresAt
entry in the sample block where expiresAt is set.
🪄 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: 6240cde5-1695-4cca-8ab1-ff9ff2f15959
📒 Files selected for processing (4)
.changeset/add-approval-types.mddocs/ack-pay/hitl.mdxpackages/ack-pay/src/index.tspackages/ack-pay/src/payment-approval.ts
Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
|
Addressed the stale approval expiry example in 5a358db by replacing the fixed past timestamp with |
|
Addressed the runtime-schema review item in a small follow-up commit. Changes:
Verification:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ack-pay/src/schemas/valibot.ts`:
- Around line 46-60: The expiresAt and decidedAt fields in
paymentApprovalDecisionSchema currently accept any string; update them to use
the same normalized timestamp validator used by paymentRequestSchema (i.e.,
replace raw v.string() with the v.pipe(v.union([v.date(), v.string()]) +
v.transform(...) pattern used there) and keep expiresAt optional and decidedAt
required as before; locate paymentApprovalDecisionSchema and change the types
for the expiresAt and decidedAt fields to match the paymentRequestSchema
timestamp normalization implementation.
In `@packages/ack-pay/src/schemas/zod/v3.ts`:
- Around line 41-55: The approval schemas accept raw strings for timestamps;
align them with the existing paymentRequestSchema pattern by replacing
paymentApprovalRequestSchema.expiresAt and
paymentApprovalDecisionSchema.decidedAt validation with the same union([date(),
string()]).transform(...) timestamp pattern used in paymentRequestSchema so both
expiresAt and decidedAt consistently accept Date or ISO string and normalize to
a Date (or desired type) during parsing; update the validators in v3.ts (and
mirror changes in v4.ts/valibot.ts) by locating paymentApprovalRequestSchema and
paymentApprovalDecisionSchema and applying the union/date+string transform used
by paymentRequestSchema.
In `@packages/ack-pay/src/schemas/zod/v4.ts`:
- Around line 41-55: The expiresAt and decidedAt fields in
paymentApprovalDecisionSchema currently accept any string; update both to use
the same robust date validation used by paymentRequestSchema (lines 22–25) —
replace z.string().optional() for expiresAt and z.string() for decidedAt with
the identical z.preprocess/refine pattern used in paymentRequestSchema so they
only accept valid ISO date strings (and keep optionality for expiresAt). Locate
paymentApprovalDecisionSchema, paymentApprovalDecisionValueSchema, and the
paymentRequestSchema date validation pattern and apply that exact validation
logic to expiresAt and decidedAt.
🪄 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: 4f013606-abc2-4d87-a8d6-0f9c825d2a67
📒 Files selected for processing (4)
packages/ack-pay/src/payment-approval.test.tspackages/ack-pay/src/schemas/valibot.tspackages/ack-pay/src/schemas/zod/v3.tspackages/ack-pay/src/schemas/zod/v4.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ack-pay/src/payment-approval.test.ts
|
Follow-up for the timestamp consistency review is pushed. Changes:
Verification:
|
| * ACK-Pay does not prescribe an approval workflow. This type gives applications | ||
| * and demos a shared object shape for approval-required paths. | ||
| */ | ||
| export interface PaymentApprovalRequest { |
There was a problem hiding this comment.
Design question: PaymentApprovalRequest has 5 of 7 fields optional.
At this level of optionality, does the type provide enough structure to be useful as a shared contract?
Integrators with strict approval requirements (e.g., mandatory requesterDid + expiresAt for compliance) would need to narrow these anyway, and integrators with loose requirements might just use their own ad-hoc shapes.
Curious if you had specific integration patterns in mind that guided the required vs. optional choices.
| export * from "./errors" | ||
| export * from "./create-signed-payment-request" | ||
| export * from "./verify-payment-request-token" | ||
| export * from "./payment-approval" |
There was a problem hiding this comment.
This is the line that makes the approval types part of the published API surface. Once released, removing or changing these types is a breaking change. That's the main reason I'd prefer to validate the shapes against a real consumer before committing them here.
|
|
||
| const urlOrDidUri = v.union([v.pipe(v.string(), v.url()), didUriSchema]) | ||
|
|
||
| const timestampSchema = v.pipe( |
There was a problem hiding this comment.
This refactoring is a nice win regardless of the approval types discussion .. extracting timestampSchema removes duplication from paymentRequestSchema.expiresAt.
Would you be open to splitting this into its own PR?
|
Appreciate the thoroughness here, @EfeDurmaz16 .. I'd say the timestamp schema extraction alone is a nice cleanup and I think this honestly could've been it's own PR. That said, I'm not sure about merging this into the core The current HITL docs describe approval conceptually without prescribing types. Before we commit these shapes to the published package, I'd like to align on whether that was intentional or a gap we want to fill. Also, the types don't have a consumer yet. PR #97 is standalone and doesn't import these types. Until we have a concrete use case that validates the shape, publishing them risks locking in a contract that doesn't fit real integrations. Most fields are optional, which undercuts the value. Consumers would likely need to extend or wrap these anyway. The I'd suggest we defer the approval types until the model solidifies. Happy to open a design discussion issue to work through whether they belong in core, a separate package, or as documented examples. The timestampSchema extraction is genuinely useful on its own though. Would you be open to splitting that into a standalone PR? |
|
That makes sense to me. I agree we should avoid locking approval request/decision shapes into the published I split the timestamp schema extraction into a standalone smaller PR here: #104 I will close this PR to reduce review load. The approval model can come back later through a design discussion or a concrete consumer-driven PR. |
2 participants
Summary
Fixes #93
Verification
AI Usage Disclosure
This contribution was AI-assisted using Codex CLI and the Codex app. AI assistance was used for repository/docs navigation, understanding ACK/Catena context, identifying relevant issues or contribution areas, and assisting with small edits/validation. I reviewed the final diff and take responsibility for the submitted changes.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores