feat: discover and parse CCXT declarations (Task 6)#9
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR completes Phase 2 Task 6 by introducing the ChangesCCXT Declaration Parsing
Sequence DiagramsequenceDiagram
participant Compile as CcxtOcx.Declarations.Compile
participant DeclarationsAPI as CcxtOcx.Declarations.parse_unified_surface/0
participant OXC as OXC.parse/2
participant MethodFilter as CcxtOcx.Declarations.Compile.public_unified_method?/1
participant LayoutGuard as assert_required_methods!/1
DeclarationsAPI->>OXC: read/parse base Exchange.d.ts
DeclarationsAPI->>OXC: read/parse exchange *.d.ts (glob)
DeclarationsAPI->>OXC: read/parse pro/*.d.ts (glob)
OXC-->>DeclarationsAPI: AST nodes
DeclarationsAPI->>MethodFilter: filter method names
MethodFilter-->>DeclarationsAPI: included methods
DeclarationsAPI->>LayoutGuard: validate smoke methods and primaries
LayoutGuard-->>DeclarationsAPI: pass or raise error
DeclarationsAPI-->>Compile: return sorted method terms with overrides
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a compile-time CCXT TypeScript declaration parser that will feed the Phase 2 macro-generation layer with unified method metadata.
Changes:
- Introduces
CcxtOcx.DeclarationsandCcxtOcx.Declarations.Compileto discover, parse, filter, and classify CCXT.d.tsmethod declarations. - Centralizes unified-method filtering and updates
BundleSurface.Compileto delegate to it. - Adds tests and updates roadmap/changelog/docs to mark Task 6 complete.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/ccxt_ocx/declarations/compile.ex |
Implements declaration discovery, parsing, type rendering, filtering, overrides, and layout guards. |
lib/ccxt_ocx/declarations.ex |
Adds public facade and external resource tracking for declaration files. |
lib/ccxt_ocx/bundle_surface/compile.ex |
Delegates unified method filtering to the new declarations module. |
test/ccxt_ocx/declarations_test.exs |
Adds unit and integration coverage for filtering, type rendering, parsing, and path helpers. |
ROADMAP.md |
Marks Task 6 as complete. |
roadmap/tasks.toml |
Marks Task 6 done and records implementation metadata. |
roadmap/data.json |
Mirrors Task 6 completion metadata. |
CHANGELOG.md |
Documents the new declaration parser under Unreleased. |
.sobelow-skips |
Updates Sobelow skip fingerprints for changed/new file-read locations. |
CLAUDE.md |
Updates project guidance to mention the new declarations data source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+353
to
+381
| missing = Enum.reject(@required_smoke_methods, &MapSet.member?(present, &1)) | ||
|
|
||
| if missing != [] do | ||
| raise """ | ||
| CCXT declaration layout changed — required unified methods disappeared. | ||
|
|
||
| Missing from :base surface (or filtered out): | ||
| #{inspect(missing)} | ||
|
|
||
| Expected them in: | ||
| #{base_dts_path()} | ||
|
|
||
| This is a deliberate loud failure (Task 6 acceptance). | ||
| After a `mix npm.update ccxt`, review the diff and either: | ||
| - pin a known-good version in package.json, or | ||
| - run `mix ccxt.verify_bundle --accept` (after human review). | ||
|
|
||
| If the methods genuinely moved to a different .d.ts, update the discovery | ||
| globs in Declarations.Compile. | ||
| """ | ||
| end | ||
|
|
||
| # Also assert that at least one of them truly came from the base Exchange.d.ts | ||
| base_sources = | ||
| terms | ||
| |> Enum.filter(&(&1.name in @required_smoke_methods)) | ||
| |> Enum.map(& &1.primary.source) | ||
|
|
||
| if !Enum.any?(base_sources, &String.ends_with?(&1, "Exchange.d.ts")) do |
Comment on lines
+90
to
+93
| |> Enum.flat_map(fn t -> [t.primary.surface] ++ Map.keys(t.overrides) end) | ||
| |> Enum.uniq() | ||
|
|
||
| assert :base in all_surfaces |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@lib/ccxt_ocx/declarations/compile.ex`:
- Around line 353-386: The current validation only ensures at least one of
`@required_smoke_methods` has a primary source ending with "Exchange.d.ts" (via
base_sources and Enum.any?), which can miss other required names; change the
check to verify each required smoke method in terms has primary.source ending in
"Exchange.d.ts" and collect any names that fail this per-method check, then
raise a clear error listing those method names (use `@required_smoke_methods`,
terms, each term.primary.source and Exchange.d.ts in the message) so the failure
surfaces which specific required methods lost their base :base primary surface.
In `@test/ccxt_ocx/declarations_test.exs`:
- Around line 84-97: The test "distinguishes base / exchange / pro surfaces and
populates overrides" calls Declarations.parse_unified_surface() and builds
all_surfaces from terms but never asserts that :pro is present; update the
assertion logic so it explicitly checks that :pro is included in all_surfaces
(the variable built from terms and Map.keys(t.overrides))—for example add an
assertion that :pro in all_surfaces or assert Enum.member?(all_surfaces, :pro)
to ensure pro-surface participation is enforced by the test.
- Around line 107-109: The two brittle assertions using a magic threshold should
be made robust: replace the hard-coded "> 50" checks on
Compile.exchange_dts_paths() and Compile.pro_dts_paths() with
existence/non-empty checks (e.g., assert length(Compile.exchange_dts_paths()) >
0 or assert Enum.any?(Compile.exchange_dts_paths())) or make the minimum
threshold configurable via an env var; update the test to use
Compile.exchange_dts_paths and Compile.pro_dts_paths references so it only
asserts the lists are non-empty (or uses a configurable threshold) rather than a
fixed 50-file magic number.
🪄 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 Plus
Run ID: 5dc6bd98-8055-4cd5-88e7-e023e442ba4b
📒 Files selected for processing (10)
.sobelow-skipsCHANGELOG.mdCLAUDE.mdROADMAP.mdlib/ccxt_ocx/bundle_surface/compile.exlib/ccxt_ocx/declarations.exlib/ccxt_ocx/declarations/compile.exroadmap/data.jsonroadmap/tasks.tomltest/ccxt_ocx/declarations_test.exs
![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: dcdf7b3eb7
ℹ️ 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".
| |> Enum.filter(&(&1.name in @required_smoke_methods)) | ||
| |> Enum.map(& &1.primary.source) | ||
|
|
||
| if !Enum.any?(base_sources, &String.ends_with?(&1, "Exchange.d.ts")) do |
There was a problem hiding this comment.
When a future CCXT update moves only one required smoke method (for example createOrder) out of Exchange.d.ts while another required method remains there, this Enum.any? still lets the layout guard pass. In that case primary for the moved method falls back to an arbitrary exchange/pro declaration, so the parser no longer fails loudly even though the base unified contract changed; the guard should verify each required smoke method has a base Exchange.d.ts primary, not just that at least one does.
Useful? React with 👍 / 👎.
…erride surfaces Layout guard previously used Enum.any?, so it would have stayed silent if e.g. createOrder moved off Exchange.d.ts while fetchTicker stayed — exactly the drift class Task 6 acceptance requires the guard to catch. Now checks each of fetchTicker/createOrder/watchTicker is primary :base from Exchange.d.ts and lists every offender in the raised message. Integration test that 'distinguishes base / exchange / pro surfaces' was flattening Map.keys(overrides) (strings like "exchange:binance"), so the :exchange / :pro atoms never appeared in all_surfaces — only :base was ever asserted. Now reads surface atoms off Map.values(overrides) and asserts :exchange and :pro both contribute, catching surface-classification regressions explicitly. Reviewed by: CodeRabbit, Copilot, Codex GitHub bot (3-bot convergence on the layout-guard finding).
5 tasks
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
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
Compile-time parser that turns CCXT's TypeScript declarations into rich per-method terms (name, params, return type, owning surface, overrides) for the Phase 2 macro layer to consume. Completes Task 6 (
v0_1milestone,macrosbundle).CcxtOcx.Declarationsfacade +CcxtOcx.Declarations.Compileparser. Walksjs/src/base/Exchange.d.ts, per-exchange*.d.ts, andpro/*.d.tswithOXC.parse/2+OXC.collect/2, classifying methods by:base,:exchange, or:prosurface and capturing per-exchange overrides.CcxtOcx.BundleSurface.Compilenow delegates toDeclarations.Compile.public_unified_method?/1so the "what gets adefunifiedwrapper" decision lives in one place."surface:exchange_id"so a method declared in bothjs/src/<id>.d.tsandjs/src/pro/<id>.d.ts(e.g.kucoinfutures.fetchBidsAsks) keeps both override entries instead of one silently overwriting the other.fetchTicker,createOrder,watchTicker) disappear from the base surface.@external_resourceregistered for every discovered base/exchange/pro.d.tssomix npm.update ccxttriggers a recompile of the facade.Test plan
mix compile --warnings-as-errorscleanmix format --check-formattedcleanmix credo --strictclean (only expected TODO tags)mix test.json --quiet— 165/165 offline tests passmix test.json --quiet --include integration test/ccxt_ocx/declarations_test.exs— 12/12 integration tests pass against real CCXT .d.tsmix sobelow --mark-skip-allregenerated for line-fingerprint driftSummary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores