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.

Sign up for GitHub

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

Jump to bottom

Move order submit to order_processor and remove Ordering service #2147

Merged
merged 1 commit into from
Apr 3, 2019
Merged

Move order submit to order_processor and remove Ordering service #2147

merged 1 commit into from
Apr 3, 2019

Conversation

mnaichuk
Copy link
Member

@mnaichuk mnaichuk commented Mar 29, 2019 •
edited by ysv
Loading

  • Add new states pending & reject for orders;
  • Add fast check_balance before publishing message;
  • Publish message for balance update in OrderProcessor from API;
  • Delete Ordering service;

@mnaichuk mnaichuk force-pushed the 2-1/create-on-daemon branch 2 times, most recently from dd774e3 to 86d2746 Compare March 29, 2019 11:41
ysv
ysv reviewed Mar 29, 2019
View reviewed changes
app/workers/worker/order_processor.rb Outdated

AMQPQueue.enqueue(:matching, action: 'submit', order: order.to_matching_attributes)
end
rescue e
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there was an issue on order submit wee need probably to reject it

@mnaichuk mnaichuk force-pushed the 2-1/create-on-daemon branch 7 times, most recently from 2120670 to 073ca61 Compare April 1, 2019 14:03
ysv
ysv suggested changes Apr 1, 2019
View reviewed changes
app/api/v2/helpers.rb Outdated
order
rescue => e
message = create_order_errors.fetch(e.class, 'market.order.create_error')
report_exception_to_screen(e)
error!({ errors: [message] }, 422)
end

def submit(order)
ActiveRecord::Base.transaction do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need transaction here

app/api/v2/helpers.rb Outdated
{ persistent: false }
end

def cancel(order)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to move trading related method into separate Helper

app/workers/worker/order_processor.rb Outdated
order = Order.find_by_id(payload.dig('order', 'id'))
cancel(order) if order
when 'submit'
order = Order.find_by_id(payload.dig('order', 'id'))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use strict find_by_id!
  • In case if order doesn't exist publish message to trade_errors queue
    Optional: Remove duplicated
order = Order.find_by_id(payload.dig('order', 'id'))

app/workers/worker/order_processor.rb Outdated
order = Order.find_by_id(payload.dig('order', 'id'))
cancel(order) if order
when 'submit'
order = Order.find_by_id(payload.dig('order', 'id'))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lock order directly after finding

app/workers/worker/order_processor.rb Outdated
end
end

private

def submit(order)
order.with_lock do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use active_record transaction here and lock in process

app/trading/matching/executor.rb Outdated
raise_error(3003, 'Ask state isn\'t equal to «wait».') unless @ask.state == Order::WAIT
raise_error(3004, 'Bid state isn\'t equal to «wait».') unless @bid.state == Order::WAIT
raise_error(3003, "Ask state isn\'t equal to «wait» (#{ask.state}).") unless @ask.state == Order::WAIT
raise_error(3004, "Bid state isn\'t equal to «wait» (#{ask.state}).") unless @bid.state == Order::WAIT
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bid.state

app/workers/worker/order_processor.rb Outdated
def cancel(order)
Ordering.new(order).cancel!
rescue StandardError => e
order.with_lock do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use active_record transaction here and lock in process

app/api/v2/helpers.rb Outdated
@@ -65,6 +65,10 @@ def build_order(attrs)
origin_volume: attrs[:volume]
end

def check_balance(order)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong business logic for bid order and spec

@mnaichuk mnaichuk force-pushed the 2-1/create-on-daemon branch 2 times, most recently from c415437 to c8cd1ef Compare April 2, 2019 09:03
ysv
ysv suggested changes Apr 2, 2019
View reviewed changes
app/api/v2/ordering.rb Outdated
@@ -0,0 +1,61 @@
module API
module V2
module Ordering
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OrderHelpers

app/workers/worker/order_processor.rb Outdated
def process(payload)
order = Order.find_by_id!(payload.dig('order', 'id'))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you nee to lock order here right after finding

app/workers/worker/order_processor.rb Outdated
@@ -3,21 +3,56 @@

module Worker
class OrderProcessor
def initialize
Order.where(state: ::Order::PENDING).each do |order|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use find_each here

app/workers/worker/order_processor.rb Outdated
end

private

def submit(order)
ActiveRecord::Base.transaction do
order.with_lock do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need with_lock here if you lock order here right after finding

app/workers/worker/order_processor.rb Outdated
Ordering.new(order).cancel!
rescue StandardError => e
ActiveRecord::Base.transaction do
order.with_lock do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need with_lock here if you lock order here right after finding

@mnaichuk mnaichuk force-pushed the 2-1/create-on-daemon branch 6 times, most recently from aea8f01 to 94e0cb5 Compare April 3, 2019 13:52
Move order submit to order_processor and remove Ordering
94e0cb5 
- Add new state `Pending` for orders;
- Add fast check_balance before publishing message;
- Publish message for submitting order from API;
- Delete Ordering module;
@ysv ysv added PR: Approved and removed PR: WIP labels Apr 3, 2019
@mod mod merged commit 0fae99d into openware:master Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysv ysv ysv approved these changes

Assignees
No one assigned
Labels
PR: Approved v2.1
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mnaichuk @ysv @mod