Skip to content
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

[tracking] adopt BlockNumberProvider for the pallets migrating to AH #6297

Open
muharem opened this issue Oct 30, 2024 · 12 comments
Open

[tracking] adopt BlockNumberProvider for the pallets migrating to AH #6297

muharem opened this issue Oct 30, 2024 · 12 comments

Comments

@muharem
Copy link
Contributor

muharem commented Oct 30, 2024

Some of the pallets migrating from the Relay Chain to the Asset Hub require a more deterministic clock than what a parachain can provide. To address this, the decision was made to adapt these pallets to use a configurable block number provider, which can be set to the Relay Chain’s block number provider for greater determinism. (For more details, see the discussions at #3268). This approach will also simplify the migration of pallets' state from the Relay Chain to the Asset Hub since, in most cases, block numbers won’t require mapping.

Task:

Adapt BlockNumberProvider for the pallets migrating to the Asset Hub:

  • Introduce a BlockNumberProvider configuration parameter and use its current_block_number function as the time reference instead of System::<T>::block_number().
  • Ensure pallet hooks like on_initialize(n) are compatible with the BlockNumberProvider and can handle cases when the hook isn’t called on every tick (e.g., adapt conditions that rely on exact matches like compilation_time == BlockNumberProvider::current_block_number()).
  • Review all instances of generic block number types, ensuring they reference the correct type (e.g., BlockNumberProvider::BlockNumber when migrated to BlockNumberProvider).
  • Update documentation for calls with block number parameters, configuration settings like BountyUpdatePeriod, and storage fields. Documentation should clarify for users that block numbers refer to those from Config::BlockNumberProvider, which may come from either the local parachain or the Relay Chain.

Reference PR: #3970

Pallet list:

(when you start to work on a pallet, pul your name next to it or leave a comment)

@aurexav
Copy link
Contributor

aurexav commented Oct 31, 2024

Love to take on some tasks once the #3970 is merged as a standard to follow.

@muharem
Copy link
Contributor Author

muharem commented Nov 1, 2024

@AurevoirXavier it is merged now

@Doordashcon
Copy link
Contributor

is anyone working pallet-nis atm?

@gui1117
Copy link
Contributor

gui1117 commented Dec 9, 2024

pallet nis make use of total_issuance. So migrating to AH also mean that DOT total issuance is recorded on AH. Also I don't how much we teleport asset between trusted chains, but teleporting asset does change the accuracy of the value no?

@Doordashcon
Copy link
Contributor

is anyone working on pallet-society atm?

github-merge-queue bot pushed a commit that referenced this issue Dec 22, 2024
Make pallet-recovery supports `BlockNumberProvider`.

Part of #6297.

---

Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: GitHub Action <[email protected]>
@seadanda
Copy link
Contributor

seadanda commented Jan 3, 2025

pallet nis make use of total_issuance. So migrating to AH also mean that DOT total issuance is recorded on AH. Also I don't how much we teleport asset between trusted chains, but teleporting asset does change the accuracy of the value no?

Teleported assets are tracked in the CheckingAccount, which is currently on the Relay tracking DOT which has been teleported to other chains, but will be on AH post-migration. Since pallet-nis is also migrated it shouldn't be a problem.

is anyone working on pallet-society atm?

I don't think anybody is, feel free to take it on if you're interested, thanks!

Not on the list @muharem but pallet-core-fellowship #6978 and pallet-salary #7000

This issue is focusing on the pallets being migrated from the Relay Chain to Asset Hub in the coming migration, the two you've mentioned are on Collectives. They should also be migrated, but at a lower priority :)

dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this issue Jan 4, 2025
Make pallet-recovery supports `BlockNumberProvider`.

Part of paritytech#6297.

---

Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: GitHub Action <[email protected]>
@seadanda seadanda moved this from Backlog to In progress in AHM Jan 8, 2025
@PolkadotDom
Copy link
Contributor

I'll take conviction voting 👍

@dharjeezy
Copy link
Contributor

dharjeezy commented Jan 14, 2025

I'll take conviction voting 👍

@PolkadotDom I already worked on it initially #6621

@dharjeezy
Copy link
Contributor

FYI, I already worked on these pallets: pallet conviction voting,
pallet society, and pallet nomination pool

@muharem
Copy link
Contributor Author

muharem commented Jan 29, 2025

@dharjeezy @Doordashcon would you like to work on pallet_election_provider_multi_phase pallet?

I think I went through all open PRs, skipped only the ones that have unsolved requests for change. You can expect my reviews as soon as you solve them. It would be very helpful if we can get this issue closed before the release (mid Feb).

@dharjeezy
Copy link
Contributor

dharjeezy commented Jan 29, 2025

@dharjeezy @Doordashcon would you like to work on pallet_election_provider_multi_phase pallet?

I think I went through all open PRs, skipped only the ones that have unsolved requests for change. You can expect my reviews as soon as you solve them. It would be very helpful if we can get this issue closed before the release (mid Feb).

I will take pallet_election_provider_multi_phase and work on it @muharem

@muharem
Copy link
Contributor Author

muharem commented Feb 3, 2025

@dharjeezy @Doordashcon @seemantaggarwal please make sure you provide a proper documentation.

Example:

/// Provider for the block number.
///
/// Normally this is the `frame_system` pallet.
type BlockNumberProvider: BlockNumberProvider;

This explanation lacks enough detail for someone encountering this feature for the first time. Same comment for PRdocs

The term “Normally” is also unclear - what exactly qualifies as a “normal” setup? Instead, it should explicitly state that to maintain the same behavior as before this feature was introduced, the setting should be configured as System. However, even with that clarification, users still need more context to confidently set it to System without second-guessing.

Additionally, as I mentioned in the issue:

Update documentation for calls with block number parameters, configuration settings like BountyUpdatePeriod, and storage fields. Documentation should clarify for users that block numbers refer to those from Config::BlockNumberProvider, which may come from either the local parachain or the Relay Chain.

Right now, clients cannot assume the block number is always local - it may come from another source. This crucial detail is missing, meaning client developers will only discover it after making a mistake, often after spending considerable time troubleshooting.

I won’t block these PRs since we need them for the next release, but improving the documentation is not a major effort - a single PR could address this properly for all pallets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

No branches or pull requests

7 participants