Skip to content

Test: Add stakingPool processExpiration unit tests #538

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 5, 2023
Merged

Test: Add stakingPool processExpiration unit tests #538

merged 8 commits into from
Jan 5, 2023

Conversation

patitonar
Copy link
Contributor

Context

Closes #524

Changes proposed in this pull request

Adds new unit tests for processExpiration. Followed the list of test cases from the issue

Test plan

New tests were added in test/unit/StakingPool/processExpirations.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

@roxdanila roxdanila linked an issue Dec 5, 2022 that may be closed by this pull request
StakingPool unit tests: processExpirations #524
Closed
6 tasks
shark0der
shark0der requested changes Dec 7, 2022
View reviewed changes
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.

The PR missed a bug caught by @kyledewy in his pricing tests. The bucket expirations has a tranche id misalignment that led to an underflow when attempting to expire the allocations from the wrong tranche.

The test that caught it is here:

smart-contracts/test/unit/StakingPool/calculatePrice.js

Line 161 in 3a81811

The bugfix is on #546.

Please include some tests for that edge case.

danoctavian
danoctavian reviewed Dec 7, 2022
View reviewed changes
danoctavian
danoctavian reviewed Dec 7, 2022
View reviewed changes
test/unit/StakingPool/processExpirations.js Outdated
const bucketStartTime = firstActiveBucketId.mul(BUCKET_DURATION);
const elapsed = bucketStartTime.sub(lastAccNxmUpdateBefore);

expect(expiredBucketRewards).to.gt(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have an exact value here for expiredBucketRewards ?

strengthen the assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here e587527

danoctavian
danoctavian reviewed Dec 7, 2022
View reviewed changes
@danoctavian danoctavian force-pushed the test/staking-pool-process-expirations-unit-tests branch 2 times, most recently from e587527 to dcbe77c Compare December 8, 2022 08:04
@patitonar patitonar force-pushed the test/staking-pool-process-expirations-unit-tests branch from dcbe77c to 3e288c7 Compare December 19, 2022 15:14
@patitonar
Copy link
Contributor Author

The PR missed a bug caught by @kyledewy in his pricing tests. The bucket expirations has a tranche id misalignment that led to an underflow when attempting to expire the allocations from the wrong tranche.
The test that caught it is here:
smart-contracts/test/unit/StakingPool/calculatePrice.js
Line 161 in 3a81811
The bugfix is on #546.
Please include some tests for that edge case.

@shark0der It seems that the mentioned bug is related to the requestAllocation() method rather than the processExpiration() method. I agree that we should add tests for that edge case, but I think it would be better to include it as part of #492

danoctavian
danoctavian approved these changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

LGTM

This looks fine for me now.

Benefits from a second look from @shark0der as well.

@roxdanila roxdanila force-pushed the test/staking-pool-process-expirations-unit-tests branch from 328d5a5 to 94749cb Compare December 23, 2022 00:07
shark0der
shark0der reviewed Jan 3, 2023
View reviewed changes
shark0der
shark0der reviewed Jan 3, 2023
View reviewed changes
shark0der
shark0der reviewed Jan 3, 2023
View reviewed changes
shark0der
shark0der reviewed Jan 3, 2023
View reviewed changes
shark0der
shark0der requested changes Jan 3, 2023
View reviewed changes
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.

Looks good overall! Minor changes requested.

@shark0der
Copy link
Contributor

After requesting changes I've added this comment.

@roxdanila roxdanila force-pushed the test/staking-pool-process-expirations-unit-tests branch from 6047d7b to a7dafea Compare January 5, 2023 10:27
@shark0der shark0der force-pushed the test/staking-pool-process-expirations-unit-tests branch from a7dafea to f942c63 Compare January 5, 2023 11:08
@roxdanila roxdanila merged commit 5d8dd9f into nexus-v2 Jan 5, 2023
@roxdanila roxdanila deleted the test/staking-pool-process-expirations-unit-tests branch January 5, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shark0der shark0der shark0der approved these changes

@roxdanila roxdanila Awaiting requested review from roxdanila

+1 more reviewer

@danoctavian danoctavian danoctavian approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StakingPool unit tests: processExpirations
4 participants
@patitonar @shark0der @danoctavian @roxdanila