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

Make the core logic deal with tok instead of tez #228

Merged
merged 2 commits into from
Aug 10, 2021
Merged

Conversation

gkaracha
Copy link
Contributor

@gkaracha gkaracha commented Aug 9, 2021

Though this transition is pretty menial, it is a necessary first step for addressing #213, independently of the path we end up choosing (conditional compilation vs. having a tez/FA2 wrapper contract). By making all the core logic deal with an opaque-ish type tok we can defer the choice of collateral type to a higher level. With this PR checker.ml deals with tez—as it did before—and makes calls to tok_of_tez before calling core logic and tez_of_tok when interpreting results from core logic. No other module makes calls to these conversion functions (excluding the tests ofc).

While working on this I also noticed that our API uses tez in names which we probably want to rename to something more generic: Deposit_tez and Withdraw_tez (which I think we could rename to Deposit_collateral and Withdraw_collateral).

@github-actions
Copy link

Gas costs 760cab0 6e0b6d8 Diff
create_burrow 47817 48914 1097
touch 537877 538751 874
activate_burrow 53700 54538 838
deposit_tez 53015 53731 716
withdraw_tez 57567 58280 713
deactivate_burrow 59027 59284 257
mint_kit 52111 51865 -246
remove_liquidity 73790 73899 109
add_liquidity 73538 73647 109
buy_kit 69524 69633 109
sell_kit 68651 68760 109
Entrypoint sizes 760cab0 6e0b6d8 Diff
create_burrow 909 1097 188
activate_burrow 1147 1292 145
touch 56479 56614 135
deposit_tez 1041 1163 122
withdraw_tez 1376 1488 112
liquidation_auction_claim_win 978 1062 84
mint_kit 1637 1588 -49
touch_liquidation_slices 14471 14508 37
deactivate_burrow 1568 1603 35
mark_for_liquidation 17127 17093 -34
add_liquidity 1942 1967 25
buy_kit 1351 1376 25
remove_liquidity 2013 2038 25
sell_kit 1347 1372 25
liquidation_auction_place_bid 2229 2213 -16
cancel_liquidation_slice 12185 12177 -8
Test coverage 760cab0 6e0b6d8 Diff
tok.ml None 97.37 97.37
liquidationAuction.ml 78.14 77.83 -0.3100000000000023
burrow.ml 96.54 96.26 -0.28000000000000114
checker.ml 89.94 90.08 0.14000000000000057
TOTAL 93.93 93.96 0.029999999999986926

gkaracha and others added 2 commits August 10, 2021 17:09
So that we can defer the choice of collateral to a higher level.
@github-actions
Copy link

Gas costs 5a87e8a dab3ff6 Diff
create_burrow 47817 48914 1097
touch 537877 538751 874
activate_burrow 53700 54538 838
deposit_tez 53015 53731 716
withdraw_tez 57567 58280 713
deactivate_burrow 59027 59284 257
mint_kit 52111 51865 -246
remove_liquidity 73790 73899 109
add_liquidity 73538 73647 109
buy_kit 69524 69633 109
sell_kit 68651 68760 109
Entrypoint sizes 5a87e8a dab3ff6 Diff
create_burrow 909 1097 188
activate_burrow 1147 1292 145
touch 56479 56614 135
deposit_tez 1041 1163 122
withdraw_tez 1376 1488 112
liquidation_auction_claim_win 978 1062 84
mint_kit 1637 1588 -49
touch_liquidation_slices 14471 14508 37
deactivate_burrow 1568 1603 35
mark_for_liquidation 17127 17093 -34
add_liquidity 1942 1967 25
buy_kit 1351 1376 25
remove_liquidity 2013 2038 25
sell_kit 1347 1372 25
liquidation_auction_place_bid 2229 2213 -16
cancel_liquidation_slice 12185 12177 -8
Test coverage 5a87e8a dab3ff6 Diff
tok.ml None 97.37 97.37
liquidationAuction.ml 78.14 77.83 -0.3100000000000023
burrow.ml 96.54 96.26 -0.28000000000000114
checker.ml 89.94 90.08 0.14000000000000057
TOTAL 93.93 93.96 0.029999999999986926

Copy link
Contributor

@utdemir utdemir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :). I fixed a trivial issue and rebased, so if you want to make changes you might need a hard reset.

@gkaracha
Copy link
Contributor Author

LGTM :). I fixed a trivial issue and rebased, so if you want to make changes you might need a hard reset.

Thanks for having a look! 🙏 No more changes I think, this PR is large enough as it is; I'll go ahead and merge it 🙂

@gkaracha gkaracha merged commit fcca75d into master Aug 10, 2021
@gkaracha gkaracha deleted the gkaracha/tok branch August 10, 2021 07:39
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.

2 participants