Feature/plmc 242 implement correct duration of vesting

[JuaniRios]

No description provided.

[linear]

PLMC-242 Implement correct duration of vesting

[lrazovic]

Not a big fan of having DAYS in the pallet, but I can't think of any alternative/better solutions at the moment.

[JuaniRios]

Our chain would produce blocks and the rate specified in our runtime. If we want to speed up or slow down block production I think we will necessarily need a runtime upgrade, therefore the DAYS will be automatically updated when using the latest version of that library.

But let's check with @joepetrowski

[joepetrowski]

But you are pulling DAYS from Cumulus's parachains-common. Nothing in Substrate/Polkadot depend on anything in Cumulus. So it's weird to have a pallet that depends on Cumulus. I think it makes more sense to have:

pub trait Config: frame_system::Config {
	/// The reference duration against which vesting schedules are calculated.
	#[pallet::constant]
	type VestingReference: Get<Self::BlockNumber>;
}

Then you can remove this WeeksToBlocks thing and do:

fn calculate_vesting_duration<T: Config>(&self) -> T::BlockNumber {
	// gradient "m" of the linear curve function y = m*x + b
	const GRADIENT: FixedU128 = FixedU128::from_rational(2167u128, 1000u128);
	// negative constant (because we cannot have negative values, so we take the negative and do
	// "-b" instead of "+b") "b" of the linear curve function y = m*x + b
	const NEG_CONSTANT: FixedU128 = FixedU128::from_rational(2167u128, 1000u128);

	let multiplier_as_fixed = FixedU128::saturating_from_integer(self.0);
	let duration = GRADIENT.saturating_mul(multiplier_as_fixed).saturating_sub(NEG_CONSTANT);
	duration.saturating_mul(T::VestingReference::get()).max(1)
}

[JuaniRios]

• calculate_vesting_duration was already using a config type WeeksToBlocks, so how would adding VestingReference improve it?
• We only use DAYS inside types.rs, which provides default values to use for the config types, so I don't see a problem with this.
• I changed it now to DaysToBlocks to make it more usable.
• I believe DaysToBlocks is a more useful name than VestingReference

If you agree @lrazovic @joepetrowski I think this PR is ready to merge

[joepetrowski]

I don't agree. You are introducing the concept of days/weeks to this type because weeks is x in y = m*x + b.
https://github.com/Polimec/polimec-node/pull/70/files#diff-d5a7ca3423acfb2236018006a4ac93760a9e9893a8c864e55bef34403cafcb5dR68

What I mean by reference is that it's the independent variable against which vesting schedules...reference.

IMO the pallet (no matter how you split it into different files) should only deal with block numbers. Specific implementations related to days/weeks/etc. should be in the runtime. So perhaps just leave this as a trait and then implement it alongside the runtime.

[JuaniRios]

• Weeks is y. x is the multiplier stored inside self.0
• This type defined in the pallet is used only in the runtime. In the pallet we use the trait VestingDurationCalculation. We also have several other default implementations of config types inside types.rs. Would you suggest then to move them all inside the runtime file?
  That would make it harder to use them across other runtimes like the mock
• We cannot abstract away the concept of weeks in the pallet because that is what is calculated based on the multiplier. But the pallet does not need to know how many blocks a week is, since it is being converted with the config type.
• I like the .max(1) instead of the if statement