Фидбек по конкурсной работе от проверяющих

[b1ng0o]

Фидбек №1:

Сборка норм, но без технического опыта поднять визуализацию не получится. Конфиг прометеуса придётся составлять самостоятельно. Если формально, то я не смог в пару команд поднять робота вместе с визуализацией.

Достаточно нормальный и понятный код, но есть немало проблем.
Все перечислять не буду, но, например, нет единой сортировки и группировки импортов, есть хардкод адреса API и request timeout-а, модуль utils со всякой разной утварью, местами не самые лучшие решения с точки зрения оптимизаций (в файле GetFormattedPositions можно было бы использовать stringbuilder). Зачем нужны неиспользуемые интерфейсы в пакете sdk — не совсем понятно. Вместо кучи ветвлений в логике для разделения поведения в случае Sandbox и в случае "Боевого режима", можно было бы воспользоваться интерфейсами. Как мне кажется, сам бот не должен вообще знать, в каком режиме он работает.

Код не устойчив к параллельному запуску нескольких горутин, рекомендуем развивать проект в сторону многопоточности. Обратите внимание на работу с указателями

Стратегии можно очень гибко настроить. Неплохая конфигурация поведения, но есть парочка непонятных настроек поведения (продавать всё на шатдауне — звучит очень опасно, но ок). Новые стратегии добавлять удобно, но слишком легко и без каких-либо правил поведения. Интерфейс Trader не обязует разработчиков готовить новые стратегии к завершению работы, хотя, как мне кажется, это важно. Входной контекст можно заигнорить. Также при написании новых алгоритмов придётся в каждом боте описывать кастомные правила поведения при sandbox режиме (это неудобно).

Фидбек №2:

Рекомендуем добавить тесты, их к сожалению нет вообще.

Отличная визуализация!

Классная документация, но в ней не хватает примера конфига прометеуса. Человек, который не имел опыта работы с ним, не сможет исключительно по инструкции легко поднять робота с визуализацией.
Хотелось бы, чтобы парочкой команд из инструкции можно было сразу поднять и робота (это есть), и графану (это почти есть) и прометеус (этого нет).

Рекомендую внимательно проверить код на гонки (флаг -race поможет).
Найдена как минимум одна в CircuitBreaker. Да и в целом этот модуль не очень удачным получился из-за странной работы с указателями (во всё проекте в целом есть немало мест, где работа с указателями выглядит недоработанной).

Местами код становиться очень нечитаемым из-за длинных ванлайнеров. Особенно внутри worker-ов. В целом рекомендуем уделить больше времени тому, чтобы сделать код чище, потому что сейчас можно найти достаточное количество странных и тяжело читаемых мест (internal/trade/strategy/gamble/worker.go:149).

if cnf.AccountID == "<your_api_token>". Эту ситуацию можно решить красивее через правильный парсинг всего конфига.

Есть несколько мест, где можно применить более элегантные конструкции языка (например, кучу if-ов заменить switch / case-ом).

Местами в структурах есть unused поля. Например, зачем в TradeBot в инициализации кладётся TradeConfig?

init() — зло.
синглтоны — зло.

Зачем заведены интерфейсы внутри sdk — непонятно. Они нигде не используются.

Присутствуют легендарные util, по которым по названию непонятно, что это конкретно за util. Да и в целом этот паттерн не самый хороший.

Местами есть ощущение, что данный модуль забегает не на свою территорию (если говорить про объектные области). Например, почему TradeWorker должен собирать вручную pb структуры для похода в API? И это не единственный пример, такие места ещё есть. Модули должны быть изолированны друг от друга по назначению и "полю деятельности" через правильно расставленные интерфейсы.

После начала graceful shutdown может продолжить работать метод checkPortfolio - возможно это несколько не корректно.

Кажется, не рекомендуем дергать tradingStatus на каждый sleep duration тик воркера. Когда биржа не работает, воркеру можно спать подольше.

Наряду с очень хорошей кастомизацией поведения, есть пара мест, где хардкод очень расстроил (например, ApiURL и DefaultRequestTimeout). Как минимум таймаут рано или поздно захочется подкрутить. Ещё, например, в вызовах GetOrderBook захардкожена глубина.

Понравилось то, что стратегии можно очень гибко настраивать, за это большой лайк. Но не понравилось, что сама логика работы бота должна учитывать то, в каком env он работает (sandbox или бой). Эту проблему можно красиво решить через интерфейсы. Также рекомендуем сделать интерфейс Trader строже, потому что сейчас можно достаточно легко забыть о том, что надо при разработке новых стратегий готовить ботов к специальному поведению во время shutdown. CancelContext в Run можно легко пропустить.