Optimize & cleanup #427
Optimize & cleanup #427
Conversation
|
Looks good to me! |
| @@ -306,8 +306,8 @@ benchmarks! { | |||
| let collator: T::AccountId = account("collator", 0, SEED); | |||
| let account_minimum = T::Currency::minimum_balance().saturating_mul(ED_MULTIPLIER.into()); | |||
|
|
|||
| T::Currency::deposit_creating(&delegator, (10_000_000_000_000_000 as u128).saturated_into()); | |||
| T::Currency::deposit_creating(&collator, (100_000_000_000_000_000 as u128).saturated_into()); | |||
| T::Currency::deposit_creating(&delegator, (10_000_000_000_000_000_u128).saturated_into()); | |||
There was a problem hiding this comment.
since you're here, we can take this change to move these value into const
There was a problem hiding this comment.
Do you mean, We should declare 10_000_000_000_000_000_u128 as const?
There was a problem hiding this comment.
Yes, it's more for code readable. such as on line 307, the code looks very nice.
T::Currency::minimum_balance().saturating_mul(ED_MULTIPLIER.into())
There was a problem hiding this comment.
The value is used only once. Declaring it as a const would just increase the overhead in my opinion.
pallets/automation-time/src/lib.rs
Outdated
| }, | ||
| _ => (), | ||
| }; | ||
| if let Action::XCMP { execution_fee, instruction_sequence, .. } = action.clone() { |
There was a problem hiding this comment.
because we're moving into if, can you see if we can switch to early return instead to avoid one more nested for the happy path.
There was a problem hiding this comment.
Looks like it's not possible, at least without functional changes I believe.
| @@ -249,8 +249,6 @@ impl<AccountId: Clone, Balance> Task<AccountId, Balance> { | |||
| instruction_sequence: InstructionSequence, | |||
| abort_errors: Vec<Vec<u8>>, | |||
| ) -> Result<Self, DispatchError> { | |||
| let destination = | |||
| MultiLocation::try_from(destination).map_err(|_| Error::<T>::BadVersion)?; | |||
There was a problem hiding this comment.
I think this is necessary? cc @imstar15 for extra 👀
There was a problem hiding this comment.
@v9n, Not in this case I believe. But yeah, it was necessary when destination's type was VersionedMultiLocation. For eg - https://github.com/OAK-Foundation/OAK-blockchain/blob/cf4a6eadc6a12fa02153434f71d34bf3e6bf9ac0/pallets/automation-time/src/lib.rs#L387-L388
|
@siddharthteli12 we're working on some other stuff and prefer to wait for this a bit. will get to your PR eventually. thanks for your contribution |
|
Hi @siddharthteli12 sorry for delay. I'm doing a final check now then will let you know. Can you check the new conflict? I will review asap and get this PR merged after that |
|
@v9n , Resolved the conflict. |
|
Thank you both. The PR is sync’ed with main so I’m merging it. |
One more pr in reference to #421