paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.62k stars 565 forks source link

[FRAME] Re-normalization migration for `pallet-conviction-voting` #125

Open ggwpez opened 10 months ago

ggwpez commented 10 months ago

In order to enact the new conviction voting period mentioned here, we need to create a migration since the factors will not be applied retroactively on their own.
The migration needs to retroactively fix-up all votes and factors.

Context from chat:

I was about to implement this, but realized that the old votes will not change over to the new conviction factor retroactively. The capital is multiplied with the factor at time of voting here. So we either A) lock up funds longer than advertised but give the correct conviction factor, or B) lock up as long as advertised but give more votes than would be due for the locked time. As example: Someone who voted with 2x and got locked up for 14 D (intead of 56) will get the 2x voting factor even after we re-normalized them to only give 0.3 for 14 Days

kianenigma commented 8 months ago

will not change over to the new conviction factor retroactively

What a bout a fn nudge_voting_conviction() (freely) callable by anyone that will recalculate the conviction?

ggwpez commented 8 months ago

What a bout a fn nudge_voting_conviction() (freely) callable by anyone that will recalculate the conviction?

This would be callable on all votes? Then it would also apply to votes that were cast with the correct config from the time before out miss-configuration.

bkchr commented 8 months ago

Doesn't this only means that whoever voted before changing the conviction period will just get the wrong conviction period? Aka earlier unlocks, but no other bugs?

ggwpez commented 8 months ago

Doesn't this only means that whoever voted before changing the conviction period will just get the wrong conviction period? Aka earlier unlocks, but no other bugs?

I dont think there will be any bugs without a migration, its just that the period and conviction for past votes would lead either to:

bkchr commented 8 months ago

Polkadot js was always showing the conviction period. Do we know what other wallets etc have shown? From a use experience POV, I would probably not the fix the old periods and live with the shorter lockup.

kianenigma commented 8 months ago

This would be callable on all votes? Then it would also apply to votes that were cast with the correct config from the time before out miss-configuration.

Yes, in which case it would do nothing and would charge tx-fees.

If this is a one-ff temporary inconsistency, I would also prefer to not deal with it. Voting seems to be this case. But we have to look into existing delegations and if/how the conviction applied to delegation will be impacted. This is not something that we can overlook.

bkchr commented 8 months ago

Yeah, I mean we should check that everything works if we do it without a migration.

xlc commented 8 months ago

Other chains may also have similar issue if they want to adjust the parameters as well so it will be best if we have something to better handle the change of parameters.

ggwpez commented 7 months ago

Other chains may also have similar issue if they want to adjust the parameters as well so it will be best if we have something to better handle the change of parameters.

Yes. I think a good start is to change the hard-coded Conviction enum to a trait. Then we have much more flexibility in the runtime instead of forcing all downstream users of the pallet to adopt our new lock and conviction mults.

 type Conviction:
    // Convert a Conviction to a Lock Duration. This obsoletes `VoteLockPeriod`.
    Convert<Self::Conviction, BlockNumberFor<Self>>
    // Convert a Conviction and a Balance to a number of Votes (often by multiplication).
    + Convert<(Self::Conviction, BalanceOf<Self, I>), BalanceOf<Self, I>>
    + Eq
    + Default
    + Copy
    + Parameter
    + Member
    + MaxEncodedLen;

(the conversions can be split up, if neccecary)