tezos-checker / checker

An in-development "robocoin" system for the Tezos blockchain
24 stars 16 forks source link

Refactor burrow slice list logic into separate module #131

Closed dorranh closed 3 years ago

dorranh commented 3 years ago

Wanted to go ahead and get this up here to discuss before making any further changes, please let me know what you think!

This PR moves the logic for working with the double-linked lists formed by burrow slices our of liquidationAuction.ml into its own module. The goal of this refactor is to reduce the number of things going on in the various functions in liquidationAuction.ml in order to make them easier to test.

Note that I have kept this totally internal to make the refactoring a bit easier (and avoid changes to the existing tests), so the types stored in the auction state are the same as before. This adds a bit of extra work for the endpoints operating on the linked lists. I also noticed that the size of a couple of the endpoints increased by ~100-200 bytes and wanted to check if that was an issue given our current size and deployment approach.

The new SliceList module acts as a high-level interface to the AVL queue(s). I originally was going to make it only handle the list logic, but it turns out that we always need to update the burrow slice lists whenever we send or remove a slice from the AVL. The challenges in the seemingly complex functions like take_with_splitting actually boil down to retrieving the appropriate list and element from the AVL storage and are handled via a couple of constructors (e.g. slice_list_from_queue_head)

If we want to move forward with this solution I can do something similar for the completed auctions lists. I originally wanted to make the module in this PR polymorphic to avoid having a similar-looking module, but doing so in Ligo seems to be impossible as far as I can tell.

utdemir commented 3 years ago

I think this is better than the existing code. However to be honest I find both this module and the existing one error prone, there is just too many things that can be missed when using them.

I liked the idea of having a subset of auctions type focused to a specific burrow id (SliceList) and manipulating it; but it also suggests that maybe there is a different way of storing our state that makes this representation change unnecessary. I think this is what you meant here, right?:

so the types stored in the auction state are the same as before. This adds a bit of extra work for the endpoints operating on the linked lists.


I also noticed that the size of a couple of the endpoints increased by ~100-200 bytes and wanted to check if that was an issue given our current size and deployment approach.

I don't think it specifically is a big issue (although less would be better :) ). It's also possible that inlining some functions will make it better. But I am suprised that we didn't end up simplifying any redundant code paths.


In short, I think it is good to merge this one since it is better than the existing solution; but it still is probably the most brittle piece of checker. We got to find a way to simplify these even more.

dorranh commented 3 years ago

Thanks for the reviews! I'm going to move this PR to a draft state until we have more time to discuss (see #135) since I don't think it adds much in terms of test coverage, etc. in the immediate term.

dorranh commented 3 years ago

@utdemir @gkaracha, thanks again for the initial feedback the other day! I've made the request changes but am going to wait on @gkaracha's PR reducing the gas cost of sealing the contract before moving forward. That way I can test whether the changes in this PR exceed the gas limit.

dorranh commented 3 years ago

It appears that this change blows up the gas cost of operations which interact with liquidationAuction.ml beyond the gas limit (sealContract now exceeds the gas limit when deploying). We shouldn't merge this until the gas costs have been reduced.

dorranh commented 3 years ago

Alright, finally got the gas costs down! Here's a comparison with master:

(this PR @ 8d34def0e6c90be9b3a4ff6107331f8fff078b63)

Gas costs:
{
    "touch": "541631",
    "remove_liquidity": "74707",
    "add_liquidity": "74076",
    "buy_kit": "70115",
    "sell_kit": "69237",
    "deactivate_burrow": "57355",
    "withdraw_tez": "55740",
    "set_burrow_delegate": "52038",
    "mint_kit": "51482",
    "deposit_tez": "50414",
    "create_burrow": "47993",
    "touch_burrow": "45015"
}

master @ a121393015ee82d5660167a426146767dd31749b

Gas costs:
{
    "touch": "530610",
    "remove_liquidity": "74707",
    "add_liquidity": "74076",
    "buy_kit": "70115",
    "sell_kit": "69240",
    "deactivate_burrow": "57355",
    "withdraw_tez": "55740",
    "set_burrow_delegate": "52038",
    "mint_kit": "51482",
    "deposit_tez": "50414",
    "create_burrow": "47993",
    "touch_burrow": "45015"
}