Revamp docs/spec/functional_spec.rst

gkaracha commented 3 years ago

The spec was somewhat out of date; this PR addresses this.

Missing descriptions added for
- [X] touch
- [X] receive_price
- [X] receive_slice_from_burrow
- [X] liquidation_auction_claim_win
- [X] liquidation_auction_place_bid
- [X] cancel_liquidation_slice
- [X] touch_liquidation_slices
- [X] mark_for_liquidation
- [X] touch_burrow
Type signatures updated for
- [X] deactivate_burrow
- [X] burrow_max_mintable_kit
- [X] is_burrow_liquidatable
- [X] is_burrow_overburrowed
Notes added on entrypoints that receive tokens, to clarify that they will fail if the transfers needed are authorized and valid.
References to muctez are removed; we use whatever denomination ctez specifies and we've already removed references to mulqt and mukit anyway.
The type of ptr has been nat (instead of int) for some time now but we'd forgotten to update the spec. This is now fixed.
Misc consistency fixes.

Also I noticed that we don't explicitly say in the spec that for several entrypoints we fail if Tezos.amount <> 0. On one hand I think this is a bonus so we don't have to be so explicit about it, but on the other we do it pretty consistently and perhaps we'd like it to be a part of the spec? Not sure, but we should probably discuss this.

Also addresses #167.

github-actions[bot] commented 3 years ago

Gas costs	1313aa6c716c4a32fc7b0f94e418eb00db9fe56e	3e5d5603c0647af65fb8f9a573c6f7010bcc90f6	Diff
touch	538767	None	-538767
remove_liquidity	73903	None	-73903
add_liquidity	73651	None	-73651
buy_kit	69637	None	-69637
sell_kit	68764	None	-68764
deactivate_burrow	59288	None	-59288
withdraw_collateral	58284	None	-58284
set_burrow_delegate	54691	None	-54691
activate_burrow	54542	None	-54542
deposit_collateral	53735	None	-53735
mint_kit	51869	None	-51869
burn_kit	51613	None	-51613
create_burrow	48918	None	-48918
touch_burrow	44409	None	-44409
update_operators	41672	None	-41672
transfer	39928	None	-39928

Entrypoint sizes	1313aa6c716c4a32fc7b0f94e418eb00db9fe56e	3e5d5603c0647af65fb8f9a573c6f7010bcc90f6	Diff
touch	56614	None	-56614
mark_for_liquidation	17093	None	-17093
touch_liquidation_slices	14508	None	-14508
cancel_liquidation_slice	12177	None	-12177
liquidation_auction_place_bid	2213	None	-2213
remove_liquidity	2038	None	-2038
add_liquidity	1967	None	-1967
deactivate_burrow	1603	None	-1603
mint_kit	1588	None	-1588
burn_kit	1561	None	-1561
withdraw_collateral	1488	None	-1488
buy_kit	1376	None	-1376
sell_kit	1372	None	-1372
activate_burrow	1292	None	-1292
deposit_collateral	1163	None	-1163
create_burrow	1097	None	-1097
liquidation_auction_claim_win	1062	None	-1062
set_burrow_delegate	1059	None	-1059
touch_burrow	660	None	-660
update_operators	468	None	-468
receive_price	155	None	-155
receive_slice_from_burrow	119	None	-119

Test coverage	1313aa6c716c4a32fc7b0f94e418eb00db9fe56e	3e5d5603c0647af65fb8f9a573c6f7010bcc90f6	Diff
ctez.ml	100	None	-100
parameters.ml	100	None	-100
lqt.ml	100	None	-100
cfmmTypes.ml	100	None	-100
checkerEntrypoints.ml	100	None	-100
ptr.ml	100	None	-100
liquidationAuctionTypes.ml	100	None	-100
fixedPoint.ml	100	None	-100
common.ml	100	None	-100
error.ml	100	None	-100
mem.ml	100	None	-100
checkerTypes.ml	100	None	-100
fa2Interface.ml	100	None	-100
kit.ml	100	None	-100
constants.ml	100	None	-100
cfmm.ml	100	None	-100
tok.ml	97.37	None	-97.37
burrow.ml	96.26	None	-96.26
sliceList.ml	95.38	None	-95.38
checkerMain.ml	92.86	None	-92.86
avl.ml	91.05	None	-91.05
checker.ml	90.08	None	-90.08
liquidationAuction.ml	77.83	None	-77.83
TOTAL	93.96	None	-93.96

utdemir commented 3 years ago

re. the above comment.

It seems like we are skipping our build workflows if only changed files are documentation:

  pull_request:
    paths-ignore:
      - '**/*.md'
      - 'docs/**/*'
      - '.readthedocs.yaml'
  push:
    paths-ignore:
      - '**/*.md'
      - 'docs/**/*'
      - '.readthedocs.yaml'

So the commits does not have the build artifacts for documentation changes, and our script interprets it as everything being deleted. We should probably fix the script to ignore commits if artifacts are not there.

