glifio / safe

A Filecoin multisig wallet
https://safe.glif.io
1 stars 0 forks source link

Fix issue with removing signers not applying #147

Closed Schwartz10 closed 2 years ago

Schwartz10 commented 2 years ago

Hey @navFooh this issue fixes a weird one that I'm surprised we didn't see before - invalid computation of decrease param in RemoveSigner.

Basically what happens (and i can't figure out if this was introduced recently from some other change or package upgrade) - the initialState of the decrease state variable is always set to true because on first render, NumApprovalsThreshold and Signers.length are both undefined.

If the user doesn't toggle the decrease toggle button, then the value will always be true, even if it shouldn't be.

There are two resulting not good scenarios (1) remove signer messages fail to get applied to the change since a user might have attempted to decrease when NumApprovalThreshold was already 1 or (2) approval thresholds actually decrease when they shouldn't.

I fixed this with a useEffect because it was the easiest way for me for now, but I'm not sold on this being the best approach here - it might make sense to have a default value, and then a user-set value (decrease) that overrides the default when it's set. This way we dont mess around with side effects