Skip to content

Conversation

0xdewy
Copy link
Contributor

@0xdewy 0xdewy commented Jan 5, 2023

Context

Closes #387

Changes proposed in this pull request

This PR adds support for active cover tracking. Everytime cover is bought, the amount to expire is stored in the bucket after the cover expires. Everytime burnStake() and buyCover are called, all buckets before and up to currentBucketId are expired, reducing activeCoverAmountInAsset.
When a cover is edited, the previous segment is instantly expired, and a new one is created at the bucket after the end of the new cover segment...
When a claim is made burnStake() is called, and this could be referencing an already expired segment or one in the future. If it is expired, the burn amount is already included in the expired amount, so it only uses expiredAmount. If the segment hasn't expired, active cover is reduced by (expiredAmount + burnAmount).

Test plan

Tests are added at: test/unit/Cover/totalActiveCoverInAsset.js

Checklist

  • Rebased the base branch
  • Attached corresponding Github issue
  • Prefixed the name with the type of change (i.e. feat, chore, test)
  • Performed a self-review of my own code
  • Followed the style guidelines of this project
  • Made corresponding changes to the documentation
  • Didn't generate new warnings
  • Didn't generate failures on existing tests
  • Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

  • 🍰 = Nice to have but not essential.
  • 💡 = Suggestion or a comment based on personal opinion
  • 🔨 = I believe this should be changed.
  • 🤔 = I don’t understand something, do you mind giving me more context?
  • 🚀 = Feedback

@0xdewy 0xdewy force-pushed the feature/active-cover-amount branch from 55c03c5 to aa3ceb8 Compare January 5, 2023 23:22
@roxdanila roxdanila force-pushed the feature/active-cover-amount branch from aa3ceb8 to 77f1329 Compare January 6, 2023 14:09
Copy link
Contributor

@shark0der shark0der left a comment

Choose a reason for hiding this comment

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

Direction definitely looks good! Left a couple of comments with some changes that are currently needed!

@roxdanila roxdanila force-pushed the feature/active-cover-amount branch from b479452 to 4af4aab Compare January 8, 2023 15:36
@0xdewy 0xdewy requested a review from shark0der January 10, 2023 15:18
@0xdewy 0xdewy marked this pull request as ready for review January 10, 2023 15:19
@0xdewy 0xdewy changed the title Implement bucket system for active cover expiry Feature: Implement bucket system for active cover expiry Jan 10, 2023
@shark0der shark0der force-pushed the feature/active-cover-amount branch 2 times, most recently from f54cd48 to f506f1d Compare January 11, 2023 14:46
@roxdanila
Copy link
Contributor

Main point from me is that there are a bunch of skipped unit tests related to active cover amount tracking. Would be good to get them fixed and re-enabled with this PR as well.

Screen Shot 2023-01-11 at 6 36 09 PM

Screen Shot 2023-01-11 at 6 36 32 PM

Also these ones are likely not relevant anymore:
Screen Shot 2023-01-11 at 6 36 18 PM

Copy link
Contributor

@shark0der shark0der left a comment

Choose a reason for hiding this comment

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

As per the discussion:

  • Do not delete the buckets and make the function view
  • Fix the accounting when burning (too much is subtracted)

@0xdewy 0xdewy force-pushed the feature/active-cover-amount branch 2 times, most recently from c6206dd to fe51259 Compare January 11, 2023 21:11
@0xdewy 0xdewy requested a review from shark0der January 13, 2023 00:14
@roxdanila roxdanila force-pushed the feature/active-cover-amount branch from 790e294 to c244d87 Compare January 16, 2023 10:10
@0xdewy 0xdewy force-pushed the feature/active-cover-amount branch 2 times, most recently from d592875 to 729c128 Compare January 17, 2023 18:48
@shark0der shark0der force-pushed the feature/active-cover-amount branch from 729c128 to d2316df Compare January 18, 2023 15:02
@shark0der
Copy link
Contributor

Blocked by #624

@0xdewy 0xdewy force-pushed the feature/active-cover-amount branch from d2316df to 47cec2c Compare January 20, 2023 23:01
@roxdanila roxdanila force-pushed the feature/active-cover-amount branch from 47cec2c to cda8f8b Compare January 26, 2023 09:15
@shark0der shark0der force-pushed the feature/active-cover-amount branch from cda8f8b to a1f8ae8 Compare January 26, 2023 10:36
@shark0der shark0der force-pushed the feature/active-cover-amount branch from a1f8ae8 to 9504a28 Compare January 26, 2023 10:56
@shark0der shark0der merged commit 9057dba into nexus-v2 Jan 26, 2023
@shark0der shark0der deleted the feature/active-cover-amount branch January 26, 2023 12:31
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.

Implement a buckets system for active cover amount tracking instead of per-cover
3 participants