moonbeam-foundation / crowdloan-rewards

Substrate pallet that enables parachains to issue rewards to crowdloan contributors in parachain-native tokens.
12 stars 16 forks source link

Completely remove cumulus (dev-)dependencies #44

Closed JoshOrndorff closed 2 years ago

JoshOrndorff commented 3 years ago

This PR is a followup to #43. It removes all dependencies (even dev-dependencies) on crates from cumulus.

Tests now use the parachain's own block number rather than the mocked relay number. This means that the InitVestingBlock was 1 rather than 2 as it was before. This caused most of the block indices to be off-by-one in tests that tested for specific vested amounts.

notlesh commented 3 years ago

This PR title looked alarming when it came across my inbox :laughing:

girazoki commented 3 years ago

Although tests work fine in this case, benchmarking is run against the moonbase runtime and this fetches the relay block height from cumulus-pallet-parachain-system. So even though the pallet will compile, the benchmarking will not work when we run it against our moonbase runtime (e.g., the claim function needs the relay chain height to advance to be able to claim). And the way to do that is to inject valid inherents (thus the reason why create_inherent_data was there before).

girazoki commented 3 years ago

So even if we do that, this associated type has only the BlockNumberProvider trait which allows only to get the current relay block number. So since we are not setting this number anywhere in the benchmarking code (nor the associated type provides a function to do so), when we benchmark against the moonbase runtime we will always read a 0 there and the benchmarking will not be properly done for functions like claim where we need that number to be moving up.

I see a couple of solutions:

1) Read this value on_initialize, and store it in our own storage. Our own storage is something we can hack in the benchmarking code. But of course this is much less efficient.

2) Add a trait bound like ProvideInherent in addition to BlockNumberProvider. In this case we could write the benchmarking code to add the inherents. The problem is we probably wont remove the dependency on cumulus but rather make it optional and add it for benchmarking only. The reason is that we still need to push valid inherents in the context of a moonbase runtime, and they need to have this https://github.com/paritytech/cumulus/blob/968c91e357e184ed117f3b39fd51fce23a6945c4/pallets/parachain-system/src/lib.rs#L566 specific format

4meta5 commented 3 years ago

Read this value on_initialize, and store it in our own storage. Our own storage is something we can hack in the benchmarking code. But of course this is much less efficient.

Sounds reasonable to me, how much less efficient is it?

JoshOrndorff commented 3 years ago

The key idea here is that a pallet may be used in multiple different runtimes, and the costs to use the pallet may differ between those runtimes.

I think rather than hacking at the pallet level to get accurate benchmarks for the one runtime that we care most about (at the expense of anyone else who uses this pallet) we should instead run the benchmarks against each runtime that actually uses the pallet.

@notlesh weren't you having similar ideas about pallets that Moonbeam uses from Substrate?

So my main point is that this problem is not specific to crowdloan rewards, so it should not have a crowdloan-rewards-specific solution.

girazoki commented 3 years ago

I like that idea, but in that case we need to write runtime-dependent benchmark code. The benchmarking code that exists here aims to be generic, something that we might want to forget about

girazoki commented 2 years ago

Superseded by https://github.com/PureStake/crowdloan-rewards/pull/61 due to merge conflicts