gkaracha commented 3 years ago

@utdemir re. the comments on touching and the reward: the way I understood it the "functional" spec is to be more about what happens, rather than why it happens, that's why I didn't add more text about the rationale. Personally I'd also like some more elaboration on the system but I am not sure if this is the place for it. On the other hand, we don't have the calculations in this document either (which would make it more "functional") so I am not sure what should and what should not be included in this file :thinking: @purcell any thoughts?

github-actions[bot] commented 3 years ago

Gas costs	1313aa6c716c4a32fc7b0f94e418eb00db9fe56e	213f657ef78217b213d382c76ab6fab280fe820b	Diff
touch	538767	None	-538767
remove_liquidity	73903	None	-73903
add_liquidity	73651	None	-73651
buy_kit	69637	None	-69637
sell_kit	68764	None	-68764
deactivate_burrow	59288	None	-59288
withdraw_collateral	58284	None	-58284
set_burrow_delegate	54691	None	-54691
activate_burrow	54542	None	-54542
deposit_collateral	53735	None	-53735
mint_kit	51869	None	-51869
burn_kit	51613	None	-51613
create_burrow	48918	None	-48918
touch_burrow	44409	None	-44409
update_operators	41672	None	-41672
transfer	39928	None	-39928

Entrypoint sizes	1313aa6c716c4a32fc7b0f94e418eb00db9fe56e	213f657ef78217b213d382c76ab6fab280fe820b	Diff
touch	56614	None	-56614
mark_for_liquidation	17093	None	-17093
touch_liquidation_slices	14508	None	-14508
cancel_liquidation_slice	12177	None	-12177
liquidation_auction_place_bid	2213	None	-2213
remove_liquidity	2038	None	-2038
add_liquidity	1967	None	-1967
deactivate_burrow	1603	None	-1603
mint_kit	1588	None	-1588
burn_kit	1561	None	-1561
withdraw_collateral	1488	None	-1488
buy_kit	1376	None	-1376
sell_kit	1372	None	-1372
activate_burrow	1292	None	-1292
deposit_collateral	1163	None	-1163
create_burrow	1097	None	-1097
liquidation_auction_claim_win	1062	None	-1062
set_burrow_delegate	1059	None	-1059
touch_burrow	660	None	-660
update_operators	468	None	-468
receive_price	155	None	-155
receive_slice_from_burrow	119	None	-119

Test coverage	1313aa6c716c4a32fc7b0f94e418eb00db9fe56e	213f657ef78217b213d382c76ab6fab280fe820b	Diff
ctez.ml	100	None	-100
parameters.ml	100	None	-100
lqt.ml	100	None	-100
cfmmTypes.ml	100	None	-100
checkerEntrypoints.ml	100	None	-100
ptr.ml	100	None	-100
liquidationAuctionTypes.ml	100	None	-100
fixedPoint.ml	100	None	-100
common.ml	100	None	-100
error.ml	100	None	-100
mem.ml	100	None	-100
checkerTypes.ml	100	None	-100
fa2Interface.ml	100	None	-100
kit.ml	100	None	-100
constants.ml	100	None	-100
cfmm.ml	100	None	-100
tok.ml	97.37	None	-97.37
burrow.ml	96.26	None	-96.26
sliceList.ml	95.38	None	-95.38
checkerMain.ml	92.86	None	-92.86
avl.ml	91.05	None	-91.05
checker.ml	90.08	None	-90.08
liquidationAuction.ml	77.83	None	-77.83
TOTAL	93.96	None	-93.96

gkaracha commented 3 years ago

re. the above comment.

It seems like we are skipping our build workflows if only changed files are documentation:
  pull_request:
    paths-ignore:
      - '**/*.md'
      - 'docs/**/*'
      - '.readthedocs.yaml'
  push:
    paths-ignore:
      - '**/*.md'
      - 'docs/**/*'
      - '.readthedocs.yaml'
So the commits does not have the build artifacts for documentation changes, and our script interprets it as everything being deleted. We should probably fix the script to ignore commits if artifacts are not there.

Now that I think about it, I think I'd prefer if we always had artifacts for master. Might be a little wasteful on some occasions, but I think it's safer and more consistent; I'd remove the following part from .github/workflows/ci.yml:

  push:
    paths-ignore:
      - '**/*.md'
      - 'docs/**/*'
      - '.readthedocs.yaml'

purcell commented 3 years ago

Personally I'd also like some more elaboration on the system but I am not sure if this is the place for it. On the other hand, we don't have the calculations in this document either (which would make it more "functional") so I am not sure what should and what should not be included in this file 🤔 @purcell any thoughts?

Things that are internal details probably don't belong in the document, as far as I am concerned. The endpoint documentation so far serves primarily as a user-level / API-level guide, ie. the information needed to use Checker. I'm generally wary of writing too much about the code in the documentation if it's not helpful to a specific target reader, because the two will inevitably get out of sync.

tezos-checker / checker

Revamp docs/spec/functional_spec.rst #231