feat: support executor callback overrides on WormholeConfig#997
feat: support executor callback overrides on WormholeConfig#997kev1n-peters wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Wormhole as Wormhole.getExecutorCapabilities
participant Executor as Config.executor.getCapabilities (optional)
participant API as fetchCapabilities(API)
Client->>Wormhole: request executor capabilities
alt executor.getCapabilities configured
Wormhole->>Executor: getCapabilities(network)
Executor-->>Wormhole: CapabilitiesResponse
else no executor hook
Wormhole->>API: fetchCapabilities(executorAPI)
API-->>Wormhole: CapabilitiesResponse
end
Wormhole-->>Client: return capabilities
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Allow users to pass a getCapabilities callback when constructing the Wormhole SDK object so every route can use it without per-route config.
0ccc9eb to
6f969d1
Compare
|
@claude review this pr and make sure no bugs were introduced, and the getCapabilities callback will work correctly when passed in |
|
Claude finished @kev1n-peters's task in 2m 16s —— View job Review of PR #997
Verdict: Looks good, no bugs found ✅I reviewed both changed files and traced the full execution path through the override system. The Detailed Analysis1. Runtime override merging works correctly ( The new Wormhole("Mainnet", platforms, {
executor: { getCapabilities: myCustomFetcher }
})
2. The new check 3. Optional chaining in
4. Two-level override precedence is clean This PR complements PR #993 nicely:
The route-level functions ( One Minor Observation (not a bug)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
connect/src/config.ts (1)
17-20: Tighten callback typing to the config’s network generic.Line 19 currently accepts
Network; usingNpreserves stronger compile-time guarantees for per-network configs.♻️ Suggested type refinement
executor?: { /** Override the default capabilities fetcher (e.g. to cache or use a custom endpoint). */ - getCapabilities?: (network: Network) => Promise<CapabilitiesResponse>; + getCapabilities?: (network: N) => Promise<CapabilitiesResponse>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connect/src/config.ts` around lines 17 - 20, The getCapabilities callback on executor currently accepts the concrete Network type; tighten it to the config's network generic by changing its parameter to the generic (N) used by the surrounding config so the signature is getCapabilities?: (network: N) => Promise<CapabilitiesResponse>; update the executor type declaration (executor.getCapabilities) and any call sites to accept/forward the generic N instead of the concrete Network to preserve per-network type guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connect/src/config.ts`:
- Around line 58-61: The RecursivePartial type incorrectly treats optional
function properties (e.g., executor?.getCapabilities) as objects because the
function branch fails for unions with undefined; update the conditional to
detect functions after removing undefined using NonNullable (e.g., change the
function check to use NonNullable<T[P]> extends (...args: any[]) => any) so
optional function properties are preserved as functions rather than recursing
into object types; locate the RecursivePartial type in connect/src/config.ts and
adjust its conditional branches accordingly.
---
Nitpick comments:
In `@connect/src/config.ts`:
- Around line 17-20: The getCapabilities callback on executor currently accepts
the concrete Network type; tighten it to the config's network generic by
changing its parameter to the generic (N) used by the surrounding config so the
signature is getCapabilities?: (network: N) => Promise<CapabilitiesResponse>;
update the executor type declaration (executor.getCapabilities) and any call
sites to accept/forward the generic N instead of the concrete Network to
preserve per-network type guarantees.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89a310db-c327-48ad-9cd3-80b8a4e1eb67
📒 Files selected for processing (2)
connect/src/config.tsconnect/src/wormhole.ts
✅ Files skipped from review due to trivial changes (1)
- connect/src/wormhole.ts
Allow users to pass a getCapabilities callback when constructing the Wormhole SDK object so every route can use it without per-route config.
Summary by CodeRabbit