feat: add multiple update* methods

[pscott]

I initially wanted to go for an updateSettings that encompasses all the settings we have, however it is currently not possible because it would break the forge coverage tool we are using (indeed, the optimizer isn't used when using forge coverage, which results in a stack too deep).

So I've decided to go for:

    function updateStrategies(
       Strategy calldata _proposalValidationStrategy,
       address[] calldata _authenticatorsToAdd,
       address[] calldata _authenticatorsToRemove,
       Strategy[] calldata _votingStrategiesToAdd,
       string[] calldata _votingStrategiesMetadataURIsToAdd,
       uint8[] calldata _votingIndicesToRemove
   ) external;
   
   function updateSettings(
       uint32 _minVotingDuration,
       uint32 _maxVotingDuration,
       uint32 _votingDelay,
       string calldata _metadataURI
   ) external;

Closes #148

[codecov]

Codecov Report

Merging #149 (3de402b) into main (538ae1c) will decrease coverage by 0.39%.
The diff coverage is 86.84%.

@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   84.35%   83.96%   -0.39%     
==========================================
  Files          28       28              
  Lines         409      424      +15     
  Branches       91      101      +10     
==========================================
+ Hits          345      356      +11     
- Misses         54       55       +1     
- Partials       10       13       +3     

Impacted Files	Coverage Δ
src/Space.sol	95.31% <86.84%> (-2.92%)	⬇️

[Orland0x]

Agree with this PR, adding and removing strategies separately was bad UX.

Do we need to keep the original single use entry points now then?

[pscott]

Agree with this PR, adding and removing strategies separately was bad UX.

Do we need to keep the original single use entry points now then?

I originally thought "yes" because it would provide more granularity but if we wish to reduce code bloat then maybe we could remove them indeed.

[Orland0x]

Agree with this PR, adding and removing strategies separately was bad UX.
Do we need to keep the original single use entry points now then?

I originally thought "yes" because it would provide more granularity but if we wish to reduce code bloat then maybe we could remove them indeed.

yeah i say we remove them. I think we should have just a single entry point updateSettings. Users can just leave arguments empty that they dont want to change.

[pscott]

Removed the external granular entrypoints and decided to keep only updateSettings and updateStrategies.

If you wish to ignore some fields, use:

• 0xffffffff for uin32s (votingDelay, min/maxVotingDuration) (NO_UPDATE_DURATION)
• I do not want to update the metadataURI to ignore setting the metadatauri (NO_UPDATE_METADATA_URI).
• 0x1337133713371337133713371337133713371337 to ignore the proposal Strategy (NO_UPDATE_PROPOSAL_STRATEGY)
• Empty arrays to ignore everything that's passed as an array (authenticators to add/remove, voting strategies to add/remove)

Also removed the EmptyArray check in the internal functions since they are now special values in updateStrategies. Added the checks in the constructor (see a10fd7d)

[arr00]

Are no-update keys the best solution to this? I feel like the experience for anyone not using sx frontend is not intuitive.

[Orland0x]

Are no-update keys the best solution to this? I feel like the experience for anyone not using sx frontend is not intuitive.

Have you got any other ideas? Its not ideal, but as 99%+ of use will be through our UI i feel the ux improvement is worth it.

[arr00]

Are no-update keys the best solution to this? I feel like the experience for anyone not using sx frontend is not intuitive.

Have you got any other ideas? Its not ideal, but as 99%+ of use will be through our UI i feel the ux improvement is worth it.

Even with our frontend, for users who check what they sign, it will be very confusing. There could be a uint8 where you set update bit flags (this is still not good ui). We could also just have a bunch of update booleans as parameters and it wouldn't cause such a large increase in gas (assuming a chain with cheap calldata).

Considering these options, current approach may be best (definitely should comment in values of no update keys though)

[Orland0x]

tACK