[AHM] Nomination pools #562
Conversation
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]>
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]>
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]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
| # Storage Maps | ||
|
|
||
| The `BondedPools` map needs translation for its commission logic. The Commission struct contains an | ||
| absolute relay timestamp |
There was a problem hiding this comment.
Is it not possible to rework this to continue using the RcBlockNumberProvider as a clock? Is it just because this hasn't been completed yet or is there a reason not to?
There was a problem hiding this comment.
I dont see an open MR to do it, so I assume that it will be done in the future. Generally yes, it can very well use the relay block number (and should). We just work off the state of the SDK that we have for now.
There was a problem hiding this comment.
Yea I'll get somebody on it. For these ones which haven't been completed yet but which are intended, I think you can write the migration and keep the number as is, but make a note and we can come back to it if it looks like the pallet won't be ready in time. Might save you a bit of time for these pallets where the mapping introduces some extra work
| @@ -30,8 +30,9 @@ We therefore do not migrate the announcements. | |||
| ## User Impact | |||
| - Announcements need to be re-created | |||
| - Proxies of type `Auction` are not migrated and need to be re-created on the Relay | |||
| - Existing proxies on Asset Hub will now have more permissions and will be able to access the new pallets as well. For example, the `NonTransfer` proxy will also be able to use nomination pools. This may affect security assumptions of previously created proxies. | |||
There was a problem hiding this comment.
may be we rename NoTransfer? and we do not include new pallets under it?
There was a problem hiding this comment.
I was thinking about something like NoTransferLegacy or so that is the same as on the Relay and then the normal NoTransfer (No or Non i dont mind).
Then we could translate the proxies in the exact same way and not increase their scope.
There was a problem hiding this comment.
with a good documentation I think it will be good enough.
| @@ -216,6 +222,18 @@ pub mod pallet { | |||
| /// How many preimage legacy status failed to integrate. | |||
| count_bad: u32, | |||
| }, | |||
| /// Received a batch of `RcNomPoolsMessage` that we are going to integrate. | |||
| NomPoolsMessagesBatchReceived { | |||
There was a problem hiding this comment.
we getting more and more of these identical events. we probably also need similar on rc-migrator side. what if we create some general even with a pallet name as a fixed size string? not sure how convenient that would be.
There was a problem hiding this comment.
Yea we could create a new enum that is called like PalletName or so and then use that inside. Just string is not enough because the front-end wont know the variants.
| let mut messages = Vec::new(); | ||
|
|
||
| loop { | ||
| if weight_counter.try_consume(Weight::from_all(1)).is_err() { |
There was a problem hiding this comment.
for the rc weight counter we can just count for db operations here + transport
| /// - AH now: 75 | ||
| /// - RC time point: 150 | ||
| /// - Result: 75 + (150 - 100) / 2 = 100 | ||
| pub fn rc_to_ah_timepoint(rc_timepoint: BlockNumberFor<T>) -> BlockNumberFor<T> { |
There was a problem hiding this comment.
for all real time sensitive timepoints I am gonna leave RC as a block provider and do not do the mapping. If it just for servicing, my plan is to keep the system block provider.
There was a problem hiding this comment.
The nom pools pallet is not migrated to RC number provider yet, otherwise i would also prefer that.
There was a problem hiding this comment.
it is in the list, so yes we did plan it
paritytech/polkadot-sdk#6297
There was a problem hiding this comment.
Would be better to have it. But turning off the translation here is a two line change so am not too concerned about it.
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
To be merged into the AHM working branch.
Pallet Nomination Pools
The nomination pools pallet has 15 storage items of which 14 can be migrated without any translation.
Storage Values
All nine storage values are migrated as is in a single message.
Storage Maps
The
BondedPoolsmap needs translation for its commission logic. The Commission struct contains an absolute relay timestamp throttle_from and the commission change min_delay.The translation for both happens upon arrival on the Asset Hub. Ideally, it would be done on the Relay chain side since we have more compute power there but it is not possible since the timestamp translation depends on the Relay number upon arrival on Asset Hub.
The timestamp is translated in rc_to_ah_timestamp and the
min_delayis directly passed toRcToAhDelay.The other five storage maps are migrated as it.
User Impact
Impact here is negligible and only for pool operators - not members:
Pool commission change rate (measured in blocks) could be decreased by one block.
Pool operators may be able to change the commission rate one block later than anticipated. This is due to the nature or translating blocks of two different blockchains which does not yield unambiguous results.
Does not require a CHANGELOG entry