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

added extra optional parameter "bid_amount_money" to "order.put_market_stock" RPC. #36

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

Conversation

djpnewton
Copy link
Contributor

Default value is true and has no change to behavior. If the value is false then the "amount" parameter of a market bid is treated as "stock" instead of "money"

see also issue #32

replaces #35

djpnewton added a commit to djpnewton/via_jsonrpc that referenced this pull request Nov 22, 2017
@andrewshvv
Copy link

andrewshvv commented Dec 24, 2017

@djpnewton @haipome Any plans to merge it? 😄

@djpnewton
Copy link
Contributor Author

its up to @haipome but I would be happy

@andrewshvv
Copy link

@djpnewton Hey, any updates on this guys?

@djpnewton
Copy link
Contributor Author

I guess I just need some feedback from @haipome on whether he is ok with the direction of this PR

}
} else {
if (mpd_cmp(taker->left, maker->left, &mpd_ctx) < 0) {
mpd_copy(amount, taker->left, &mpd_ctx);
Copy link

@HumanSwissArmyKnifeQP HumanSwissArmyKnifeQP May 25, 2018

Choose a reason for hiding this comment

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

(More of a stylistic and lower priority comment than anything else.)

For a file of this size, could we not replace many of the if/else blocks with a ternary operator, to improve code readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure but perhaps that kind of stylistic change should probably be in a dedicated PR

@compdzwio
Copy link

compdzwio commented Aug 26, 2018

fuck, me_loader.c int ret = market_put_market_order(false, NULL, market, user_id, side, amount, taker_fee, source, /* bid_amount_money what to do? */);

me_loader.c Market records for loading databases, but the database does not have bid_amount_money, How to fill bid_amount_money when loading?

@djpnewton
Copy link
Contributor Author

I have rebased this PR to master

@jkmaina
Copy link

jkmaina commented Dec 14, 2018

when i check the deal.stock and deal.money on finished orders using the change with the optional parameter market orders, the returned values are zero. Is this expected behavior?

@haipome
Copy link
Member

haipome commented Mar 12, 2019

This function is useful. I will read and test careful before merge it into master

@mahdi13
Copy link

mahdi13 commented Aug 6, 2019

This code has a serious bug. Sometimes the deal's money exceeds the user's balance. Because of the calculation problem on these lines of code:

        } else {
            if (mpd_cmp(taker->left, maker->left, &mpd_ctx) < 0) {
                mpd_copy(amount, taker->left, &mpd_ctx);
            } else {
                mpd_copy(amount, maker->left, &mpd_ctx);
            }
        }

Then matching engine try to subtract this money value from the balance:

balance_sub(taker->user_id, BALANCE_TYPE_AVAILABLE, m->money, deal);

But the balance is not enough and the balance_sub method return NULL value. No one checks the returned value after this method call. So we'll have a duplicated balance value on the user balance history table. Actually, we'll have a subtract record (minus change value) on db, but the balance field of this record is the same as the previous record's balance field. The result is that the user put a bid market order and receive the requested stock amount without any change in it's money balance

@mahdi13
Copy link

mahdi13 commented Aug 7, 2019

The problem is on balance calculation. This line should be moved after the balance check:

mpd_del(money_required);

@djpnewton
Copy link
Contributor Author

oh i see, money_required is being freed too soon

@djpnewton
Copy link
Contributor Author

updated

@djpnewton
Copy link
Contributor Author

rebased

the "amount" parameter now refers to the order "stock"
…t_stock" RPC.

Default value is true and has no change to behavior. If the value is false then the "amount" parameter of a market bid is treated as "stock" instead of "money"
@ceyonur
Copy link
Contributor

ceyonur commented Nov 29, 2020

any update on this? currently this functionality cannot be used as: "I want to buy x amount stock from the market". I believe this is a de facto standard among exchange services. They all provide "stock amount" in market orders.

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.

8 participants