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

Touch the oldest completed slice when marking a burrow for liquidation #240

Draft
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

gkaracha
Copy link
Contributor

@gkaracha gkaracha commented Aug 16, 2021

Closes #155.

I am opening this as a draft PR already so that it's easier to evaluate the gas cost differences.

By looking at this I realized that we have quite some code duplication around touching liquidation slices. touch_liquidation_slice is the function processing a single slice, but then we have

  1. touch_oldest that touches the oldest Constants.number_of_slices_to_process completed slices,
  2. touch_liquidation_slices_rec that touches a list of completed slices passed as an argument, and now
  3. entrypoint_mark_for_liquidation also finds the oldest completed slice and then touches it.

I am thinking that perhaps this is a good time to restructure the code a little to reduce duplication, as part of this PR. If we implement find_oldest_slices : Ligo.nat -> liquidation_auctions -> leaf_ptr list, then (1) and (3) can be simplified quite some:

  • touch_oldest can be rewritten (inlined, really) as touch_liquidation_slices_rec (find_oldest_slices Constants.number_of_slices_to_process auctions), and
  • entrypoint_mark_for_liquidation can now also simply call touch_liquidation_slices_rec (find_oldest_slices 1 auctions).

I'll have a go and see if gas costs allow for these changes.

@gkaracha gkaracha force-pushed the gkaracha/touch-when-marking branch 3 times, most recently from 77d595c to 25d5979 Compare August 17, 2021 11:35
@gkaracha
Copy link
Contributor Author

Sidenote: experiment of the find_oldest_slices/touch_slices separation here. By now I wonder if any of these changes is really possible, given gas costs.

@dorranh
Copy link
Contributor

dorranh commented Oct 1, 2021

Gas costs 335029d 4849a4b Diff
touch 537452 537403 -49
buy_kit 69339 69341 2
Entrypoint sizes 335029d 4849a4b Diff
mark_for_liquidation 17093 33792 16699
touch 56590 56586 -4
Test coverage 335029d 4849a4b Diff
checker.ml 90.18 89.82 -0.36000000000001364
TOTAL 93.93 93.85 -0.0800000000000125

3 similar comments
@dorranh
Copy link
Contributor

dorranh commented Oct 1, 2021

Gas costs 335029d 4849a4b Diff
touch 537452 537403 -49
buy_kit 69339 69341 2
Entrypoint sizes 335029d 4849a4b Diff
mark_for_liquidation 17093 33792 16699
touch 56590 56586 -4
Test coverage 335029d 4849a4b Diff
checker.ml 90.18 89.82 -0.36000000000001364
TOTAL 93.93 93.85 -0.0800000000000125

@dorranh
Copy link
Contributor

dorranh commented Oct 1, 2021

Gas costs 335029d 4849a4b Diff
touch 537452 537403 -49
buy_kit 69339 69341 2
Entrypoint sizes 335029d 4849a4b Diff
mark_for_liquidation 17093 33792 16699
touch 56590 56586 -4
Test coverage 335029d 4849a4b Diff
checker.ml 90.18 89.82 -0.36000000000001364
TOTAL 93.93 93.85 -0.0800000000000125

@dorranh
Copy link
Contributor

dorranh commented Oct 1, 2021

Gas costs 335029d 4849a4b Diff
touch 537452 537403 -49
buy_kit 69339 69341 2
Entrypoint sizes 335029d 4849a4b Diff
mark_for_liquidation 17093 33792 16699
touch 56590 56586 -4
Test coverage 335029d 4849a4b Diff
checker.ml 90.18 89.82 -0.36000000000001364
TOTAL 93.93 93.85 -0.0800000000000125

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.

Touch a completed slice when marking a burrow for liquidation
2 participants