-
Notifications
You must be signed in to change notification settings - Fork 4
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
[Orderbook]: Wiring up tick to price to fulfillments #49
Conversation
price: Decimal256, | ||
) -> ContractResult<Uint128> { | ||
match order { | ||
OrderDirection::Bid => multiply_by_price(amount, price), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanity check on this mapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance this looks correct to me
.checked_mul(Decimal256::new(Uint256::from(amount)))? | ||
.to_uint_ceil(); | ||
|
||
// TODO: Rounding? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we decide on ceiling or floor rounding?
quantity: Uint128::MAX | ||
} | ||
); | ||
let amount_to_send = Uint128::from_str(amount_to_send_u256.to_string().as_str()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible unforseen issues by converting from u256 to u128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm okay this does seem problematic. If we can only operate with u128 due to coin precision this is a bit of an issue, as we would need to constrain prices quite a bit to make that work. Worth discussing separately.
In the meantime, we should be catching overflows here
contracts/orderbook/src/tick_math.rs
Outdated
pub fn divide_by_price(amount: Uint128, price: Decimal256) -> ContractResult<Uint128> { | ||
let amount_to_send_u256 = price | ||
.checked_div(Decimal256::new(Uint256::from(amount)))? | ||
.to_uint_ceil(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be truncating regardless right?
Also I just realized we should be passing in this rounding direction to the tick_to_price
conversion as well instead of always truncating for it. Flagged in this issue #50
price: Decimal256, | ||
) -> ContractResult<Uint128> { | ||
match order { | ||
OrderDirection::Bid => multiply_by_price(amount, price), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance this looks correct to me
quantity: Uint128::MAX | ||
} | ||
); | ||
let amount_to_send = Uint128::from_str(amount_to_send_u256.to_string().as_str()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm okay this does seem problematic. If we can only operate with u128 due to coin precision this is a bit of an issue, as we would need to constrain prices quite a bit to make that work. Worth discussing separately.
In the meantime, we should be catching overflows here
Closing this since it was merged as part of #56 |
Closes: #43
What is the purpose of the change
Previous filling logic assumed a 1 to 1 price ratio for every tick. With the addition of #41 the actual price ratio can be calculated and wired up to order filling. This is done via the
amount_to_value
method which maps the order direction to the correct multiplication or division method.This was added in two places:
order.fill(...)
)Testing and Verifying
Added basic tests for multiply/divide by price in
test_tick_math.rs
:Adjusted previous limit tests for pricing adjustments.