Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: breaking changes and new StopOrder functions #284

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

usikpavel
Copy link
Contributor

@usikpavel usikpavel commented Dec 16, 2020

Добрый день. У меня накопились исправления и новый функционал, которым я пользуюсь. Необходимо потестировать - либо дописать автотесты, либо погонять руками в рамках своих приложений. Я пока не смотрел, как здесь устроены автотесты, но попробую разобраться с ними и дописать, если это возможно.

Новый функционал:

StopOrderFunctions class - GetStopOrder_by_transID(), GetStopOrder_by_Number(), CreateStopOrderOrThrow(), KillStopOrderEx() methods
OrderFunctions class - KillOrderEx() method
StopOrder class - Datetime property

BreakingChanges:

  1. Зоопарк из reply.Comment, CLIENT_CODE и TRANS_ID (в связке TradingFunctions.SendTransaction() - QuikEvents.OnTransReplyCall()) заменил на TRANS_ID.

Пояснение.

При отладке новых методов StopOrderFunctions в коллбэке QuikEvents.OnTransReplyCall() не обнаруживалась транзакция. А мне надо положить в нее TransactionReply, чтобы корректно завершилось ожидание транзакции, например, в CreateStopOrderOrThrow(). Причина в том, что TradingFunctions.SendTransaction() сохраняет транзакцию под CLIENT_CODE (или TRANS_ID)

QuikService.Storage.Set(transaction.CLIENT_CODE, transaction);

а QuikEvents.OnTransReply() ищет ее под reply.Comment

QuikService.Storage.Get<Transaction>(reply.Comment);

reply.Comment часто приходит пустой.

Я почитал две ветки обсуждений этого зоопарка из reply.Comment, CLIENT_CODE и TRANS_ID, но так и не нашел рациональную причину его возникновения (issue #264, issue #269). Поэтому исходя из того, что QUIKSharp предназначен для использования в однопоточном режиме (т.е. требуется внешняя синхронизация как минимум для выставления транзакций), и за 1 момент времени выставляется 1 заявка, я переписал все просто на TRANS_ID. И теперь надо потестировать, у кого что сломалось. :) У меня корректно работают все используемые мною функции, связанные с TRANS_ID - отправка/отмена лимитных и рыночных ордеров, а также стоп-заявок в синхронизированном режиме (т.е. с ожиданием выполнения каждой транзакции).

Дополнение. Отправленные Transaction будут бесконечно накапливаться в QuikService.Storage. Я пока не задумывался над его очисткой, т.к. в моем случае их количество достаточно мало, чтобы влиять на память/производительность робота.

  1. В OrderFunctions в методы SendOrder(), SendMarketOrder(), SendLimitOrder() добавлен параметр clientCode

На площадке "Фондовый рынок" Московской биржи у меня всегда требуется код клиента.

@usikpavel usikpavel changed the title Feature: breaking changes and new StopOrder functions feature: breaking changes and new StopOrder functions Dec 16, 2020
@Pr0phet1c
Copy link
Collaborator

К сожалению, данные изменения гарантированно "сломают" работоспособность всех роботов, построенных на базе предыдущих версий библиотеки (при условии что они используют SendOrder). Такие изменения требуют тщательной подготовки, и проверки необходимости внедрения.
Я как-то давно общался с @buybackoff, на тему необходимости указывать ClientCode в транзакциях, и тогда ему удалось убедить меня в отсутствии такой необходимости. Те проверки, которые я смог тогда организовать у себя, подтвердили это (речь именно про выставление заявок программным способом). Однако, я не знал (да и сейчас не знаю) что будет если в одном клиенте квика активны несколько кодов клиента (и бывает ли такое). Можете подробно описать свою ситуацию с этими Кодами клиента?
Из того, что я успел по быстрому глянуть в Вашем, вроде бы все логично, и каких-то явных противоречий не вызывает.
Но очень хотелось бы увидеть, что по этому поводу скажет @buybackoff.

@buybackoff
Copy link
Collaborator

Мне кажется/помнится, что когда я писал эту библитеку, у меня было два разных кода клиента на основном и ФОРТСе. Какая-то заморочка с этим была. Но сейчас детали совсем не помню.

Мы не примем breaking changes. Как минимум это должно было обсуждаться изначально в issues. Всегда можно добавить overload, или функцию с суффиксом ...Ex, ...2, и т.д. если меняется поведение с теми же параметрами. Так не делается, чтобы просто взять и сломать работающий код, которым пользуются люди для торговли. Тем более, что многие (все?) клонируют репо, т.к. NuGet редко обновляется (хорошо бы сделать GitHub Action для обновления после каждого коммита).

Я поддерживаю избаление от "зоопарка", но не ломая текущий код. Можете сделать параллельную имплементацию? Или это сильно завязано на внутренности? Другой выход - добавить версию к транзакции и обрабатывать по-разному разные версии. Мы можем (аккуратно) добавлять поля к сруктурам данных, только нельзя изменять/удалять, если это не соответствует изменениям Квика.

QuikService.Storage

Я думал мы выпилили полностью storage. По крайней мере собирались это сделать, это за пределами периметра проекта. Можно удалять целиком. Кому нужно, могут сами сделать сохранение транзакций и данных любым удобным для себя способом. Кажется остался только InMemory-storage, от него все равно мало толку.

@buybackoff
Copy link
Collaborator

@usikpavel Я сначала подумал, что это Вы с длинной историей ломающих все изменений тут: https://github.com/finsight/QUIKSharp/network

Интересно, какие планы у @Daramant на этот форк?

@Pr0phet1c
Copy link
Collaborator

Интересно, какие планы у @Daramant на этот форк?

Да, я тоже не так давно заметил этот форк. Там все глобально.

@Pr0phet1c
Copy link
Collaborator

Я поддерживаю избаление от "зоопарка", но не ломая текущий код. Можете сделать параллельную имплементацию? Или это сильно завязано на внутренности?

Мне кажется (могу ошибаться), можно было бы попробовать обойтись малой кровью, если реализовать что-то типа:

async Task SendOrder(string classCode, string securityCode, string accountID, Operation operation, decimal price, int qty, TransactionType orderType, string clientCode = "")

@buybackoff
Copy link
Collaborator

Повторю основую философию: нужно повторить API Квик. Если я что-то изначально сделал дополнительно, то это была ошибка, нужно было делать дополнительную QuikSharp-specific API для облегчения работы с "зоопарком".

Поэтому если текущее поведение не на 100% соотвествует Квик, то стоит сделать как в Квик, и добавить наши функции, где здравого смысла usability побольше.

@Daramant
Copy link

Daramant commented Dec 20, 2020

@usikpavel Я сначала подумал, что это Вы с длинной историей ломающих все изменений тут: https://github.com/finsight/QUIKSharp/network

Интересно, какие планы у @Daramant на этот форк?

Первоначально, планы были: разобраться детально, как работает библиотека. В процессе - обратил внимание, что код местами усложнен и запутан, решил порефакторить, и что-то увлекся))
Сейчас планы: все же довести рефакторинг до конца + оптимизировать некоторые моменты работы библиотеки.

Дополнение. Отправленные Transaction будут бесконечно накапливаться в QuikService.Storage. Я пока не задумывался над его очисткой, т.к. в моем случае их количество достаточно мало, чтобы влиять на память/производительность робота.

Тоже заметил, переделаю эту часть.

@buybackoff
Copy link
Collaborator

buybackoff commented Dec 21, 2020

@Daramant

У Вас совсем всё breaking или public API неизменна? Планируете pool request?

Конечно в библиотекe куча технического долга. И мои знания в декабре 2014 не сравнить с текущими. Но подход был "не сломалось - не чини".

Если Вам комфортно с переделкой сложных внутренностей, могу добавить как co-maintainer сюда. Главное простое требование - не ломать публиный API.

@Daramant
Copy link

Daramant commented Dec 21, 2020

@Daramant

У Вас совсем всё breaking или public API неизменна? Планируете pool request?

В сожалению, да, много breaking changes, public api поменялось. Pool request - я бы и рад, но, т.к. много изменений, то кто такое примет))
Разве что, если вы согласны на создание QuikSharp V2.0 на основе моей ветки.

@buybackoff
Copy link
Collaborator

