FeeTracker deduplications #8477
Conversation
serban300
added
T6-XCM
| // if we can't decrease the delivery fee factor anymore, we don't change anything | ||
| if bridge.delivery_fee_factor == MINIMAL_DELIVERY_FEE_FACTOR { | ||
| if !Self::do_decrease_fee_factor(&mut bridge.delivery_fee_factor) { |
There was a problem hiding this comment.
Well, actually, I didn't want to touch pallet-xcm-bridge-hub-router, because we will deprecate and remove it as soon as we get merged pallet-xcm-bridge-router here #6231
There was a problem hiding this comment.
There was a problem hiding this comment.
I modified
pallet-xcm-bridge-hub-routeras well because this PR is small and I would like to merge it before #6231 . After that, these changes will need to be merged in #6231 . I can do that if you want. It might be a bit of throwaway work, but it's almost insignificant.
Yes, but we will loose possibility to backport wherever we need to, because this is major change, right?
There was a problem hiding this comment.
Hmm yes, I think it's major. But being a small change, I think we can merge this one before the 2506 cutoff. Do you plan to backport #6231 also to older releases ?
There was a problem hiding this comment.
yes, we want to use it on fellowship runtimes depending on 2503 so we will "backport" to 2503
There was a problem hiding this comment.
Ok, I see. But even so, the changes to the FeeTracker are major. So we won't be able to use this in #6231 . We will need to create a separate PR (which will not be backported) on top of #6231 which will use the new FeeTracker in pallet-xcm-bridge-router. But we can still leave the changes to pallet-xcm-bridge-router in the current PR right ?
There was a problem hiding this comment.
Ok, I see. But even so, the changes to the
FeeTrackerare major. So we won't be able to use this in #6231 . We will need to create a separate PR (which will not be backported) on top of #6231 which will use the newFeeTrackerinpallet-xcm-bridge-router. But we can still leave the changes topallet-xcm-bridge-routerin the current PR right ?
But we can still leave the changes to
pallet-xcm-bridge-routerin the current PR right ?
Did you mean pallet-xcm-bridge-hub-router (the old one)? :)
If so, then yes, I think we can even go ahead and merge this PR, since it should be orthogonal to #6231. Once we merge/backport #6231 where needed, then we can also merge the FeeTracker for pallet-xcm-bridge-router (the new one) from #6231.
| impl<T: Config<I>, I: 'static> FeeTracker for Pallet<T, I> { | ||
| type Id = (); | ||
|
|
||
| fn get_min_fee_factor() -> FixedU128 { | ||
| MINIMAL_DELIVERY_FEE_FACTOR | ||
| } | ||
|
|
||
| fn get_fee_factor(_id: Self::Id) -> FixedU128 { | ||
| Self::bridge().delivery_fee_factor | ||
| } | ||
|
|
||
| fn set_fee_factor(_id: Self::Id, val: FixedU128) { | ||
| let mut bridge = Self::bridge(); | ||
| bridge.delivery_fee_factor = val; | ||
| Bridge::<T, I>::put(bridge); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
👍 nice, I like this new setup - id/min/get/set :)
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
| @@ -997,9 +997,9 @@ fn verify_fee_factor_increase_and_decrease() { | |||
| // We take 5 100 byte pages | |||
| XcmpQueue::take_outbound_messages(1); | |||
| } | |||
| assert!(DeliveryFeeFactor::<Test>::get(sibling_para_id) < FixedU128::from_float(1.72)); | |||
| assert!(DeliveryFeeFactor::<Test>::get(sibling_para_id) < FixedU128::from_float(1.80)); | |||
There was a problem hiding this comment.
@serban300 why is this changed? Did you change anything else besides refactoring FeeTracker?
| fn get_min_fee_factor() -> FixedU128 { | ||
| unimplemented!() | ||
| } |
There was a problem hiding this comment.
Should we be defensive and use something that will crash only on debug like debug_assert? Just in case this reaches prod and we get a panic.
Closes #8462