Skip to content

[parity] add the missing research smart-money pnl-leaderboard command - #550

Open
gulshngill wants to merge 1 commit into
mainfrom
parity/cli-sm-pnl-leaderboard
Open

[parity] add the missing research smart-money pnl-leaderboard command#550
gulshngill wants to merge 1 commit into
mainfrom
parity/cli-sm-pnl-leaderboard

Conversation

@gulshngill

Copy link
Copy Markdown
Contributor

What

The parity checker reports POST /api/v1/smart-money/pnl-leaderboard as a public API endpoint
with no CLI command. Its six sibling smart-money endpoints all have one. This adds it:

nansen research smart-money pnl-leaderboard --chain solana --timeframe 7

Ranks smart money wallets by realized / unrealized PnL. --timeframe accepts the spec's
1, 7, 30, 90, 180 (days), default 7.

Parity report line addressed

## CLI: spec endpoints with no command (8)

- `POST /api/v1/smart-money/pnl-leaderboard` — Smart Money — Get Smart Money PnL Leaderboard

Re-running the checker with this branch as the CLI repo takes cli_missing from 8 → 7,
closing exactly /api/v1/smart-money/pnl-leaderboard and introducing no new findings in any
category (no new stale, no method mismatch, drift unchanged).

Request shape

Derived from the public OpenAPI spec (SmartMoneyPnlLeaderboardRequest), not guessed. Wire body
with defaults — cleanBody strips the empty filters, exactly as every sibling does:

{ "chains": ["solana"], "timeframe": 7 }

Fully populated:

{
  "chains": ["all"],
  "timeframe": 30,
  "filters": { "include_smart_money_labels": ["Fund"] },
  "order_by": [{ "field": "total_pnl_usd", "direction": "DESC" }],
  "pagination": { "page": 1, "per_page": 10 }
}

--chain/--chains, --labels, --sort/--order-by and pagination all come from the existing
shared smart-money handler — no new option plumbing.

Live validation

⚠️ Not live-validated — please smoke-test before merging.

No API key was available to this run, so no request was made to api.nansen.ai. This is a
missing-coverage finding, not a removal: the endpoint's existence and its request/response
contract come from the curated public OpenAPI spec at nansen-api 1b2d057, so nothing here
depends on a probe. What a probe would have added is confirmation that the assembled body is
accepted end-to-end — that check is outstanding, and a reviewer with a key should run the command
above once before merging.

Verification

Run in the pinned origin/main worktree with credentials stripped from the environment
(env -u NANSEN_API_KEY -u GH_TOKEN -u GITHUB_TOKEN):

  • npm test — 62 files passed, 2545 passed, 2 skipped, 0 failed
  • npm run lint — clean
  • JSON.parse(src/schema.json) — valid

New tests: src/__tests__/api.test.js asserts the POST path and body (defaults and fully
populated); src/__tests__/coverage.test.js gains the endpoint so the coverage guard now
enforces it.

Scope

src/api.js, src/cli.js, src/schema.json, the two test files, and a minor changeset —
insertions only apart from the one-line help command list.