buybackoff commented Dec 21, 2020

В сожалению, да, много breaking changes, public api поменялось. Pool request - я бы и рад, но, т.к. много изменений, то кто такое примет))

Не совсем понятно, почему много breaking changes, если публичный API старается повторить API Квик. Если речь идет о других специфических для Quik# API, и Вам это интересно, то можно подумать о версии 3.0.

А можете в двух словах описать основные изменения именно функционала (кроме рефакторинга, чистки конверторов JSON.NET)? То есть что сейчас у нас не так, что может приводить к некорректной работе?

@Daramant
Copy link

Daramant commented Dec 21, 2020

А можете в двух словах описать основные изменения именно функционала (кроме рефакторинга, чистки конверторов JSON.NET)? То есть что сейчас у нас не так, что может приводить к некорректной работе?

На данный момент много изменений в плане рефакторинга: выделил интерфейсы, методы/события где-то переехали в другие классы, где-то изменились сигнатуры методов, вместо Message, ввел понятия: Command, Result, Event. Рефакторинг внутренней реализации классов. Изменения форматов данных по взаимодейтсвию с lua (например, зачем в .Net собирать строку из параметров через разделитель, а затем в lua ее обратно разбивать, если можно просто массив параметров (строк) передать в lua). До рефакторинга lua-скриптов еще не добрался.

Т.е. пока занимаюсь основным функционалом, в соответствии с целью библиотеки - пробросить функционал lua в .Net.
Из дополнительного функционала рядом - будет работа с транзакциями, возможно что-то еще.
Из доп. функционала были мысли: реализовать возможность фильтровать события на стороне сервера (в скрипте lua), чтобы даже не отправлять их через сокет клиенту. Т.е. в .Net отправлять только те события, на которые подписались.
Но тут надо проверять/тестировать, на сколько это ускорит работу и будет ли полезно.

То есть что сейчас у нас не так, что может приводить к некорректной работе?

Просто большое количество изменений, которые необходимо проверить.

@usikpavel
Copy link
Contributor Author

Согласен с замечаниями, breaking changes вообще делать было стрёмно, мне тут больше нужно было ваше мнение и критика - разобраться, правильно ли я понимаю идею и текущее состояние проекта, почему в коде иногда встречаются какие-то непонятные комменты, иногда не полное единообразие в вызовах к LUA и т.д.. :) И как лучше оформлять такие изменения, и вообще нужны ли они. Пулл-реквест понадобился для демонстрации кода и запуска обсуждения (вместо issue - сразу прошу прощения, что issue сразу не завел, подумал, что в pull-request можно все обсудить и выкинуть его/допилить). Спасибо за комменты, многое вроде прояснилось, еще и форк @Daramant -а показали, посмотрю. За саму либу тоже спасибо, для меня она хорошо и понятно написана и легко поддается изменениям, а главное - РАБОТАЕТ! Намучался с StockSharp...

Может, и в самом деле пора начать новую ветку/версию - выкинуть QuikService.Storage и оставить голые привязки к LUA, а все обвесы пилить в отдельном классе/репозитории? Или взять версию @Daramant и допилить ее.

По коду, в ближайшие дни внесу изменения и создам новую ветку с non-breaking changes. (Или сюда же коммит добавить?) Только изменения многие, получается, завязаны у меня на QuikService.Storage - нужны ли они библиотеке? Могу отфильтровать и не вносить их.

@usikpavel
Copy link
Contributor Author

usikpavel commented Dec 22, 2020

Забыл про clientCode написать. У меня на торговом счете (AccountNumber) на фондовом рынке есть субсчета, и к ним брокер сгенерировал разные clientCode. Вероятно, поэтому QUIK обязательно требует clientCode.
Также есть просто разные торговые счета - обычный и ИИС, у них свои AccountNumber и clientCode. Может быть, с этим тоже связано.

@Pr0phet1c
Copy link
Collaborator

В общем, по поводу ClientCode - если реализуете то, о чем я написал чуть раньше, то думаю, что PR можно будет принять. т.к. в такой реализации ничего не должно сломаться.
Если только кто-нибудь уже не нашел существенных недостатков у такого решения...

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.

4 participants