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

[Protocol3] Added circuit testing support + small improvements #396

Merged
merged 3 commits into from Aug 12, 2019
Merged

Conversation

Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented Aug 11, 2019

  • The circuits testing executable is made with make. All tests can be run with npm run test-circuits.
  • The rounding error check is now done on the rate of the actual trade (fillS/fillB) instead of on the rounding error on the division to calculate fillB. This should be more accurate and is cheaper.
  • When cancelling an order with an orderID > tradingHistory.orderID the filled amount is reset to 0. This accurately represent the filled amount of the orderID stored in the trading history (but the order is cancelled, so it doesn't actually matter for the protocol).

@dong77
Copy link
Contributor

dong77 commented Aug 11, 2019

When cancelling an order with an orderID > tradingHistory.orderID the filled amount is reset to 0. This accurately represent the filled amount of the orderID stored in the trading history (but the order is cancelled, so it doesn't actually matter for the protocol).

Let's assume one particular slot will be used by ID 1, 101, 201, .... 901... When we set the slot's current ID to 801 with some non-zero filed amount, does that guarantee that all orders (created or not) that have id=1, 101,...701 are considered canceled (fully matched)?

It is also my understanding that the filled amount set shall never be 0, otherwise, it will clear all history data for order 1, 101, ... 701.

@dong77 dong77 added the testing label Aug 11, 2019
@dong77 dong77 added this to In progress in Loopring Protocol via automation Aug 11, 2019
@dong77 dong77 moved this from In progress to Review in progress in Loopring Protocol Aug 11, 2019
@Brechtpd
Copy link
Contributor Author

Brechtpd commented Aug 11, 2019

When cancelling an order with an orderID > tradingHistory.orderID the filled amount is reset to 0. This accurately represent the filled amount of the orderID stored in the trading history (but the order is cancelled, so it doesn't actually matter for the protocol).

Let's assume one particular slot will be used by ID 1, 101, 201, .... 901... When we set the slot's current ID to 801 with some non-zero filed amount, does that guarantee that all orders (created or not) that have id=1, 101,...701 are considered canceled (fully matched)?

It is also my understanding that the filled amount set shall never be 0, otherwise, it will clear all history data for order 1, 101, ... 701.

There are 2^TREE_DEPTH_TRADING_HISTORY trading history leafs which store [filled, canceled, orderID]. The slot used by the order is orderID % 2^TREE_DEPTH_TRADING_HISTORY. So only orders with the same orderID after the modulo are affected by a trade history update for a specific slot (so 2^TREE_DEPTH_TRADING_HISTORY slots can be used independently from each other).

So let's say an order gets partially filled with filled := 100 for orderID := 2. The user creates another order with orderID := 2 + 2^TREE_DEPTH_TRADING_HISTORY. This order will use the same slot, but because new orderID > tradeHistory.orderIDit will override the current filled amount, so it will be as filled := 0. The order with orderID := 2 is now considered cancelled because orderID < tradeHistory.orderID and the filled amount is trimmed from the Merkle tree.

What was changed in this PR is that cancellations of orders only updated the canceled value in the leaf. So if orderID := 2 was filled for 100, but you cancel an order with orderID := 2 + 2^TREE_DEPTH_TRADING_HISTORYthe leaf would contain [100, true, 2 + 2^TREE_DEPTH_TRADING_HISTORY], which is not accurate because the order with the orderID that is now stored in the leaf was never filled. But this is just to be consistent, this doesn't matter for the protocol because canceled is true and the orderID is updated, so all orders with orderID <= orderID := 2 + 2 ^TREE_DEPTH_TRADING_HISTORY are canceled. The filled amount stored in the leaf will not be used any more by the protocol because the slot can now only be used by orders with orderID >= 2 + n * 2^TREE_DEPTH_TRADING_HISTORY with n >= 2, which will override the filled amount in the leaf.

@Brechtpd Brechtpd marked this pull request as ready for review August 11, 2019 22:40
Loopring Protocol automation moved this from Review in progress to Reviewer approved Aug 11, 2019
@dong77
Copy link
Contributor

dong77 commented Aug 11, 2019

When cancelling an order with an orderID > tradingHistory.orderID the filled amount is reset to 0. This accurately represent the filled amount of the orderID stored in the trading history (but the order is cancelled, so it doesn't actually matter for the protocol).

Let's assume one particular slot will be used by ID 1, 101, 201, .... 901... When we set the slot's current ID to 801 with some non-zero filed amount, does that guarantee that all orders (created or not) that have id=1, 101,...701 are considered canceled (fully matched)?
It is also my understanding that the filled amount set shall never be 0, otherwise, it will clear all history data for order 1, 101, ... 701.

There are 2^TREE_DEPTH_TRADING_HISTORY trading history leafs which store [filled, canceled, orderID]. The slot used by the order is orderID % 2^TREE_DEPTH_TRADING_HISTORY. So only orders with the same orderID after the modulo are affected by a trade history update for a specific slot (so 2^TREE_DEPTH_TRADING_HISTORY slots can be used independently from each other).

So let's say an order gets partially filled with filled := 100 for orderID := 2. The user creates another order with orderID := 2 + 2^TREE_DEPTH_TRADING_HISTORY. This order will use the same slot, but because new orderID > tradeHistory.orderIDit will override the current filled amount, so it will be as filled := 0. The order with orderID := 2 is now considered cancelled because orderID < tradeHistory.orderID and the filled amount is trimmed from the Merkle tree.

What was changed in this PR is that cancellations of orders only updated the canceled value in the leaf. So if orderID := 2 was filled for 100, but you cancel an order with orderID := 2 + 2^TREE_DEPTH_TRADING_HISTORYthe leaf would contain [100, true, 2 + 2^TREE_DEPTH_TRADING_HISTORY], which is not accurate because the order with the orderID that is now stored in the leaf was never filled. But this is just to be consistent, this doesn't matter for the protocol because canceled is true and the orderID is updated, so all orders with orderID <= orderID := 2 + 2 ^TREE_DEPTH_TRADING_HISTORY are canceled. The filled amount stored in the leaf will not be used any more by the protocol because the slot can now only be used by orders with orderID >= 2 + n * 2^TREE_DEPTH_TRADING_HISTORY with n >= 2, which will override the filled amount in the leaf.

This is something @hosschao and @lydy should take a look at.

@dong77 dong77 added the improvement New feature or request label Aug 12, 2019
@dong77 dong77 merged commit b41c442 into master Aug 12, 2019
Loopring Protocol automation moved this from Reviewer approved to Done Aug 12, 2019
@dong77 dong77 deleted the tests branch September 18, 2019 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature or request testing
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants