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.

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

Fix: Revert if trying to initialize new pool with forbidden or non-existing products #805

Merged
merged 19 commits into from
May 4, 2023
Merged

Fix: Revert if trying to initialize new pool with forbidden or non-existing products #805

merged 19 commits into from
May 4, 2023

Conversation

0xdewy
Copy link
Contributor

@0xdewy 0xdewy commented Mar 10, 2023 •
edited
Loading

Context

Fixes #803 - When adding initial staking products to a newly created staking pool, we should check for each product that it exists & is allowed for the pool

Changes proposed in this pull request

This PR checks that each product is allowed to be initialized for newly created staking pools in Cover.sol. The check is made for migrations and for new pool creations.

Changes:

  • reverts with PoolNotAllowedForThisProduct(uint productId) if isPoolAllowed() returns false
    • (this should happen when the product doesn't exist, product is deprecated or the poolId isn't found in the allow list)
  • changed isPoolAllowed to also return false when the product doesn't exist

Test plan

I added a unit and integration test in createStakingPool.js and a fork test at test/fork/staked-product-authorization.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 marked this pull request as ready for review March 10, 2023 23:58
@0xdewy 0xdewy linked an issue Mar 13, 2023 that may be closed by this pull request
StakingProducts: When adding initial staking products to a newly created staking pool, we should check for each product that it exists & is allowed for the pool #803
Closed
Base automatically changed from nexus-v2 to master March 16, 2023 12:37
@roxdanila roxdanila changed the base branch from master to release-candidate March 17, 2023 13:27
@0xdewy 0xdewy force-pushed the fix/create-staking-pool-product-check branch from b1712b5 to 14fe339 Compare March 18, 2023 02:08
@roxdanila roxdanila marked this pull request as draft March 29, 2023 10:06
@roxdanila
Copy link
Contributor

@kyledewy converted back to draft until you add context, proposed changes, etc.

@0xdewy 0xdewy force-pushed the fix/create-staking-pool-product-check branch 2 times, most recently from 878117d to cac6070 Compare April 3, 2023 16:26
@0xdewy 0xdewy marked this pull request as ready for review April 3, 2023 21:25
@0xdewy 0xdewy self-assigned this Apr 3, 2023
@0xdewy 0xdewy marked this pull request as draft April 3, 2023 22:39
@0xdewy 0xdewy marked this pull request as ready for review April 4, 2023 21:54
@0xdewy 0xdewy changed the title Fix: Add check that new pool is allowed to have the product Fix: Revert if trying to initialize new pool with forbidden or non-existing products Apr 4, 2023
@0xdewy 0xdewy requested a review from danoctavian April 5, 2023 23:11
danoctavian
danoctavian previously requested changes Apr 7, 2023
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.

Happy to argue over the obervations made but to me some changes are necessary

@0xdewy 0xdewy force-pushed the fix/create-staking-pool-product-check branch from f1d2593 to 06b1c4d Compare April 7, 2023 19:29
@0xdewy 0xdewy requested a review from danoctavian April 7, 2023 19:30
@0xdewy 0xdewy force-pushed the fix/create-staking-pool-product-check branch from 06b1c4d to 4026567 Compare April 11, 2023 21:09
@0xdewy 0xdewy requested a review from danoctavian April 12, 2023 00:30
@0xdewy
Copy link
Contributor Author

0xdewy commented Apr 21, 2023

I made the changes suggested by @shark0der , along with the optimization I suggested above. The only thing that should be missing is the deprecated check in setProducts yet. Want to write some tests for it as well.

@0xdewy 0xdewy marked this pull request as draft May 2, 2023 00:49
@0xdewy 0xdewy requested a review from danoctavian May 3, 2023 00:31
@0xdewy 0xdewy marked this pull request as ready for review May 3, 2023 00:31
@shark0der shark0der force-pushed the fix/create-staking-pool-product-check branch 2 times, most recently from f69fcdd to 3ad8a67 Compare May 4, 2023 14:05
0xdewy and others added 18 commits May 4, 2023 17:09
@0xdewy @shark0der
Add check that new pool is allowed to have the product
9d05434
@0xdewy @shark0der
Small test improvement
d52f283
@0xdewy @shark0der
Add product existence check to isPoolAllowed
58b8020
@0xdewy @shark0der
Add fork tests
64aed23
@0xdewy @shark0der
Cleanup tests and clarify helper function names
588b385
@0xdewy @shark0der
Add isDeprecated check
1d3c18c
@0xdewy @shark0der
Clean up tests and comments
de5efba
@0xdewy @shark0der
Add another product during setup
91ce61c
@0xdewy @shark0der
Make internal functions for isDeprecated and if product exists. Reorg…
5194d7c 
…anize logic to be more clear
@0xdewy @shark0der
Deploy new pool after validation
b6d005c
@0xdewy @shark0der
Use Bignumber.from instead of One.mul()
f464325
@0xdewy @shark0der
Inline product existence/deprecation checks. Clarify comments in isPo…
7fd683e 
…olAllowed
@shark0der
Remove pooled staking ability to create pools. Add onlyMember modifie…
88d814c 
…r. Add requirePoolIsAllowed function.
@shark0der
Add integration tests. Move custom error so it reverts before invalid…
2e87b36 
… array access
@0xdewy @shark0der
Change integration test order to avoid breaking other tests
5a01c1a
@shark0der
Cleanup comments
d928ea2
@shark0der
Remove exclusive mocha tests
62c1c74
@shark0der
Remove obsolete test/fork/external.js file
82c66b7
@shark0der shark0der force-pushed the fix/create-staking-pool-product-check branch from 6b3bf3e to 82c66b7 Compare May 4, 2023 14:11
@shark0der shark0der merged commit 81b8a15 into release-candidate May 4, 2023
@shark0der shark0der deleted the fix/create-staking-pool-product-check branch May 4, 2023 15:18
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

@danoctavian danoctavian Awaiting requested review from danoctavian

Assignees

@shark0der shark0der

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@0xdewy @roxdanila @shark0der @danoctavian