Add test for order quotes #331
Conversation
this adds the tables orders, order_quotes, trades to the test database. a new test using this data was added. it covers - market orders (normal reward) - partially fillable orders (no reward) - in market limit orders (no reward)
There was a problem hiding this comment.
Looks nice although I didn't explicitly try and follow the example logic but assume you captured the case you were hoping for.
You might want to keep the CIP-20 data around for backwards compatibility, but I guess it's in the history anyway and someone can checkout older versions if they want to do older calculations.
At the moment, the data for all tests is contained in this one database. This includes the data for testing CIP-20 batch rewards. (Due to the renaming this is not obvious.) I was thinking about splitting the data for different tests: one for batch rewards and one for quote rewards. On a different note: I can now appreciate way more the way tests are written in this repo, @bh2smith . Having a dedicated small database for unit testing is great! |
|
If i understand correctly, this is a test db where tables are created and dropped on the spot, right? If so, is it necessary to add all this extra information, which we don't actually need for what we are testing here? |
Yes, this is just a test database which creates tables on the spot for tests. Which extra information are you referring to? What was added was 3 tables used in the quotes query. Those tables contain information which is not used (e.g. order creation time stamp). This was done to follow the schema for the table. There is also additional information in the database for batch rewards. One thing that would be possible is to split the database into two, one for each test: batch rewards and quote rewards. For the quote test this will work nicely. For the protocol fee test this will not really simplify things since essentially the combined information from the two databases is needed. |
|
Information such as or
Not saying we should drop all these, and might need them for other tests, but it felt a bit noisy. We could also simplify the addresses of the solvers in the examples, e.g.,
Again these are not real issues, just could make the whole test easier to read and check |
The current approach is to set up the tables as in the actual database. This implies this that unused data is included. We could also just add the data that we actually need.
As to why the data is filled as is: not sure why the original table contained the longer solver names. Might have to do with other tests which were run locally on that data. Then the address is checked to have the correct length. I could change the additional data to something like |
|
I also suspect we can make the numbers small so that we can easily compare them. E.g., sell_amount = 10, fee = 3, buy_amount = 7 etc... |
- use less data - use separate file
I made the numbers a bit easier, but mostly just use the same numbers as for the protocol fee test in the quote test. They correspond to trades of the form 100 USDC to 100 DAI now. For quotes it should be fine to use really small numbers. For the original test (and the protocol fee test) it is useful to have realistically large numbers so as to also check for issues with prices, decimals, overflow, etc. |
| ('\x05'::bytea, 100000000, 94000000000000000000, 0, 'sell', 'f'), -- in market sell limit order | ||
| ('\x06'::bytea, 106000000, 100000000000000000000, 0, 'buy', 'f'); -- in market buy limit order | ||
|
|
||
| INSERT INTO order_quotes (order_uid, sell_amount, buy_amount, solver) |
There was a problem hiding this comment.
the order_quotes table does have a fee entry but i am fine with assuming there is none here and instead treat sell_amount as the total amount users need to send to the contract, according to the quote.
There was a problem hiding this comment.
If I understand correctly, the order_quotes table does not have a fee entry. It does have gas_amount, gas_price, and sellt_token_price but due to hooks this is not enough to recover fees.
For market orders we can use the amount in the orders or trades table. For limit orders I am not sure what to do. Is there a way to recover the estimated fee amount or does quoting maybe work differently for such orders?
There was a problem hiding this comment.
Oh you are right, it is implicit.
| ('\x02'::bytea, 101000000, 100000000000000000000, '\x02'::bytea), | ||
| ('\x03'::bytea, 100000000, 95000000000000000000, '\x03'::bytea), | ||
| ('\x04'::bytea, 105000000, 100000000000000000000, '\x03'::bytea), | ||
| ('\x05'::bytea, 100000000, 95000000000000000000, '\x03'::bytea), |
There was a problem hiding this comment.
Let's look at order \x05. I think that we should have order_quotes.sell_amount < orders.sell_amount, as the quote takes into account the fee.
There was a problem hiding this comment.
I just checked our database and it seems that for limit orders, the sell amount is not reduced by some fee amount. E.g.
select * from orders where uid = '\xD3C1CE97725452DFF58BDC755DDE48E0A7420046075E092BCBC2F397484A287DBDBCE09CF017D8BD51C88B988297B594BD2D3F1065A5E83A' and select * from order_quotes where order_uid = '\xD3C1CE97725452DFF58BDC755DDE48E0A7420046075E092BCBC2F397484A287DBDBCE09CF017D8BD51C88B988297B594BD2D3F1065A5E83A' both return the same sell amount for some recent order.
There was a problem hiding this comment.
Yeah you are probably right, although I find this a bit confusing and want to look into how the order_quotes table is created. Somewhat irrelevant to this PR though
This PR adds a test for the quote rewards query.
The tables
orders,order_quotes,tradeswere added to the test database.A new unit test using this data was added. It covers:
At the moment, the data in the test database is not consistent: e.g. the surplus from the first settlement does not actually correspond to the surplus in the
settlement_scorestable. This problem could be avoided by choosing more appropriate traded amount or by using a separate data base for the test.I opted for the simplest approach since any test would be better than having no tests at all.