used MaxUnlockingChunks from config

[MrishoLukamba]

@kianenigma @ggwpez Fixes #12227

[MrishoLukamba]

Solved issue #12227

[kianenigma]

Please follow this syntax to close the linked issue https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

[MrishoLukamba]

closes #12227

[Ank4n]

@MrishoLukamba Can you also add a test that documents what happens in the scenario where MaxUnlockingChunks is reduced without migration? Here are couple of examples you can refer https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L4872 https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L5367

[MrishoLukamba]

@MrishoLukamba Can you also add a test that documents what happens in the scenario where MaxUnlockingChunks is reduced without migration? Here are couple of examples you can refer https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L4872 https://github.com/paritytech/substrate/blob/ankan/bound-staking-storage-trivial/frame/staking/src/tests.rs#L5367

Okey let me get into it, if anything I will ask

[MrishoLukamba]

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

[Ank4n]

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

Can you elaborate more? May be refer in the code.

I see that when we unbond, a new UnlockChunk is pushed (or UnlockChunk for same era updated). https://github.com/paritytech/substrate/blob/master/frame/staking/src/pallet/mod.rs#L984

[MrishoLukamba]

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

Can you elaborate more? May be refer in the code.

I see that when we unbond, a new UnlockChunk is pushed (or UnlockChunk for same era updated). https://github.com/paritytech/substrate/blob/master/frame/staking/src/pallet/mod.rs#L984

In the code it shows that,a new UnlockChunk is being pushed but in testing it is not pushed it is just updated the value field. So the bound MaxUnlockChunk placed there is not used at all. You can see in the new test i've added

[Ank4n]

@Ank4n @kianenigma There is an issue which I noticed, StakingLedger.staking BoundedVec does not push new object (UnlockingChunks) instead it just increments the value inside UnlockingChunks. So the MaxUnlockingChunks bound in boundedVec is not quite useful, I think is a bug.

Can you elaborate more? May be refer in the code. I see that when we unbond, a new UnlockChunk is pushed (or UnlockChunk for same era updated). https://github.com/paritytech/substrate/blob/master/frame/staking/src/pallet/mod.rs#L984

In the code it shows that,a new UnlockChunk is being pushed but in testing it is not pushed it is just updated the value field. So the bound MaxUnlockChunk placed there is not used at all. You can see in the new test i've added

@MrishoLukamba The behaviour looks correct to me. An UnlockChunk is only updated if multiple unbond happen in the same era. If you try to move forward in eras and unbond, you should see multiple chunks being added. If the UnlockChunks are already full, the transaction should return an error NoMoreChunks as the boundedVec is full.

@kianenigma Shouldn't we be able to keep the MaxUnlockChunk equal to BondingDuration. This will imply users can have UnlockChunks without claiming only for up to 28 eras. And in that case all the UnlockChunks are ready to be claimed (after 28 eras) so encouraging the user to just claim existing unlocks before requesting for more unlocks. If you agree with above, we can bound StakingLedger.unlocking with BondingDuration and not have to introduce a new bound MaxUnlockChunk.

[MrishoLukamba]

Logic seems correct, but code needs to be formatted.

Done, and should i use cargo fmt?

[Ank4n]

@MrishoLukamba I will close this PR and work on top of your code to get it merged. Pls add your Polkadot address in the PR for the tip.

Super thanks for your contribution. Feel free to follow the new PR here: https://github.com/paritytech/substrate/pull/12343

[MrishoLukamba]

@MrishoLukamba I will close this PR and work on top of your code to get it merged. Pls add your Polkadot address in the PR for the tip.

Super thanks for your contribution. Feel free to follow the new PR here: #12343

Thanks, will also continue contributing on other issues.

[Ank4n]

/tip small

[substrate-tip-bot[bot]]

@Ank4n You are not allowed to request a tip. Only shawntabrizi, gavofyork, rphmeier, athei, andresilva, arkpar, bkchr, eskimor, drahnr, dvdplm, robbepop, cmichi, tomaka, pepyakin, tomusdrw, kianenigma, jacogr, rossbulat are allowed.

[kianenigma]

/tip small

[substrate-tip-bot[bot]]

@kianenigma A small tip was successfully submitted for MrishoLukamba (133WyzJ4cEEbQ4twhT3nQpoXYeTCzeKn8kWfRfEi4XDF7wKh on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

[MrishoLukamba]

/tip small

Sorry @kianenigma @Ank4n https://polkadot.js.org/apps/#/treasury/tips, the tip didnt go through