fix(app-data): add hook type, ease validation on dappId #589
Conversation
WalkthroughAdds AppData v1.9.0 and bumps hooks metadata to v0.3.0: new generated TypeScript types, new JSON schemas for hooks and root schema, updated generatedTypes exports, and corresponding test updates and constant bumps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
| }, | ||
| "dappId": { | ||
| "$id": "#/definitions/dappId", | ||
| "pattern": "^[a-fA-F0-9]{64}$", |
There was a problem hiding this comment.
This is no longer true since we moved to human readable URL format.
| "$ref": "../definitions.json#/definitions/dappId", | ||
| "title": "Id of the dApp which has built the hook", | ||
| "description": "Deprecated, will be removed in future versions. Use `type` field instead.", | ||
| "deprecated": true |
There was a problem hiding this comment.
dappId marked as deprecated
| "deprecated": true | ||
| }, | ||
| "type": { | ||
| "$ref": "../definitions.json#/definitions/hookType" |
There was a problem hiding this comment.
Added type field as the name is more explicit
📦 GitHub Packages PublishedLast updated: Oct 17, 2025, 11:02:03 AM UTC The following packages have been published to GitHub Packages with pre-release version
InstallationThese packages require authentication to install from GitHub Packages. First, create a # Create .npmrc file in your project root
echo "@cowprotocol:registry=https://npm.pkg.github.com" > .npmrc
echo "//npm.pkg.github.com/:_authToken=YOUR_GITHUB_TOKEN" >> .npmrcTo get your GitHub token:
Then install any of the packages above, either by exact version (i.e. # Yarn
yarn add npm:@cowprotocol/cow-sdk@pr-589
# pnpm
pnpm install npm:@cowprotocol/cow-sdk@pr-589
# NPM
npm install npm:@cowprotocol/cow-sdk@pr-589Update to the latest version (only if you used the tag)Every commit will publish a new package. To upgrade to the latest version, run: # Yarn
yarn upgrade @cowprotocol/cow-sdk
# pnpm
pnpm update @cowprotocol/cow-sdk
# NPM
npm update @cowprotocol/cow-sdkView PackagesYou can view the published packages at: https://github.com/cowprotocol/cow-sdk/packages |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/app-data/src/schemas/definitions.json (2)
51-56: dappId docs are now misleading; update title/description to reflect relaxed validation and deprecationSince the hex pattern was removed, "A hex string..." is inaccurate. Suggest clarifying and marking deprecated at the definition level.
"dappId": { "$id": "#/definitions/dappId", - "title": "A hex string representing a dApp ID", - "examples": ["75716a3cb48fdbb43ebdff58ce6c541f6a2c269be690513131355800367f2da2"], + "title": "dApp identifier", + "description": "Deprecated; replaced by `type` in hooks. Accepts arbitrary string for backward compatibility.", + "deprecated": true, + "examples": ["cow-swap", "legacy-hex-id-75716a3c..."], "type": "string" }
57-63: Prevent empty hookType valuesConsider enforcing a minimal length to avoid accidental empty strings while keeping it otherwise free-form.
"hookType": { "$id": "#/definitions/hookType", "title": "Hook Type", "description": "Non-typed identifier of a hook, usually in the form of a URL.", "examples": ["cow-swap://libs/hook-dapp-lib/permit", "cow-sdk://bridging/providers/socket"], - "type": "string" + "type": "string", + "minLength": 1 },packages/app-data/src/schemas/hooks/v0.3.0.json (1)
16-33: Quality-of-life: default empty arrays for pre/postDefaulting to [] reduces boilerplate and matches “empty hooks” semantics.
"pre": { "$id": "#/properties/hooks/pre", "title": "Pre-Hooks", "description": "CoW Hooks to call before an order executes", "type": "array", + "default": [], "items": { "$ref": "../hook/v0.3.0.json#" } }, "post": { "$id": "#/properties/hooks/post", "title": "Post-Hooks", "description": "CoW Hooks to call after an order executes", "type": "array", + "default": [], "items": { "$ref": "../hook/v0.3.0.json#" } }packages/app-data/test/schema.spec.ts (1)
1165-1237: Add a negative test to enforce additionalProperties: false on hook itemsEnsure unknown fields in hook objects are rejected.
Add:
test('Reject unknown fields in hook item', () => { const invalidDoc = { version: '1.9.0', metadata: { hooks: { pre: [ { target: '0x0102030405060708091011121314151617181920', callData: '0x01020304', gasLimit: '10000', type: 'cow-swap://libs/hook', unexpected: 'nope', }, ], }, }, } expect(validator(invalidDoc)).toBe(false) })packages/app-data/src/generatedTypes/v1.9.0.ts (1)
233-239: Surface deprecation in typings for better DXConsider configuring json-schema-to-typescript (or updating schema annotations) so CoWHook.dappId is emitted with a JSDoc @deprecated tag. This makes IDEs warn at call sites while keeping the field available.
If you want, I can check the generator version and options used to see if it supports emitting @deprecated from
deprecated: truein the schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/app-data/src/generatedTypes/index.ts(2 hunks)packages/app-data/src/generatedTypes/latest.ts(1 hunks)packages/app-data/src/generatedTypes/v1.9.0.ts(1 hunks)packages/app-data/src/schemas/definitions.json(1 hunks)packages/app-data/src/schemas/hook/v0.3.0.json(1 hunks)packages/app-data/src/schemas/hooks/v0.3.0.json(1 hunks)packages/app-data/src/schemas/v1.9.0.json(1 hunks)packages/app-data/test/schema.spec.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-10T03:06:46.146Z
Learnt from: jeffersonBastos
PR: cowprotocol/cow-sdk#327
File: packages/app-data/src/schemas/v0.3.0.json:1-10
Timestamp: 2025-06-10T03:06:46.146Z
Learning: Schema versions v0.1.0 through v0.7.0 in the app-data package are intentionally designed to allow additional properties (no "additionalProperties": false constraint), while versions v0.8.0 and later enforce strict validation. This represents a deliberate design evolution and historical schema versions should not be modified to add stricter validation.
Applied to files:
packages/app-data/src/schemas/v1.9.0.jsonpackages/app-data/src/generatedTypes/index.ts
🧬 Code graph analysis (2)
packages/app-data/test/schema.spec.ts (1)
packages/app-data/test/test-utils.ts (1)
buildAssertValidFn(3-11)
packages/app-data/src/generatedTypes/index.ts (2)
packages/app-data/src/generatedTypes/v1.9.0.ts (1)
AppDataRootSchema(186-191)packages/trading/src/types.ts (1)
AppDataRootSchema(229-229)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Publish to GitHub Packages
- GitHub Check: eslint
🔇 Additional comments (4)
packages/app-data/src/generatedTypes/latest.ts (1)
3-3: LGTM: latest export now targets v1.9.0Aligns the alias with the new schema version.
packages/app-data/src/schemas/v1.9.0.json (1)
1-84: v1.9.0 schema wiring looks correctRoot/metadata strictness and hooks->v0.3.0 reference align with the change.
packages/app-data/src/schemas/hook/v0.3.0.json (1)
1-44: Hook v0.3.0 matches the migration planRequired fields + optional deprecated dappId and new type are correct. With hookType minLength enforcement in definitions.json, this stays flexible without allowing empty strings.
packages/app-data/src/generatedTypes/index.ts (1)
27-41: LGTM: latest constants/types upgraded to 1.9.0 and hooks 0.3.0Public API aligns with new schema; union updated appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/app-data/test/flashloan-v1.7.0.spec.ts (1)
5-5: Consider renaming the describe block to reflect mixed version coverage.The describe block name "Flashloan metadata v1.7.0" and the file name
flashloan-v1.7.0.spec.tssuggest all tests validate v1.7.0 specifically. However, tests at lines 14-25 and 156-172 now validate the latest version (1.9.0). While the behavior is correct—newly generated documents should use the latest schema—the naming is misleading.Consider either:
- Renaming the describe block to "Flashloan metadata" (removing version reference)
- Splitting tests into separate describe blocks for v1.7.0 and latest version
- Renaming the file to
flashloan.spec.tsAlso applies to: 14-14, 156-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app-data/test/flashloan-v1.7.0.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app-data/test/flashloan-v1.7.0.spec.ts (1)
packages/app-data/src/generatedTypes/index.ts (1)
LATEST_APP_DATA_VERSION(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: Publish to GitHub Packages
- GitHub Check: eslint
🔇 Additional comments (2)
packages/app-data/test/flashloan-v1.7.0.spec.ts (2)
3-3: LGTM: Import ofLATEST_APP_DATA_VERSIONis correct.The import statement correctly brings in the latest version constant for use in the tests.
14-14: LGTM: Correct usage ofLATEST_APP_DATA_VERSIONconstant.The test names were appropriately updated to "latest" and the assertions correctly use the
LATEST_APP_DATA_VERSIONconstant instead of hard-coded version strings. This ensures tests automatically validate against the current latest version.Also applies to: 24-24, 156-156, 171-171
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-data/test/flashloan-metadata.spec.ts (1)
27-154: Consider adding v1.9.0 compatibility tests.The existing tests validate flashloan metadata for v1.7.0, which is good for regression testing. However, consider adding similar validation tests using v1.9.0 to ensure flashloan metadata remains compatible with the latest schema version.
Example test to add:
test('Validates valid flashloan metadata v1.9.0', async () => { // given const appDataDocV190 = { version: '1.9.0', appCode: 'aave-v3-flashloan', metadata: { flashloan: validFlashloanMetadata, }, } // when const validation = await validateAppDataDoc(appDataDocV190) // then expect(validation.success).toBeTruthy() expect(validation.errors).toBeUndefined() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/app-data/src/schemas/definitions.json(2 hunks)packages/app-data/test/flashloan-metadata.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app-data/test/flashloan-metadata.spec.ts (1)
packages/app-data/src/generatedTypes/index.ts (1)
LATEST_APP_DATA_VERSION(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Publish to GitHub Packages
- GitHub Check: test
- GitHub Check: eslint
🔇 Additional comments (2)
packages/app-data/test/flashloan-metadata.spec.ts (2)
3-3: LGTM! Good use of version constant for maintainability.Using
LATEST_APP_DATA_VERSIONinstead of hardcoded version strings improves maintainability and ensures tests stay in sync with the latest schema version.Also applies to: 24-24, 171-171
5-5: LGTM! Test naming now version-agnostic.The updated test names appropriately reference "latest" instead of specific versions, making them easier to maintain as new versions are introduced.
Also applies to: 14-14, 156-156
I moved it back to draft, as we need to update usages of |
Summary
AppData hook field
dappIdproperty is currently too strict, specially since we moved to a human readable format.Partially addressed already on #591
This PR:
Removed the hex limitation of thecovered on fix(app-data): remove dappId validation #591dappIdfield to accommodate for current usagedappIdfield as deprecated in favor of the new fieldtypetypefield to be more inline with the current usageTesting
Unit tests
Summary by CodeRabbit
New Features
Tests