-
Notifications
You must be signed in to change notification settings - Fork 106
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
[AHM] Fast unstake and Bags list #563
Conversation
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
…y-ahm-fast-unstake
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
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.
Need to check if we have to re-generate these values.
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.
No proper review given from me for this file. But I will do it on request. May be we better consult with staking team on this, will take more time from me.
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.
No proper review given from me for this file. But I will do it on request. May be we better consult with staking team on this, will take more time from me.
type RuntimeEvent = RuntimeEvent; | ||
type Currency = Balances; | ||
type BatchSize = frame_support::traits::ConstU32<16>; | ||
type Deposit = frame_support::traits::ConstU128<{ UNITS }>; |
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.
might need to be updated, since should cost less on a parachain
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.
Updated to 1 / 100 UNIT.
pallets/rc-migrator/src/lib.rs
Outdated
MigrationDone, | ||
} | ||
|
||
pub type MigrationStageFor<T> = MigrationStage<<T as frame_system::Config>::AccountId>; | ||
pub type MigrationStageFor<T> = MigrationStage< |
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.
Do we have some non written convention when we use For
or Of
suffix? I wished to keep it consistent (=
I had a feeling that Gav used For
for storage items and Of
for types, but I am not sure.
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.
I was wondering about the same thing. Core issue is that there is AccountIdOf
and BlockNumberFor
, so two very central types in contradiction 😆 🤦♂️
I dont really mind when we use what, we can use Of
for types if you want.
break; | ||
} | ||
} | ||
if messages.len() > 10_000 { |
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.
temporary measure or not? can you explain please
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.
I was not sure here how exactly we benchmark things. Depending that, this would be a failsafe in case that our weights are wrong to not completely overload the system.
It is not great or precise, but better than some infinite loop.
messages: Vec<RcBagsListMessage<T>>, | ||
) -> Result<(), Error<T>> { | ||
let (mut good, mut bad) = (0, 0); | ||
log::info!("Integrating {} BagsListMessages", messages.len()); |
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.
log target?
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.
Not sure if it adds much. Do you use it often to filter?
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
…y-ahm-fast-unstake
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
To be merged into the AHM dev branch.
There should be no user impact here - both pallets have their storage migrated exactly as is and the used times are always in Epoch.