Feat/order book add fulfill order partially #1559
Conversation
cdamian
force-pushed
the
feat/order-book-add-fulfill-order-partially
branch
from
1cb3ac7
to
97c6940
Compare
pallets/order-book/src/lib.rs
Outdated
| let partial_fulfillment = !remaining_buy_amount.is_zero(); | ||
|
|
||
| ensure!( | ||
| partial_buy_amount >= order.min_fulfillment_amount, |
There was a problem hiding this comment.
This would mean the min_fullfillment_amount is not a global min but rather a min_per_buy. I am okay with that, just wanted to channel this by you @hieronx .
There was a problem hiding this comment.
Please let me know if we are OK with this, otherwise we can add a check for the remaining buy amount to ensure that it doesn't go too low, even though I'm not sure if that makes sense ^^
There was a problem hiding this comment.
No, I was rather thinking that it is a one-time amount. Like, I wanna get at least x. But it is fine to go with the given solution.
cdamian
force-pushed
the
feat/order-book-add-fulfill-order-partially
branch
from
97c6940
to
6c9421b
Compare
| order_id, | ||
| placing_account: order.placing_account, | ||
| fulfilling_account: ACCOUNT_1, | ||
| partial_fulfillment: true, |
There was a problem hiding this comment.
AFAICS, the test should fail here if fulfillment_ratio is 100%?!
There was a problem hiding this comment.
Yes, but it doesn't get to 100 in this test. The 100 percent case is handled separately given the different expected events.
There was a problem hiding this comment.
Ah dumb me forgetting that 0..100 != 0..=100 in Rust 🙈
There was a problem hiding this comment.
Nah, that's on me since I did not find a proper way to have a separate test for each percentage using a macro. If we get a failure here now, one would have to debug and figure out which percentage is problematic, which is awkward to say the least.
Maybe @lemunozm can help ^^
There was a problem hiding this comment.
IMO it's the current test is more than sane. Theoretically, you could add a debug message to the assertion(s) to populate the failure fulfillment_ratio.
There was a problem hiding this comment.
Sorry, just saw this now.
Does each different value of fulfillment_ratio use a different path in the code? Maybe it's unnecessary to test all values and only do it with corner cases, for example, with 1, 50, 99.
The macro would be cool, but doesn't hurt to leave it as it is and print the ratio on each iteration. To the purists 😄, exists a catch_unwind that can be used in these cases to add some extra information when the test fails. For example, adding the fulfillment_ratio value in the error message used in that iteration. Maybe I will add it to the fuzzer.
… extra test setup
cdamian
force-pushed
the
feat/order-book-add-fulfill-order-partially
branch
from
c6002c4
to
a9b6cfd
Compare
| order_id, | ||
| placing_account: order.placing_account, | ||
| fulfilling_account: ACCOUNT_1, | ||
| partial_fulfillment: true, |
There was a problem hiding this comment.
IMO it's the current test is more than sane. Theoretically, you could add a debug message to the assertion(s) to populate the failure fulfillment_ratio.
Description
Added a new extrinsic in the
order-bookpallet that allows for a partial order fulfillment.Checklist: