Feature: edit product grace period #409

0xdewy · 2022-10-06T11:41:22Z

Context

Resolves #287

Changes proposed in this pull request

This PR adds the ability to edit gracePeriodInDays. To do this gracePeriodInDays was added to the CoverSegments struct. Deallocation and claims should now use the grace period from the cover segment. Allocations should use the product level grace period.

Test plan

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

danoctavian · 2022-10-06T19:03:43Z

contracts/interfaces/ICover.sol

@@ -51,6 +51,7 @@ struct CoverSegment {
  uint96 amount;
  uint32 start;
  uint32 period;  // seconds
+  uint16 gracePeriodInDays;


Total current struct size:

96 + 32 + 32 + 16 + 16 + 8 + 24
224

Looks fine.

danoctavian · 2022-10-06T19:12:10Z

test/unit/IndividualClaims/getClaimsToDisplay.js

@@ -35,7 +35,7 @@ describe('getClaimsToDisplay', function () {
        coverOwner.address,
        0, // productId
        ASSET.ETH,
-        [[coverAmount, timestamp + 1, coverPeriod, 0, false, 0]],
+        [[coverAmount, timestamp + 1, coverPeriod, 7, 0, false, 0]],


consider using a constant test variable here for the test since it's the same throughout

danoctavian · 2022-10-06T19:12:47Z

test/unit/IndividualClaims/redeemClaimPayout.js

@@ -24,7 +24,7 @@ describe('redeemClaimPayout', function () {
      coverOwner.address,
      0, // productId
      ASSET.ETH,
-      [[coverAmount, timestamp + 1, coverPeriod, 0, false, 0]],
+      [[coverAmount, timestamp + 1, coverPeriod, 7, 0, false, 0]],


same here use a named constant for the value 7 since it's the same in every test case

Yea these are a bit tedious in the case that CoverSegment structure changes. I thought about storing this struct as a fixture somewhere and have each tests just modify which field it is adjusting?

yeah agree with fixture

danoctavian · 2022-10-06T19:13:08Z

test/unit/YieldTokenIncidents/redeemPayout.js

@@ -18,7 +18,7 @@ describe('redeemPayout', function () {
        coverOwner.address,
        2, // productId
        ASSET.ETH,
-        [[parseEther('10000'), timestamp + 1, segmentPeriod, 0, false, 0]],
+        [[parseEther('10000'), timestamp + 1, segmentPeriod, 7, 0, false, 0]],


use a constant

danoctavian · 2022-10-06T19:13:29Z

contracts/modules/cover/Cover.sol

@@ -680,7 +681,7 @@ contract Cover is ICover, MasterAwareV2, IStakingPoolBeacon, ReentrancyGuard {
    Product memory product,
    uint32 _coverAssetsFallback,
    uint productTypesCount
-  ) internal {
+  ) internal pure {


danoctavian · 2022-10-06T19:17:46Z

Add an assertion here for gracePeriod

smart-contracts/test/unit/Cover/helpers.js

Line 43 in 8b86fc1

async function assertCoverFields(

danoctavian

The changes mentioned above.

Otherwise looks good

0xdewy · 2022-10-07T15:00:01Z

contracts/modules/cover/Cover.sol

@@ -400,7 +401,7 @@ contract Cover is ICover, MasterAwareV2, IStakingPoolBeacon, ReentrancyGuard {
          cover.productId,
          allocation.coverAmountInNXM,
          lastCoverSegment.period,
-          _productTypes[product.productType].gracePeriodInDays * 1 days,
+          uint(lastCoverSegment.gracePeriodInDays) * 1 days,


I had to cast gracePeriodInDays as a uint here as it was overflowing.

roxdanila · 2022-10-10T10:38:54Z

test/unit/Cover/performStakeBurn.js


-describe('performStakeBurn', function () {
+describe.only('performStakeBurn', function () {


forgot an .only here

shark0der · 2022-10-10T13:24:41Z

Missed a refactor from productType.gracePeriodInDays to segment.gracePeriodInDays in CoverViewer.sol:93

danoctavian

LGTM

0xdewy linked an issue Oct 6, 2022 that may be closed by this pull request

Cover / Staking: Allow editing grace periods #287

Closed

0xdewy marked this pull request as ready for review October 6, 2022 12:17

danoctavian reviewed Oct 6, 2022

View reviewed changes

danoctavian suggested changes Oct 6, 2022

View reviewed changes

0xdewy commented Oct 7, 2022

View reviewed changes

0xdewy requested a review from danoctavian October 10, 2022 09:29

roxdanila reviewed Oct 10, 2022

View reviewed changes

shark0der changed the title ~~Feature/ editproduct grace period~~ Feature: edit product grace period Oct 10, 2022

shark0der force-pushed the feature/edit-product-grace-period branch from 8fee40e to 891708c Compare October 10, 2022 11:00

0xdewy requested review from roxdanila and shark0der October 11, 2022 07:23

shark0der approved these changes Oct 11, 2022

View reviewed changes

danoctavian approved these changes Oct 12, 2022

View reviewed changes

0xdewy added 10 commits October 13, 2022 12:20

Add gracePeriodInDays to CoverSegment and use during deallocation/claims

15a0372

Fix broken test

b964705

Remove unused variables + change validateProducts() from view to pure

490c3a9

Fix gracePeriodInDays overflowing when casting to seconds

f6d9d4c

Add unit tests for gracePeriod + Refactor redeemPayout.js unit tests

2a7d2cc

Refactor cover segment creation in tests

916c7a9

Update submitClaims tests to reuse CoverSegment struct

5009414

Add gracePeriodInDays to assertCoverFields test helper

6abae65

return gracePeriodInDays for each cover segment in CoverViewer

a621d8c

Return latest segments gracePeriodInDays in CoverViewer

924e31f

shark0der force-pushed the feature/edit-product-grace-period branch from cf4bf9a to 924e31f Compare October 13, 2022 09:20

shark0der merged commit f275722 into nexus-v2 Oct 13, 2022

shark0der deleted the feature/edit-product-grace-period branch October 13, 2022 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: edit product grace period #409

Feature: edit product grace period #409

0xdewy commented Oct 6, 2022 •

edited

Loading

danoctavian Oct 6, 2022

danoctavian Oct 6, 2022

danoctavian Oct 6, 2022

0xdewy Oct 7, 2022

danoctavian Oct 7, 2022

danoctavian Oct 6, 2022

danoctavian Oct 6, 2022

danoctavian commented Oct 6, 2022

danoctavian left a comment

0xdewy Oct 7, 2022

roxdanila Oct 10, 2022

shark0der commented Oct 10, 2022

danoctavian left a comment


		describe('performStakeBurn', function () {
		describe.only('performStakeBurn', function () {

Feature: edit product grace period #409

Feature: edit product grace period #409

Conversation

0xdewy commented Oct 6, 2022 • edited Loading

Context

Changes proposed in this pull request

Test plan

Checklist

Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian commented Oct 6, 2022

danoctavian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shark0der commented Oct 10, 2022

danoctavian left a comment

Choose a reason for hiding this comment

0xdewy commented Oct 6, 2022 •

edited

Loading