Skip to content

REVIEW.md

Latest commit

 

History

History
208 lines (139 loc) · 6.86 KB

REVIEW.md

File metadata and controls

208 lines (139 loc) · 6.86 KB

Review

Scope

This review checks dev-runtime against dev with the following standards:

  • Do not accidentally remove or override teammate payment capabilities already merged into dev.
  • Do not submit local test artifacts or dirty files.
  • Confirm Redis -> SQLite replacement is functionally complete for the intended single-instance target.
  • Judge code quality, elegance, robustness, and maintainability.
  • Check for real bugs, mojibake, and redundant code.
  • Compare current quality with the previous dev implementation.

These standards are also the default development constraints for future work:

  • no invalid tests
  • no mirror tests
  • prefer behavior-based tests over implementation-shaped tests
  • explain intentional semantic changes in review notes
  • verify dirty local artifacts are excluded before commit
  • think about teammate-regression risk before coding, not only after coding

Files That Must Not Be Committed

These local SQLite artifacts must never be committed:

  • src/epusdt.db
  • src/epusdt.db-shm
  • src/epusdt.db-wal

They are now excluded by .gitignore.

Teammate Capabilities Still Preserved

The following teammate payment capabilities are still present:

  • multi-currency order model: src/model/mdb/orders_mdb.go
  • wallet address model: src/model/mdb/wallet_address_mdb.go
  • native TRX transfer detection: src/model/service/task_service.go
  • TRC20 USDT detection: src/model/service/task_service.go
  • dynamic exchange-rate path: src/model/service/order_service.go, src/config/config.go
  • TRON_GRID_API_KEY support: src/config/config.go

Conclusion: teammate core payment capabilities were not accidentally removed.

Intentional Architecture Changes

These are intentional refactors, not accidental overrides:

  • Redis runtime removed: src/model/dao/rdb.go
  • asynq handlers removed: src/mq/handle/callback_queue.go, src/mq/handle/order_expiration_queue.go
  • SQLite runtime lock introduced: src/model/mdb/transaction_lock_mdb.go
  • SQLite scheduler introduced: src/mq/queue.go, src/mq/worker.go
  • reservation moved from Redis key to SQLite unique constraint: src/model/data/order_data.go

Conclusion: this branch keeps payment capability but changes the runtime implementation.

Previously Regressed Parts That Are Now Restored

The following regressions have been restored in the current working tree:

  • rich Telegram payment notification template: src/model/service/task_service.go
  • key chain-scan observability logs: src/model/service/task_service.go
  • Chinese checkout error message: src/model/service/pay_service.go
  • more reliable Telegram add-wallet flow: src/telegram/handle.go

Functional Completion

For the single-instance target, the Redis -> SQLite replacement is complete across:

  • order creation
  • amount reservation
  • chain scan matching
  • payment success processing
  • order expiration
  • callback scheduling
  • Telegram notification

Conclusion: feature work is complete for the intended single-instance model.

Mojibake Check

No real mojibake was found in the source files checked during review.

Files spot-checked:

  • src/telegram/handle.go
  • src/model/service/task_service.go
  • src/model/service/pay_service.go

Note: some terminals may render UTF-8 Chinese poorly, but the source content itself is fine.

Redundant Code Check

No obvious redundant business code was found.

Recent helper additions are justified:

  • PendingCallbackOrder
  • expirableOrder
  • configureSQLite

Conclusion: no clear "fix by duplication" smell was found.

Bug Assessment

No confirmed blocking business bug is currently visible in the reviewed code path.

Important risk that still needs attention:

  • SQLite main DB can still hit SQLITE_BUSY under concurrent polling and writes.

This risk is now being mitigated by:

  • src/model/dao/sqlite_config.go
  • src/model/dao/mdb_sqlite.go
  • src/model/dao/runtime_sqlite.go
  • lightweight busy retry in src/mq/worker.go

Conclusion: no confirmed blocking functional bug, but SQLite concurrency remains a real operational boundary that should still be watched in runtime.

Robustness Assessment

Strengths:

  • conditional state transitions prevent reviving expired orders
  • reservation is now a persistent unique constraint instead of a temporary Redis key
  • callback state is durable across process restarts
  • behavior tests exist for key paths:
    • src/model/service/order_service_test.go
    • src/mq/worker_test.go
  • callback polling now has a small targeted retry for transient SQLite busy errors

Remaining limits:

  • robust for single-instance deployment
  • not a final design for multi-instance deployment
  • SQLite contention should still be watched in real runtime

Conclusion: robust enough for single-instance use, not a direct multi-instance design.

Elegance Assessment

Strengths:

  • runtime responsibility is cleaner with SQLite reservation + scheduler
  • logging responsibility is split more clearly:
    • log_level
    • http_access_log
    • sql_debug
  • Telegram input flow is more practical for real clients
  • SQLite tuning is extracted into one helper

Tradeoffs:

  • callback semantics are no longer the same as the old immediate-asynq enqueue path
  • current branch still has local uncommitted fixes that must be organized into a clean commit set

Conclusion: elegant for the chosen single-instance direction, but must be explained clearly when merging.

Quality Compared With Previous Code

Overall judgment: quality did not go down; it improved in the single-instance direction.

Improvements:

  • stronger state-machine constraints
  • better log control
  • more durable runtime reservation
  • better Telegram interaction resilience
  • added behavior tests
  • added restart-oriented callback recovery coverage
  • reduced scheduler fragility under transient SQLite busy errors

Changed semantics rather than degradation:

  • merchant callback is now polled by SQLite scheduler instead of immediate asynq enqueue

Conclusion: quality is not lower, but runtime semantics changed and must be stated explicitly.

Self Score

  • functional completeness: 9.1/10
  • engineering implementation: 8.9/10
  • robustness: 8.7/10
  • elegance: 9.0/10
  • commit readiness: 8.8/10

Overall self score: 8.9/10

Main deductions:

  • current uncommitted fixes still need to be organized into a clean commit
  • SQLite single-file concurrency boundary still exists
  • callback semantics differ from dev-payment and require clear communication

Final Conclusion

This code can be submitted, but only if all of the following are done:

  1. Do not commit local SQLite artifacts.
  2. Include the current uncommitted fixes in a clean, intentional commit set.
  3. Explain clearly in merge notes:
    • what teammate capabilities were preserved
    • what runtime behavior was intentionally refactored
    • what semantics changed and why

If those three conditions are met, this is not a hidden override of teammate code. It is a reviewed and documented refactor.