Integrate Starknet facilitator-server coverage and validation#42
Open
welttowelt wants to merge 3 commits intodaydreamsai:mainfrom
Open
Integrate Starknet facilitator-server coverage and validation#42welttowelt wants to merge 3 commits intodaydreamsai:mainfrom
welttowelt wants to merge 3 commits intodaydreamsai:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds opt-in Starknet support to the facilitator example: a new helper to build Starknet facilitator configs, refactors setup to use it, adds tests validating /supported and config validation behavior, and updates docs and docker-compose with commented Starknet env examples. Starknet remains opt-in via environment variables. Changes
Sequence Diagram(s)(Skipped — changes are a utility, config handling, documentation, and tests; control flow is not sufficiently complex to require a sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/facilitator-server/tests/starknet-supported.test.ts (1)
31-42:fetchSupported()is called independently in each of the 4 tests — considerbeforeAllto avoid redundant setup.Each
it()creates a new facilitator, app, and handles an HTTP request, which means 4× redundant instantiation for a single read-only shared state.♻️ Proposed refactor
-async function fetchSupported(): Promise<SupportedResponse> { - const facilitator = createFacilitator({ - starknetConfigs: [MAINNET_CONFIG, SEPOLIA_CONFIG], - }); - - const app = createApp({ facilitator }); - const response = await app.handle(new Request("http://localhost/supported")); - - expect(response.status).toBe(200); - - return response.json() as Promise<SupportedResponse>; -} - describe("/supported Starknet integration", () => { + let supported: SupportedResponse; + + beforeAll(async () => { + const facilitator = createFacilitator({ + starknetConfigs: [MAINNET_CONFIG, SEPOLIA_CONFIG], + }); + const app = createApp({ facilitator }); + const response = await app.handle(new Request("http://localhost/supported")); + expect(response.status).toBe(200); + supported = await response.json() as SupportedResponse; + }); + it("returns Starknet kinds when Starknet configs are registered", async () => { - const supported = await fetchSupported(); // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/facilitator-server/tests/starknet-supported.test.ts` around lines 31 - 42, The tests call fetchSupported() separately in each it(), causing redundant creation of facilitator and app; move the setup and single HTTP fetch into a beforeAll block: create the facilitator via createFacilitator({ starknetConfigs: [...] }), create the app via createApp({ facilitator }), call app.handle(new Request("/supported")) once, assert response.status === 200, and store the parsed SupportedResponse for all tests to reuse instead of invoking fetchSupported() per test; keep the fetchSupported helper only if you refactor it to be used by beforeAll or inline the same logic in beforeAll.examples/facilitator-server/src/starknet-config.ts (1)
4-8: DuplicateStarknetConfigtype alias — export from here and remove fromsetup.ts.The same conditional-type alias is independently defined in
setup.ts(lines 45–49). SincebuildStarknetConfigsalready lives here, exporting the type avoids drift.♻️ Proposed refactor
-type StarknetConfig = FacilitatorConfig["starknetConfigs"] extends +export type StarknetConfig = FacilitatorConfig["starknetConfigs"] extends | (infer T)[] | undefined ? T : never;Then in
examples/facilitator-server/src/setup.ts, replace the duplicate declaration with an import:-import { buildStarknetConfigs } from "./starknet-config.js"; +import { buildStarknetConfigs, type StarknetConfig } from "./starknet-config.js"; -type StarknetConfig = FacilitatorConfig["starknetConfigs"] extends - | (infer T)[] - | undefined - ? T - : never;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/facilitator-server/src/starknet-config.ts` around lines 4 - 8, The StarknetConfig conditional type alias is duplicated; export the existing StarknetConfig from this file (where buildStarknetConfigs is defined) and remove the duplicate declaration in setup.ts, then update setup.ts to import the exported StarknetConfig instead of redefining it; reference the StarknetConfig type alias and the buildStarknetConfigs symbol to locate the canonical declaration to export and the duplicate to delete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/facilitator-server/src/setup.ts`:
- Around line 45-49: The type alias StarknetConfig in setup.ts duplicates the
existing definition in starknet-config.ts; remove this duplicate declaration and
instead import and use the original StarknetConfig type from starknet-config.ts
(replace the local alias in setup.ts with an import and update any references to
use the imported StarknetConfig and FacilitatorConfig as needed).
---
Nitpick comments:
In `@examples/facilitator-server/src/starknet-config.ts`:
- Around line 4-8: The StarknetConfig conditional type alias is duplicated;
export the existing StarknetConfig from this file (where buildStarknetConfigs is
defined) and remove the duplicate declaration in setup.ts, then update setup.ts
to import the exported StarknetConfig instead of redefining it; reference the
StarknetConfig type alias and the buildStarknetConfigs symbol to locate the
canonical declaration to export and the duplicate to delete.
In `@examples/facilitator-server/tests/starknet-supported.test.ts`:
- Around line 31-42: The tests call fetchSupported() separately in each it(),
causing redundant creation of facilitator and app; move the setup and single
HTTP fetch into a beforeAll block: create the facilitator via
createFacilitator({ starknetConfigs: [...] }), create the app via createApp({
facilitator }), call app.handle(new Request("/supported")) once, assert
response.status === 200, and store the parsed SupportedResponse for all tests to
reuse instead of invoking fetchSupported() per test; keep the fetchSupported
helper only if you refactor it to be used by beforeAll or inline the same logic
in beforeAll.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/facilitator-server/tests/starknet-supported.test.ts (1)
56-71: Add explicittoBeDefined()guards before optional-chained.extraassertionsIf either
find()returnsundefined,expect(undefined).toEqual({...})fails with a confusing mismatch message instead of clearly indicating which kind was not found.🧪 Proposed improvement for diagnostic clarity
+ expect(mainnetKind).toBeDefined(); + expect(sepoliaKind).toBeDefined(); + - expect(mainnetKind?.extra).toEqual({ + expect(mainnetKind!.extra).toEqual({ paymasterEndpoint: MAINNET_CONFIG.paymasterEndpoint, sponsorAddress: MAINNET_CONFIG.sponsorAddress, }); - expect(sepoliaKind?.extra).toEqual({ + expect(sepoliaKind!.extra).toEqual({ paymasterEndpoint: SEPOLIA_CONFIG.paymasterEndpoint, sponsorAddress: SEPOLIA_CONFIG.sponsorAddress, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/facilitator-server/tests/starknet-supported.test.ts` around lines 56 - 71, Add explicit existence checks before asserting .extra: after computing mainnetKind and sepoliaKind (from supported.kinds.find(...)), add expect(mainnetKind).toBeDefined() and expect(sepoliaKind).toBeDefined() so failures clearly indicate a missing kind before you access optional-chained .extra in the subsequent toEqual assertions that compare against MAINNET_CONFIG and SEPOLIA_CONFIG paymasterEndpoint/sponsorAddress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/facilitator-server/tests/starknet-supported.test.ts`:
- Around line 56-71: Add explicit existence checks before asserting .extra:
after computing mainnetKind and sepoliaKind (from supported.kinds.find(...)),
add expect(mainnetKind).toBeDefined() and expect(sepoliaKind).toBeDefined() so
failures clearly indicate a missing kind before you access optional-chained
.extra in the subsequent toEqual assertions that compare against MAINNET_CONFIG
and SEPOLIA_CONFIG paymasterEndpoint/sponsorAddress.
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
/supportedtests for kinds, metadata, signer list, and canonical CAIP/x402 v2 behavior/supportedTesting
Closes #41
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes
Refactor