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

Fix: Multi-sig extension settings issues #3780

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iamsamgibbs
Copy link
Contributor

@iamsamgibbs iamsamgibbs commented Nov 26, 2024

Description

This PR adds an improvement and fixes an issue with the extensions settings tabs.

The extension data is now refetched when saving changes to an extension and properly waits for these updates to be reflected in the database.

The multi-sig custom settings accordion is now open by default if any threshold is different than the colony threshold.

Testing

  • Step 1 - Install the Multi-Sig extension
  • Step 2 - Change the threshold setting to "Fixed threshold" with a value of 2.
Screenshot 2024-11-27 at 10 06 02
  • Step 3 - Without refreshing, navigate to the dashboard, then back to the multi-sig extension page. Check the threshold is still set to 2. (If you really want to be sure the threshold has updated correctly, then go through the extra steps of assigning multi-sig permissions and checking the threshold is as expected)
Screenshot 2024-11-27 at 10 06 25
  • Step 4 - Open the "Customize thresholds per team" accordion and set one of the domains to "Majority approval", then click "Save changes"
Screenshot 2024-11-27 at 10 09 29
  • Step 5 - Without refreshing, navigate to the dashboard, then back to the multi-sig extension page. Check the "Customize thresholds per team" accordion is already open.

Diffs

New stuff

  • Added new waitForCondition utility function
  • Added new waitForDbAfterExtensionSettingsChange function which is called on save changes with configurable extensions to ensure extension data is refetched after the change in settings is applied

Changes 🏗

  • Added new useEffect to MultiSigSettings component to compare domain thresholds against the global threshold and toggle the accordion open if any domain thresholds do not match

Resolves #3598
Resolves #3034

@iamsamgibbs iamsamgibbs self-assigned this Nov 26, 2024
@iamsamgibbs iamsamgibbs force-pushed the fix/3598-multi-sig-settings-issues branch from 928afbd to d2ac447 Compare November 27, 2024 09:52
@iamsamgibbs iamsamgibbs marked this pull request as ready for review November 27, 2024 10:14
@iamsamgibbs iamsamgibbs requested a review from a team as a code owner November 27, 2024 10:14
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @iamsamgibbs thanks for your super nice fix 🥇 👏

Tested updating the global threshold and upon returning to the extension page, the value is still displayed

Screen.Recording.2024-11-27.at.12.10.02.mov

Also if we update the threshold per team, the values get saved and upon return to the extension page, this section is toggled on

Screen.Recording.2024-11-27.at.12.10.44.mov

Just one thing that I have noticed: if you try to save the same values, after the timeout expires, an error toast is shown

Screen.Recording.2024-11-27.at.12.36.52.mov

So I'm wondering if there is anything we can do about it (maybe not show the Save settings button if the values are identical with the already saved ones) or even if it makes sense to solve this issue in this PR. What do you think @iamsamgibbs?

@iamsamgibbs
Copy link
Contributor Author

Just one thing that I have noticed: if you try to save the same values, after the timeout expires, an error toast is shown

I actually tried to implement hiding the save button as you suggested, but it turns out to be a bit of a pain. There is already an isDirty check to show / hide the button, but it doesn't reset itself if you re-select the same values. So yes to implement properly will require comparing all the form values against the currently saved values which is a bit more involved. I'll open a new issue for it. :)

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @iamsamgibbs! Then I'll approve this PR 💪

@iamsamgibbs
Copy link
Contributor Author

Created new issue #3783 to handle this

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this all without refreshing and all values show up as expected. Nicely fixed!

Screencast.from.2024-11-28.12-25-58.mp4

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitForCondition is such a great helper, kudos, we'll be able to use this to solve a lot of cases, (looking at you PaymentBuilderWidget 👀 )
The threshold is correctly updated ✔️ And I can also confirm that I could immediately start craeting multisig actions when I had enough memebrs
https://github.com/user-attachments/assets/84edac2b-2ded-471b-985f-aeb3e7c05ef9

I have however found a bug which.... I hope won't be too much of a pain to solve :')
If you set the global threshold to something else, it will update it, but not the domain values, so it opens the accordion.
I couldn't reproduce it 100% of the time, but basically

  1. set the threshold to 2, refresh the page
  2. set the threshold to 1 and see that the accordion opens and the previous values are used
    Attaching a video where you can see how this failed to happen the first time, but it happened the second time 🤷
multisig-threshold-bug-2024-11-28_11.46.05.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants