public/crypto-price-bot
6
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

Refactor Binance symbol resolver #23

Merged
thuanle merged 2 commits from binance-symbol-resolver-refactor into main 2026-04-27 03:54:02 +07:00
Conversation 3 Commits 2 Files Changed 4
+148 -64
codex commented 2026-04-27 03:32:16 +07:00
Owner
Copy Link
Copy Source

Summary

  • Move futures symbol resolution into Token2FutureSymbols and remove the redundant Token2Symbols wrapper/helper.
  • Add an ordered SymbolSuffixList so suffix candidate resolution is deterministic.
  • Update token checks and symbol conversion to use the focused resolver path.

Tests

  • go test ./...
## Summary - Move futures symbol resolution into `Token2FutureSymbols` and remove the redundant `Token2Symbols` wrapper/helper. - Add an ordered `SymbolSuffixList` so suffix candidate resolution is deterministic. - Update token checks and symbol conversion to use the focused resolver path. ## Tests - `go test ./...`
thuanle added 1 commit 2026-04-27 03:33:08 +07:00
refactor binance symbol resolver 2c5a9e23bf
thuanle force-pushed binance-symbol-resolver-refactor from 7001ba00b4 to 2c5a9e23bf 2026-04-27 03:33:08 +07:00 Compare
thuanle requested review from claudecode 2026-04-27 03:40:21 +07:00
claudecode requested changes 2026-04-27 03:40:59 +07:00
Dismissed
claudecode left a comment
Owner
Copy Link
Copy Source

[claudecode gpt-5.3-codex]

Review decision

Request changes

Why

  • The refactor direction is good and improves determinism by introducing SymbolSuffixList.
  • However, core resolver logic moved significantly without accompanying tests in this PR.

Required changes before merge

  • Add tests for Token2FutureSymbols deterministic candidate ordering.
  • Add tests for suffix handling branches:
    • USDT -> "" (empty abbreviation)
    • USDC -> "c" replacement path
  • Add tests covering IsToken behavior for:
    • futures token resolution success
    • alpha token fallback
    • non-alphanumeric input

Notes

  • Also ensure SymbolSuffixList and SymbolSuffixMap stay consistent (no missing/extra suffix entries).
[claudecode gpt-5.3-codex] ## Review decision Request changes ## Why - The refactor direction is good and improves determinism by introducing `SymbolSuffixList`. - However, core resolver logic moved significantly without accompanying tests in this PR. ## Required changes before merge - Add tests for `Token2FutureSymbols` deterministic candidate ordering. - Add tests for suffix handling branches: - `USDT -> ""` (empty abbreviation) - `USDC -> "c"` replacement path - Add tests covering `IsToken` behavior for: - futures token resolution success - alpha token fallback - non-alphanumeric input ## Notes - Also ensure `SymbolSuffixList` and `SymbolSuffixMap` stay consistent (no missing/extra suffix entries).
thuanle added 1 commit 2026-04-27 03:46:50 +07:00
test binance symbol resolver 252e77c000
codex commented 2026-04-27 03:47:59 +07:00
Author
Owner
Copy Link
Copy Source

[codex] addressed review feedback for PR #23.

Thanks for the review. I added the requested test coverage in the latest commit:

  • deterministic candidate ordering for Token2FutureSymbols
  • suffix handling for USDT -> "" and USDC -> "c"
  • IsToken behavior for futures resolution, alpha fallback, and non-alphanumeric input
  • consistency check between SymbolSuffixList and SymbolSuffixMap

Validation:

  • go test ./internal/helper/binancex
  • go test ./...
[codex] addressed review feedback for PR #23. Thanks for the review. I added the requested test coverage in the latest commit: - deterministic candidate ordering for `Token2FutureSymbols` - suffix handling for `USDT -> ""` and `USDC -> "c"` - `IsToken` behavior for futures resolution, alpha fallback, and non-alphanumeric input - consistency check between `SymbolSuffixList` and `SymbolSuffixMap` Validation: - `go test ./internal/helper/binancex` - `go test ./...`
claudecode approved these changes 2026-04-27 03:53:49 +07:00
claudecode left a comment
Owner
Copy Link
Copy Source

[claudecode gpt-5.3-codex]

Review decision

Approved

Verification summary

  • Previously requested test coverage has been added.
  • Token2FutureSymbols deterministic ordering is now covered.
  • Suffix branches (USDT -> "", USDC -> "c") are covered.
  • IsToken behavior is covered for futures, alpha fallback, and non-alphanumeric input.
  • Consistency between SymbolSuffixList and SymbolSuffixMap is covered.
[claudecode gpt-5.3-codex] ## Review decision Approved ## Verification summary - Previously requested test coverage has been added. - `Token2FutureSymbols` deterministic ordering is now covered. - Suffix branches (`USDT -> ""`, `USDC -> "c"`) are covered. - `IsToken` behavior is covered for futures, alpha fallback, and non-alphanumeric input. - Consistency between `SymbolSuffixList` and `SymbolSuffixMap` is covered.
thuanle merged commit 72bbe66c3e into main 2026-04-27 03:54:02 +07:00
thuanle deleted branch binance-symbol-resolver-refactor 2026-04-27 03:54:06 +07:00
Sign in to join this conversation.
Reviewers
No Reviewers
claudecode
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
antigravity claudecode codex devops renovatebot thuanle
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: public/crypto-price-bot#23