-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: add freeze & unfreeze extrinsics #1947
Conversation
}) | ||
} | ||
|
||
/// NOTE: Collecting redemptions does not check pre-conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pallets/investments/src/tests.rs
Outdated
TestExternalitiesBuilder::build().execute_with(|| { | ||
assert_noop!( | ||
Investments::collect_investments(RuntimeOrigin::signed(NOT_INVESTOR), INVESTMENT_0_0), | ||
DispatchError::Other("PreCondition mock fails on u64::MAX account on purpose") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - move this error to a const and use it in the mock and tests.
pallets/liquidity-pools/src/lib.rs
Outdated
false, | ||
)?; | ||
|
||
T::Permission::add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason this is not implemented in the same way as update_member
: the permission is set separately using the permsisions.add_permission
extrinsic, and then in the liquidity pools method it is just checked that the permission is already set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. I will refactor it to be in sync with the update_member
flow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1947 +/- ##
==========================================
+ Coverage 47.83% 48.31% +0.48%
==========================================
Files 183 184 +1
Lines 12904 12999 +95
==========================================
+ Hits 6172 6281 +109
+ Misses 6732 6718 -14 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks William for you fully complete and well organized PR. Super easy to review 🙌🏻.
Some minor comments below, not found any major
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks super good!
Just my concern regarding that is_frozen
increases the size of the type. So the code will try to deserialize more bytes than the existing ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the migration! It seemed not easy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LSGTM!
Description
Poolrole
variantFrozenTrancheInvestor
TrancheInvestor
permission with downside of having to migrate all existing storage entriespallet-liquidity-pools
pallet-liqudiity-pools
defensive weightsChecklist: