Skip to content

fix: scope --token-address filter to actual token addresses#298

Merged
kome12 merged 3 commits intomainfrom
fix/alert-token-address-filter
Mar 16, 2026
Merged

fix: scope --token-address filter to actual token addresses#298
kome12 merged 3 commits intomainfrom
fix/alert-token-address-filter

Conversation

@0xlaveen
Copy link
Contributor

Summary

  • Bug: --token-address filter used JSON.stringify(a.data).includes(addr), a raw substring search across the entire serialized alert data blob. This caused false positives where searching for "ethereum" matched chain names, "all" matched chains: ['all'], and "send" matched event types.
  • Fix: Replace the substring search with a targeted check that only searches within data.inclusion.tokens and data.exclusion.tokens arrays, matching against the address field specifically.
  • Tests: Added 3 new test cases covering exclusion token matching and false-positive prevention. Fixed test counts for the 5th alert fixture.

Test plan

  • Existing --token-address case-insensitive test still passes
  • New test: matches token address in exclusion tokens
  • New test: --token-address ethereum no longer false-positives against chain names
  • New test: --token-address all no longer false-positives against chains: ['all']
  • All 347 tests pass (2 pre-existing failures in time-window tests from prior commit)

🤖 Generated with Claude Code

0xlaveen and others added 2 commits March 13, 2026 17:58
buildCommonTokenTransferData unconditionally set data.subjects = subjects ?? [],
causing the update path (applyDefaults: false) to overwrite existing subjects
with an empty array via shallow merge. Now subjects is only set when the user
explicitly passes --subject, matching the pattern used by counterparties.

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

Add tests verifying --token-address only matches actual token addresses
in inclusion/exclusion arrays, not chain names or other data fields.
Fix test counts for the 5th alert fixture added in prior commit.

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

nansen-pr-reviewer bot commented Mar 13, 2026

pr-reviewer Summary

📝 1 finding

Review completed. Please address the findings below.

Findings by Severity

Severity Count
🔵 Low 1

Review effort: 2/5 (Simple)

Summary

This is a clean, focused PR that correctly addresses two issues: a tighter token-address filter and a validateAlertData simplification. The test additions are thorough and the logic changes are correct. One minor process observation is noted below.

Findings (1 low)

src/commands/alerts.js — Missing changeset for user-facing bug fix

Severity: Low

Description: CONTRIBUTING.md requires a changeset file for any "new feature, bug fix, or changed output." The --token-address filter change from substring matching to exact equality (=== addr) is a user-facing behavior change — it fixes false positives but will also break any callers that relied on partial-address lookup. No new changeset was added in this PR.

There is an existing uncommitted changeset (alerts-list-client-side-filtering.md) that covers --token-address filtering for the prior client-side filtering PR. It may be considered sufficient since it hasn't been consumed by CI yet and explicitly mentions --token-address. However, it describes a different bug (server-side vs. client-side filtering), so a reviewer could argue this fix deserves its own changelog entry.

Suggested fix: Either add a small patch changeset describing the exact-match tightening, or confirm the existing alerts-list-client-side-filtering.md entry is intentionally being relied upon to cover this change.


Other observations (no action needed)

  • isRangeSet simplification (range.min != null || range.max != null): Semantically equivalent to the original !== undefined && !== null pair — != null via loose equality covers both undefined and null, including the 0-as-threshold edge case correctly. ✅

  • --token-address exact match: Correctly limits matching to data.inclusion.tokens and data.exclusion.tokens, comparing only the address field with ===. Optional chaining on t.address?.toLowerCase() correctly handles tokens without an address field (yields undefined, which never === addr). ✅

  • Test fixes: The two repaired create-flow tests now supply the required fields (inflow-1h-min for sm-token-flows, subject for common-token-transfer) that validateAlertData enforces. The 10 new validateAlertData unit tests cover all three alert types with both passing and throwing cases, including null/falsy guard and unknown-type no-op. ✅


Token usage: 17 input, 6,325 output, 210,938 cache read, 22,956 cache write

…ilter

- Fix broken create integration tests by supplying required fields
- Add 10 dedicated validateAlertData unit tests (pass + throw per type)
- Tighten --token-address filter to exact match instead of substring
- Simplify isRangeSet with loose equality

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 (Ari?) Please remember to create a new branch from main instead of another branch because it makes it harder to merge isolated PRs.

@kome12 kome12 merged commit 7cf524c into main Mar 16, 2026
7 checks passed
@kome12 kome12 deleted the fix/alert-token-address-filter branch March 16, 2026 02:13
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