No docs changed: no README or skills/*/SKILL.md enumerates the smart-money subcommand set as a
contract (nansen-smart-money-tracker/SKILL.md documents four of the six existing subcommands and
omits dcas), and skills.test.js asserts only the limit-orders skill.

--timeframe is parsed as an integer but not validated client-side against the enum — no sibling
validates locally either, and the API returns its own error. Left as-is deliberately.

The other 7 CLI gaps, the 15 MCP gaps, and the schema-drift items are not touched here.

Pinned SHAs audited by this run

Repo origin/main
nansen-api 1b2d05709f3f747b4a1b0a4264c9ce680a838584
nansen-ra 425394130e5ea1130056fa9edbf0c63cf5a01327
nansen-cli 72e71231e22dad83c3d8b5a9a7a8708d19e2c0ff

Found by the parity-check skill (nansen-api) for
API-311.

Opened by the parity agent as a proposal for human review — auto-merge is not enabled.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nansen-pr-reviewer

Copy link
Copy Markdown

pr-reviewer Summary for #2e44b74

📝 1 finding

Review completed. Please address the findings below.

Findings by Severity

Severity Count
🟡 Medium 1

Review effort: 2/5 (Simple)

Summary

This PR adds the pnl-leaderboard subcommand to the smart-money category, closing one of eight parity gaps. The implementation follows the exact same patterns as the six sibling endpoints. The changeset, schema update, and test coverage are all present and correct.

Findings

src/cli.js — Medium: --timeframe is silently dropped for non-pnl-leaderboard subcommands

Lines 1224–1225 (new code)

const days = options.days ? parseInt(options.days) : 30;
const timeframe = options.timeframe ? parseInt(options.timeframe) : 7;

timeframe is parsed unconditionally for every smart-money subcommand invocation, but it is only forwarded to smartMoneyPnlLeaderboard. This is a one-off issue — if a user accidentally passes --timeframe 30 to smart-money netflow, the CLI silently ignores it, which is fine. The concern is the reverse: the pnl-leaderboard subcommand relies on options.timeframe being picked up here, but there is no corresponding --timeframe entry in the pnl-leaderboard block of src/schema.json's option definitions used for CLI auto-completion / help text generation (the schema options object only has chain and timeframe for the sub-schema, which is fine), and more importantly, perp-trades is also missing from the help command's list — it appears in the handler map at line 1230 but was not added to the commands array at line 1236 historically. That is a pre-existing issue.

The actionable issue here is narrower: the timeframe variable is computed before the subcommand dispatch but options.timeframe will arrive as a string from parseArgs (the parser keeps numeric-looking values as strings to avoid precision loss — see line 203: parsedValue = typeof parsed === 'number' ? next : parsed). The parseInt call handles that correctly, so there is no bug in the numeric conversion itself.

However, because parseArgs represents numbers as strings, options.timeframe for a call like nansen smart-money pnl-leaderboard --timeframe 30 arrives as the string "30". parseInt("30")30 ✓. This path is fine.

The real gap: the timeframe variable would be 7 (default) even when the user does not pass --timeframe on any subcommand other than pnl-leaderboard. This does not cause incorrect behavior today since only pnl-leaderboard consumes it, but it means the default value is eagerly materialised for every call. This is a minor cleanliness issue, not a bug, and matches the pre-existing days pattern exactly (days is also always computed).

Recommendation: No immediate code fix is required for correctness. However, the --timeframe option should be documented in the schema's pnl-leaderboard block's options — it already is there ("timeframe": { "default": 7, "enum": [1,7,30,90,180] }), so this is correctly handled. ✓


Actual medium finding — missing --timeframe option in the smart-money command's shared option parsing path vs. schema

The schema.json correctly documents timeframe as an option of pnl-leaderboard with an enum constraint, but cli.js does not validate the parsed integer against that enum ([1, 7, 30, 90, 180]) before sending it to the API. The PR description explicitly acknowledges this as an intentional decision: "Left as-is deliberately" because no sibling validates locally either, and the API returns its own error. This is an accepted trade-off, not an actionable finding.


Actual medium finding — smart-money help list is incomplete for perp-trades

The new help list at line 1236 now reads:

commands: ['netflow', 'dex-trades', 'perp-trades', 'holdings', 'dcas', 'historical-holdings', 'pnl-leaderboard'],

This is an improvement over the pre-PR list (which omitted pnl-leaderboard). However, the list still uses a fixed hard-coded array rather than Object.keys(handlers). If a future subcommand is added to the handler map but the hard-coded list is not updated, it will silently disappear from help. Consider deriving the list from Object.keys(handlers).filter(k => k !== 'help') to keep them in sync automatically — this also removes the risk of pnl-leaderboard falling off the list in a future refactor.

Fix: Replace the hard-coded array with Object.keys(handlers).filter(k => k !== 'help') (same pattern can also be applied to profiler and token help handlers to prevent future drift).


Token usage: 5,653 input, 4,514 output, 377,546 cache read, 36,947 cache write | Usage Guide

New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment @nansen-pr-reviewer re-review.

@nansen-pr-reviewer nansen-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved

This PR was automatically approved because:

  • Claude recommends approval
  • Claude assessed this as a moderate effort change
  • The effort level is within the auto-approval threshold of 2
  • No high or critical issues were detected
  • Review comment contains non-blocking feedback

If you have any concerns, please request a manual review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant