entropyxyz / entropy-core

Protocol and cryptography development.
https://docs.entropy.xyz/
GNU Affero General Public License v3.0
11 stars 2 forks source link

No unbonding when signer or next signer #1031

Closed JesseAbram closed 1 month ago

JesseAbram commented 2 months ago

Related #942

This is a little controversial as it hurts UX a bit but I'm ok with that. I did wanna bring up that this breaks convention of staking and maybe if we rename this pallet to staking and mock all the calls in that pallet we can use all the frontends that are built around staking, a little more pallet code but a hella a lot less GUI code.

Also @HCastano wanna double check all my entries here, I read up and I believe there are only three ways to unbond from a validator

but second eyes on that would be nice.

Closes #942.

JesseAbram commented 2 months ago

+1 This seems like something we need.

One question i have is how do we force people to use these wrappers and not call the underlying pallet-staking extrinsics? Are they somehow not exposed by our runtime?

That is handled in the base call filter here https://github.com/entropyxyz/entropy-core/pull/1031/files#diff-0ec06ea58bd455f09ce6b3bb4c2c1c0d37bda51c1e1be2151c560c9c973959ecR320-R322

Also, if i understand right, validators can anyway only leave at a session change, which is currently when we do a reshare. So if they call unbond (or chill etc) during a session, could we mark them as being the one who wants to leave the signing committee at the next reshare - or block them from being chosen as a next signer. That way validators can leave whenever they want.

The argument against this would be that it would make it more complicated to change the frequency of reshares, which i think we will want to do, and there could be complications or security issues if several members of the signing comittee want to unbond in the same session.

Yes I had this same thought, however this works if one wants to, but if more than one wants to it gets messy, and it isn't like we are locking them in forever, it isn't that long ( a day or two) I think this is worse UX but way simpler, that being said, id say lets do this for now and we can potentially look at and discuss having them pushed out if they want to unbond earlier

HCastano commented 2 months ago

I did wanna bring up that this breaks convention of staking and maybe if we rename this pallet to staking and mock all the calls in that pallet we can use all the frontends that are built around staking, a little more pallet code but a hella a lot less GUI code.

Oh also, can you elaborate on this? Not sure how renaming and mocking ties into using staking frontends

JesseAbram commented 2 months ago

I did wanna bring up that this breaks convention of staking and maybe if we rename this pallet to staking and mock all the calls in that pallet we can use all the frontends that are built around staking, a little more pallet code but a hella a lot less GUI code.

Oh also, can you elaborate on this? Not sure how renaming and mocking ties into using staking frontends

mocking may not be the write word, wrapping is probably better. Pretty much all staking GUIs would call staking.chill or staking.function. Our code blocks that for the specific calls (chill, unbond........etc) so we can add some extra logic, it would be stakingExtention.function, if we rename staking_extention to staking and staking to idk frame_staking, and wrap all the rest of the calls in that pallet in staking extention, (and keep the same interface) then the GUIs for staking like polkadot js frontend or any other staking wallets should just work, instead of us having to roll our own

JesseAbram commented 2 months ago

Okay, so while the direction of this is correct, the implementation breaks the nominator flow since they can also chill(), unbond(), etc.

What we need to do here is check if an account is a validator or nominator and act differently depending on the case. See chill_stash() from the Staking pallet for example.

I think I get it, i think I broke the nominator flow, just to be sure we are on the same page, I still wanna apply the same logic to both however for nominator I need to get who they are nominating and for a validator same flow just get the related stash to the controller

HCastano commented 2 months ago

mocking may not be the write word, wrapping is probably better. Pretty much all staking GUIs would call staking.chill or staking.function. Our code blocks that for the specific calls (chill, unbond........etc) so we can add some extra logic, it would be stakingExtention.function, if we rename staking_extention to staking and staking to idk frame_staking, and wrap all the rest of the calls in that pallet in staking extention, (and keep the same interface) then the GUIs for staking like polkadot js frontend or any other staking wallets should just work, instead of us having to roll our own

Umm, not sure if it'll be that simple. UIs will probably want to do validation on their end anyways, and we're emitting different events than the Staking pallet (so the UI might say something succeeded, but in reality it failed). Something worth following up on though.

I think I get it, i think I broke the nominator flow, just to be sure we are on the same page, I still wanna apply the same logic to both however for nominator I need to get who they are nominating and for a validator same flow just get the related stash to the controller

For the nominator flow we need to allow a nominator to call chill(), unbond(), etc. That's not possible right now since the nominator account would fail the checks that the controller is a signer.

So here we have a couple of options:

  1. Skip some of these checks in that case that an an account is a nominator and not a validator.
  2. If a controller is a nominator, check to see who they're delegaging to. If that person is a signer then we don't allow them to perform an action.

I can see (2) being pretty annoying for nominators, but this would help prevent the case where a validator only puts up a bond of say, 1 UNIT themselves and then uses a different nomination account to get reach the required stake.

JesseAbram commented 2 months ago

mocking may not be the write word, wrapping is probably better. Pretty much all staking GUIs would call staking.chill or staking.function. Our code blocks that for the specific calls (chill, unbond........etc) so we can add some extra logic, it would be stakingExtention.function, if we rename staking_extention to staking and staking to idk frame_staking, and wrap all the rest of the calls in that pallet in staking extention, (and keep the same interface) then the GUIs for staking like polkadot js frontend or any other staking wallets should just work, instead of us having to roll our own

Umm, not sure if it'll be that simple. UIs will probably want to do validation on their end anyways, and we're emitting different events than the Staking pallet (so the UI might say something succeeded, but in reality it failed). Something worth following up on though.

I think I get it, i think I broke the nominator flow, just to be sure we are on the same page, I still wanna apply the same logic to both however for nominator I need to get who they are nominating and for a validator same flow just get the related stash to the controller

For the nominator flow we need to allow a nominator to call chill(), unbond(), etc. That's not possible right now since the nominator account would fail the checks that the controller is a signer.

on reading into it, it actually wouldn't but the check won't apply to a nominator which also needs to be fixed. pretty much stash would not apply to who the nominator is validating but their own stash and would fail, however this info is kept in the Nominators storage struct

So here we have a couple of options:

  1. Skip some of these checks in that case that an an account is a nominator and not a validator.
  2. If a controller is a nominator, check to see who they're delegaging to. If that person is a signer then we don't allow them to perform an action.

I can see (2) being pretty annoying for nominators, but this would help prevent the case where a validator only puts up a bond of say, 1 UNIT themselves and then uses a different nomination account to get reach the required stake.

yes 2 is where I was landing mentally too, you don't need to bond from your validator and it is needed