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

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

Open
ggwpez opened this issue Aug 22, 2023 · 9 comments
Open

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

ggwpez opened this issue Aug 22, 2023 · 9 comments
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@ggwpez
Copy link
Member

ggwpez commented Aug 22, 2023

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

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed J0-enhancement labels Aug 25, 2023
@kianenigma
Copy link
Contributor

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
Copy link
Member Author

ggwpez commented Oct 17, 2023

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
Copy link
Member

bkchr commented Oct 17, 2023

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
Copy link
Member Author

ggwpez commented Oct 17, 2023

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:

  • people having more conviction than they should have
  • OR people getting less conviction than they should have
    Its both not going to break anything, just not a nice use experience.

@bkchr
Copy link
Member

bkchr commented Oct 17, 2023

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
Copy link
Contributor

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
Copy link
Member

bkchr commented Oct 17, 2023

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

@xlc
Copy link
Contributor

xlc commented Oct 17, 2023

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
Copy link
Member Author

ggwpez commented Nov 12, 2023

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)

serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
jonathanudd pushed a commit to jonathanudd/polkadot-sdk that referenced this issue Apr 10, 2024
claravanstaden pushed a commit to claravanstaden/polkadot-sdk that referenced this issue Sep 19, 2024
…ion header storage (paritytech#125)

* Remove execution header storage & submit inbound message with execution update

* Fix ci breaking

* More refactoring

* Update beacon test fixtures

* Format code

* Initialize storage with finalized checkpoint for benchmark

* Update weights include verify the execution proof

* Fix breaking test

* Remove fixture not in use

* Add detail for InvalidExecutionProof

* Initialize checkpoint for tests on demand

* Revert error as NotBootstrapped for submit_update_with_missing_bootstrap

* Add verify_execution_header tests back

* Add error test back

* Cleanup templates

* Update bridges/snowbridge/primitives/beacon/src/types.rs

Co-authored-by: Vincent Geddes <[email protected]>

* Narrowed inputs

* Remove fields irrelevant from proof

* Update bridges/snowbridge/primitives/core/src/inbound.rs

Co-authored-by: Vincent Geddes <[email protected]>

* Update bridges/snowbridge/pallets/ethereum-client/src/impls.rs

Co-authored-by: Vincent Geddes <[email protected]>

* Polish

---------

Co-authored-by: Vincent Geddes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

6 participants