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

[Orderbook]: Wiring up tick to price to fulfillments #49

Closed
wants to merge 8 commits into from

Conversation

crnbarr93
Copy link
Collaborator

@crnbarr93 crnbarr93 commented Feb 27, 2024

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:

  1. When filling a limit order (order.fill(...))
  2. When running a market order

Testing and Verifying

Added basic tests for multiply/divide by price in test_tick_math.rs:

cargo unit-test test_divide_by_price
cargo unit-test test_multiply_by_price

Adjusted previous limit tests for pricing adjustments.

price: Decimal256,
) -> ContractResult<Uint128> {
match order {
OrderDirection::Bid => multiply_by_price(amount, price),
Copy link
Collaborator Author

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?

Copy link
Collaborator

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?
Copy link
Collaborator Author

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();
Copy link
Collaborator Author

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

Copy link
Collaborator

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

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();
Copy link
Collaborator

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),
Copy link
Collaborator

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();
Copy link
Collaborator

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

@crnbarr93 crnbarr93 changed the title [WIP/Orderbook]: Wiring up tick to price to fulfillments [Orderbook]: Wiring up tick to price to fulfillments Mar 1, 2024
@AlpinYukseloglu
Copy link
Collaborator

Closing this since it was merged as part of #56

@AlpinYukseloglu AlpinYukseloglu deleted the connor/tick-to-price branch April 7, 2024 20:59
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.

[Orderbook]: Wire up tick to price conversion into market filling logic
2 participants