Skip to content

fix: add client-side validation for alert type-specific required fields#299

Open
0xlaveen wants to merge 2 commits intomainfrom
fix/alert-cli-validation
Open

fix: add client-side validation for alert type-specific required fields#299
0xlaveen wants to merge 2 commits intomainfrom
fix/alert-cli-validation

Conversation

@0xlaveen
Copy link
Contributor

Summary

  • Adds validateAlertData() that checks type-specific required fields before sending to the API, giving users clear error messages instead of raw API errors:
    • sm-token-flows: requires at least one inflow/outflow/netflow threshold
    • common-token-transfer: requires at least one --subject or --token
    • smart-contract-call: requires at least one --caller, --contract, or --signature-hash
  • Validation runs on both create and update (on the final merged data)
  • Fixes deep-merge of range fields during updates so providing only --min doesn't overwrite existing --max with null

Test plan

  • All 1046 existing tests pass
  • 16 new tests covering all validation paths (pass/fail for each type, null/undefined edge cases, integration tests for create rejecting invalid payloads)
  • Manual: nansen alerts create --name Test --type sm-token-flows --chains ethereum --telegram 123 → should show clear validation error
  • Manual: nansen alerts create --name Test --type smart-contract-call --chains ethereum --telegram 123 --signature-hash 0xa9059cbb → should succeed

🤖 Generated with Claude Code

0xlaveen and others added 2 commits March 13, 2026 18:04
…min/max

The shallow merge in the update handler replaced entire range objects
(e.g. inflow_1h, usdValue), so updating only --inflow-1h-min would
drop the existing max to null. Now all nested plain objects (ranges,
inclusion, exclusion) are deep-merged, and null sub-keys from
buildRange are preserved from the existing value.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…alerts

Before this change, users creating/updating alerts without required
type-specific fields would get raw API error messages. Now the CLI
validates before sending to the API:

- sm-token-flows: requires at least one inflow/outflow/netflow threshold
- common-token-transfer: requires at least one --subject or --token
- smart-contract-call: requires at least one --caller, --contract, or --signature-hash

Also fixes deep-merge of range fields during updates so that providing
only --min doesn't overwrite an existing --max with null.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nansen-pr-reviewer
Copy link

pr-reviewer Summary

📝 2 findings

Review completed. Please address the findings below.

Findings by Severity

Severity Count
🟡 Medium 1
🔵 Low 1

Review effort: 3/5 (Moderate)

Summary

This PR adds meaningful client-side validation for type-specific required fields in alerts (thresholds for sm-token-flows, subjects/tokens for common-token-transfer, callers/contracts for smart-contract-call), and fixes a subtle but real bug where the old shallow merge during updates would silently clear subjects and range fields. The deep-merge logic is correct and well-tested. Two issues worth addressing before merging:

File-by-file findings

src/commands/alerts.js — update handler (lines 652–653)

Severity: Medium

validateAlertData is called with params.data ?? existing.data unconditionally, meaning it runs against the fetched API data even for metadata-only updates (e.g., renaming an alert or changing channels). If an existing alert in production has a structure that satisfies the API but fails the new CLI rules (e.g., a sm-token-flows alert created via the API with no flow thresholds set, or a common-token-transfer with no subjects), any CLI update — even a trivial rename — will throw a INVALID_PARAMS error and leave the alert unmodifiable without a delete+recreate.

The existing test at line 830 passes because its mock returns { type: 'sm-token-flows' } with no data field, making finalData undefined and skipping validation. But a mock like { type: 'sm-token-flows', data: { chains: ['ethereum'] } } (valid API response, no thresholds) would expose the breakage.

Suggested fix: Only validate when the user actually provided data-affecting flags — i.e., only run validateAlertData when params.data is set:

if (params.data) validateAlertData(type, params.data);

This preserves the invariant that the CLI validates what it is about to send, not what already exists on the server.


src/commands/alerts.js — deep-merge null filter (line 644)

Severity: Low

const filtered = Object.fromEntries(Object.entries(newVal).filter(([, v]) => v !== null));

The null-value filter correctly prevents an unspecified bound from clobbering an existing one (the stated goal of this PR). However, it also makes it impossible to explicitly clear an existing max or min via the CLI. For example, if an alert has inflow_1h: { min: 100, max: 5_000_000 } and the user later wants to remove the upper cap, there is no CLI flag combination that can achieve this — --inflow-1h-min 100 (without --inflow-1h-max) will preserve the old max: 5_000_000. The only recourse is delete+recreate.

This is a reasonable trade-off for the majority use case (accidental dropping of bounds), but it's worth a comment in the code or a note in the help text (--inflow-1h-max) so users aren't confused by stale bounds persisting. No functional change required if the design decision is accepted.


Token usage: 335 input, 8,108 output, 155,936 cache read, 33,879 cache write

Copy link
Contributor

@kome12 kome12 left a comment

Choose a reason for hiding this comment

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

@0xlaveen Same here where the branch is from another branch so it's hard to merge. Also, I already merged some of the other PRs but these all don't have changesets. Please include it in the PRs.

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.

2 participants