[WIP] Combine RemoveParaIdsWithNoCredit and CollatorAssignmentHook
#692
Conversation
ParthDesai
force-pushed
the
combine-remove-paraids-and-collator-assignment-hook
branch
from
611e197
to
ac88d40
Compare
ParthDesai
changed the title
RemoveParaIdsWithNoCredit and CollatorAssignmentHookRemoveParaIdsWithNoCredit and CollatorAssignmentHook
ParthDesai
force-pushed
the
combine-remove-paraids-and-collator-assignment-hook
branch
from
ac88d40
to
b01541a
Compare
ParthDesai
changed the title
RemoveParaIdsWithNoCredit and CollatorAssignmentHookRemoveParaIdsWithNoCredit and CollatorAssignmentHook
ParthDesai
force-pushed
the
combine-remove-paraids-and-collator-assignment-hook
branch
from
b01541a
to
b655d6b
Compare
Coverage Report@@ Coverage Diff @@
## master combine-remove-paraids-and-collator-assignment-hook +/- ##
=======================================================================================
+ Coverage 66.82% 66.85% +0.03%
Files 297 297
+ Lines 51894 52072 +178
=======================================================================================
+ Hits 34677 34812 +135
+ Misses 17217 17260 +43
|
ParthDesai
force-pushed
the
combine-remove-paraids-and-collator-assignment-hook
branch
2 times, most recently
from
916be99
to
d0ad35f
Compare
| fn remove_para_ids_with_no_credits( | ||
| para_ids: &mut Vec<ParaId>, | ||
| impl RemoveParaIdsWithNoCreditsImpl { | ||
| fn charge_para_ids_internal( |
There was a problem hiding this comment.
this is becoming hard to read, I would probably split it into different subfunctions that are public in the services payment pallet so that all this code is not in the runtime.
There was a problem hiding this comment.
also the naming of the function is not ideal, I would call it something like charge_services_for_para_id
|
|
||
| let remaining_to_pay = remaining_block_credits_to_pay.saturating_add(remaining_session_credits_to_pay).saturating_add(max_tip); | ||
| impl<AC> RemoveParaIdsWithNoCredits<BalanceOf<Runtime>, AC> for RemoveParaIdsWithNoCreditsImpl { | ||
| fn pre_assignment_remove_para_ids_with_no_credits( |
There was a problem hiding this comment.
Here I would rename pre_assignment_remove_para_ids_with_no_credits to simply pre_assignment_hook or pre_assignment_filtering
| // This should take into account whether we tank goes below ED | ||
| // The true refers to keepAlive | ||
| Balances::can_withdraw(&pallet_services_payment::Pallet::<Runtime>::parachain_tank(*para_id), remaining_to_pay).into_result(true).is_ok() | ||
| fn post_assignment_remove_para_ids_with_no_credits( |
There was a problem hiding this comment.
same thing here, the naming is very specific to what this is doing. I would call it something like post_assignmnet_hook
| if collators.is_empty() { | ||
| return true; | ||
| } | ||
| with_storage_layer(|| { |
There was a problem hiding this comment.
what is with_storage_layer¿?
There was a problem hiding this comment.
So, with_storage_layer is similar to with_transaction but it automatically commits the storage changes into parent overlay if the closure returns Ok and do the rollback otherwise.
Parity added this function since it is more convenient to use. (i.e you don't have to manually return Ok or Err for commit and rollback respectively.
There was a problem hiding this comment.
Are there any performance concerns when running that function inside a loop? I guess not?
There was a problem hiding this comment.
Fairly low cost. See here: paritytech/substrate#10809 (comment)
ParthDesai
force-pushed
the
combine-remove-paraids-and-collator-assignment-hook
branch
from
d0ad35f
to
7ccaa48
Compare
| // This should take into account whether we tank goes below ED | ||
| // The true refers to keepAlive | ||
| Balances::can_withdraw(&pallet_services_payment::Pallet::<Runtime>::parachain_tank(*para_id), remaining_to_pay).into_result(true).is_ok() | ||
| fn post_assignment_remove_para_ids_with_no_credits( |
There was a problem hiding this comment.
So can we remove this method? pre_assignment already uses with_transaction to remove para_ids that would have failed if Self::charge_para_ids_internal returns Err, so if I'm not wrong, calling post_assignment will never remove any additional para id?
There was a problem hiding this comment.
In perfect world yes. But, since we are changing state in between calling pre and post we need to run it again to eliminate even a tiniest possibility that some para_id escapes the restrictions.
Description
Combine
RemoveParaIdsWithNoCreditandCollatorAssignmentHookin one trait with two methods.Both method executes same logic. Only difference being
pre_assignment_*method always rollback whilepost_assignment_*only rollback in case it errors out.Note
Naming and tests are still remaining. Just want to get some early feedback on overall design.