Feat: Improve failed extension transaction handling #3349

iamsamgibbs · 2024-10-16T10:09:53Z

Description

This PR aims to improve failed transactions when installing / enabling / assigning permissions for extensions.

Currently, when installing an extension it will call the extensionInstallAndEnable saga which includes transactions for install, initialise and setUserRoles (and will conditionally call them depending on the extension configuration).

However, if either the initalise or setUserRoles transaction fails or is rejected, the UI will not handle this error state correctly and will appear as though the extension has not been installed (even though the install transaction succeeded).

This PR aims to resolve this by adding error steps to the extensionInstallAndEnable saga and pick these up in the error handler to update the UI as appropriate.

Whilst working on this, I discovered the same issue affects the extensionEnable saga and related handlers. This PR applies the same changes to the extensionEnable saga and the places from which that is called.

I also discovered an issue when re-enabling an extension which I have created a new issue for (#3430) and left a TODO in this PR which suggests a starting point for a potential fix.

Testing

This one is going to be a pain to test I am afraid! Before anything else, switch to the metamask wallet as we need to be able to reject specific transactions.

There a three extensions we will be testing with as the install / enable steps work slightly differently for each.

Reputation Weighted Extension

This extension has an install step which calls only the install transaction. It then has an entirely separate enable step which calls the initialise and setUserRoles transactions. This is the only extension which explicitly has this extra enable step.

Let us first test with the intended use.

Step 1 - Navigate to: http://localhost:9091/planex/extensions/VotingReputation
Step 2 - Click the "Install" button and accept the install transaction. The transaction should succeed, the success toast should appear and the tab should automatically switch to the "Extension settings" tab.
Step 3 - Select a governance style and click the "Enable" button. Accept both the initialise and setUserRoles transactions. The transactions should succeed, the install success toast should appear and the tab should automatically switch to the "Overview" tab.

Now let us test rejecting some of the transactions. First we must remove the extension.

Step 4 - Deprecate the extension.
Step 5 - Uninstall the extension.

Okay, we will reject each transaction.

Step 6 - Click the "Install" button and reject the install transaction. The transaction should fail, the install error toast should appear.
Step 7 - Click the "Install" button and accept the install transaction. Select a governance style and click the "Enable" button. This time, reject the initialise transaction. The setUserRoles transaction should not appear at all, the initialise transaction should fail, an enable error toast should appear.
Step 8 - Click the "Enable" button again. Accept the initialise transaction and reject the setUserRoles transaction. The initialise transaction should succeed, the setUserRole transaction should fail, the "Failed to assign extension permissions" toast should appear. Also note that the extension is intialised, the "Missing permissions banner" should appear.
Step 9 - Click "Enable permissions" in the "Missing permissions banner". Reject the setUserRoles transaction. The banner should remain visible.
Step 10 - Click "Enable permissions" in the "Missing permissions banner". Accept the transaction. The banner should switch to the green success banner.

Multi-sig Extension

This extension has only the install step, but calls both the install and setUserRoles transactions. It has no initialise transaction.

Feel free to test with the intended use to make sure it still functions properly, but I will only detail the testing steps which cover the new functionality.

Step 1 - Navigate to: http://localhost:9091/planex/extensions/MultisigPermissions
Step 2 - Click the "Install" button and reject the install transaction. The transaction should fail, the install error toast should appear.
Step 3 - Click the "Install" button. Accept the install transaction and reject the setUserRoles transaction. The install transaction should succeed, the setUserRoles transaction should fail, the install success toast should show (as technically the install did succeed), but also the assign extension permissions error toast should show. The tab should automatically switch to the "Extension settings" tab and the "Missing permissions banner" should show.
Step 4 - Change the threshold type to a fixed threshold with 2 approvals, and click the "Save changes" button. Accept the multicall transaction. The "Missing permissions banner" should remain visible and the tab should not change.

Feel free to retest the Manage permissions banner functionality here too if you wish.

Staking Advanced Payments

This extension has only the install step and calls all 3 of the install, initialise and setUserRoles transactions.

Step 1 - Navigate to: http://localhost:9091/planex/extensions/StakedExpenditure
Step 2 - This extension is installed by default, so deprecate and uninstall the extension.
Step 3 - Rejecting the install transaction should work exactly the same as the last two extensions, so let us skip that step. Click the "Install" button. Accept the install transaction and reject the initialise transaction. The install transaction should succeed, the initialise transaction should fail and the setUserRoles transaction should not get called at all. The install success toast should show, but also the enable extension error toast should show. The tab should automatically switch to the "Extension settings" tab. The "Missing permissions banner" should not yet show at this point as the extension is not yet enabled. This is different than the multi-sig extension as the multi-sig extension does not have the initialise step.
Step 4 - Click the "Enable" button. Reject the initialise transaction. The setUserRoles transaction should not appear at all, the initialise transaction should fail, an enable error toast should appear.
Step 5 - Click the "Enable" button again. Accept the initialise transaction and reject the setUserRoles transaction. The initialise transaction should succeed, the setUserRole transaction should fail, the "Failed to assign extension permissions" error toast should appear. Also note that the extension is intialised, the "Missing permissions banner" should appear.

From here, test the missing permissions banner functionality if you wish.

Final bit of testing, if you have made it this far, have a drink to sustain you for the last bit! ☕ We will test accepting the install and initialise transactions, but rejecting the setUserRoles transaction.

Step 6 - Remove the extension (deprecate and uninstall)
Step 7 - Click the "Install" button. Accept the install transaction. Accept the initialise transaction. Reject the setUserRoles transaction. The install success toast should show, but also the extension permissions error toast should show. The tab should automatically switch to the "Extension settings" tab. This time the "missing permissions banner" should show as the extension has been enabled this time around.

Further testing: Feel free to try and break this in anyway you can but the testing steps are hopefully thorough enough that this is not possible. There is an unrelated issue on master related to deprecating and re-enabling where the missing permissions banner will incorrectly show (#3430).

Diffs

New stuff ✨

Added error flags to extensionInstallAndEnable and extensionEnable sagas
Added new handleWaitingForDbAfterFormCompletion util

Changes 🏗

waitForDbAfterExtensionAction handles new error flags
useInstall hook now uses handleWaitingForDbAfterFormCompletion util in onSuccess and onError handlers
ExtensionsSettingsForm uses handleWaitingForDbAfterFormCompletion util in onSuccess and onError handlers

TODO

Added an @todo as a recommended starting point for [Bug] Missing permissions banner incorrectly shows when reenabling an extension #3430

Resolves #2754
Contributes to #3337

mmioana · 2024-10-16T11:07:25Z

...onents/frame/Extensions/pages/ExtensionDetailsPage/partials/ExtensionDetailsHeader/hooks.tsx

+        await waitForDbAfterExtensionAction({
+          method: ExtensionMethods.INSTALL,
+          refetchExtensionData,
+          initialiseTransactionFailed,
+          setUserRolesTransactionFailed,
+        });
+
+        toast.success(
+          <Toast
+            type="success"
+            title={{ id: 'extensionInstall.toast.title.success' }}
+            description={{
+              id: 'extensionInstall.toast.description.success',
+            }}
+          />,
+        );


@iamsamgibbs if there could be some errors while either initialising the transaction or setting the user roles, does it make sense to refetch the extension action and show the success toast?

In the case where the initialise transaction fails, I think you might be right that we don't need to refetch the extension data.

In the case where the setUserRole transaction fails, we still want to refetch the extension data to check that the extension has been enabled still.

As for the success toast, it's a bit of a confusing ones as technically the install was successful, but the initalise / setUserRoles transactions failed, so I've opted to still show the install success but also show a failure toast for initalise / setUserRoles.

mmioana · 2024-10-16T11:10:51Z

src/redux/sagas/extensions/extensionInstallAndEnable.ts

@iamsamgibbs I really like your approach with the error steps 🙌 though I'm wondering if it is not more clear to set the initialiseTransactionFailed and setUserRolesTransactionFailed flags directly?

bassgeta · 2024-10-16T12:32:16Z

src/redux/sagas/extensions/extensionInstallAndEnable.ts

+      yield waitForTxResult(installExtension.channel);
+    } catch (error) {
+      // Declare an error step for the error handler
+      error.step = ExtensionInstallAndEnableErrorStep.InstallExtension;


we could throw specific errors and then have typeguards too
Like for example a ExtensionInstallError which has a step property like this component I just randomly pulled up

and then we can check the error like this:
if (error instanceof ExtensionInstallError)

and the step property will be defined

Maybe it's overkill, just wanted to drop this here :D

…ansaction handling

…ompletion

iamsamgibbs · 2024-10-22T15:35:13Z

src/components/frame/Extensions/pages/ExtensionDetailsPage/partials/ExtensionSettings/utils.tsx

+export const getExtensionSettingsDefaultValues = (
+  extensionData: AnyExtensionData,
+): object => {
+  const { initializationParams = [] } = extensionData;
+  const defaultValues = getInitializationDefaultValues(initializationParams);
+
+  if (!isInstalledExtensionData(extensionData)) {
+    return defaultValues;
+  }

+  switch (extensionData.extensionId) {
+    case Extension.StakedExpenditure: {
+      return {
+        stakeFraction: extensionData?.params?.stakedExpenditure?.stakeFraction
+          ? convertFractionToEth(
+              extensionData.params.stakedExpenditure.stakeFraction,
+            )
+          : String(defaultValues.stakeFraction),
+      };
+    }
+    case Extension.MultisigPermissions: {
+      const { colonyThreshold: globalThreshold } =
+        extensionData.params?.multiSig ?? {};
+
+      return {
+        thresholdType:
+          globalThreshold !== undefined
+            ? getGlobalThresholdType(globalThreshold)
+            : MultiSigThresholdType.MAJORITY_APPROVAL,
+        globalThreshold: globalThreshold ?? 0,
+        domainThresholds: [],
+      } satisfies MultiSigSettingsFormValues;
+    }
+    default: {
+      return defaultValues;
+    }
+  }
+};


This is just moving an existing function up to please the liner. Below here are the actual changes.

rdig

You weren't kidding that this was finniky to test, but managed to do so in the end :)

Everything seems to be working as expected, so nice work as always 💯

Voting Reputation

Multisig

*Staking

bassgeta

This was a lot to untangle damn, really nice job!
It covers so many edge cases and the comments are quite welcome since all of our extension enabling has so much proprietary logic :')
Thanks for the testing steps 💯

Voting reputation

Installing the voting reputation extension works ✔️

Rejecting installation ✔️

Failed initialisation ✔️

Rejected set user roles ✔️

Rejection from the banner ✔️

Everything goes through when signing the tx ✔️

Multisig

Rejected the install tx ✔️

Signed the enable step but rejected set user roles ✔️

The banner is shown ✔️

Settings still work ✔️

Staking advanced payments

Rejected initialise ✔️

Rejected set user roles ✔️

Accept install, initialise and reject set user roles ✔️

🚢

mmioana

Such a nice solution to look at code-wise and also to test 💯 Really nice job here @iamsamgibbs 🙌 🥇

Went through the testing steps for the Reputation Weighted Extension extension

Then went through the testing steps for Multi-sig

And finally got to the Staking Advanced Payments extension - here I also tested rejecting the deprecate and uninstall transactions and everything was working as expected

All nice and clean, let's get it merged 🎉

iamsamgibbs self-assigned this Oct 16, 2024

mmioana reviewed Oct 16, 2024

View reviewed changes

bassgeta reviewed Oct 16, 2024

View reviewed changes

feat: improve failed extension install, initialise and setUserRole tr…

4380ba0

…ansaction handling

iamsamgibbs force-pushed the feat/2754-improve-failed-extension-transaction-handling branch from 5f905ce to 4380ba0 Compare October 21, 2024 12:35

iamsamgibbs added 7 commits October 21, 2024 16:57

feat: add ExtensionInstallAndEnableError

c0cdc34

feat: refactor waitForDbAfterExtensionAction util for install and enable

2117a93

Feat: Handle extensionEnable failed transactions

82b22b8

Fix: Handle PermissionsNeededBanner isSaveChanges issue

acde597

Fix: Destructure method in utils

37c4110

Fix: Add todo in PermissionsNeededBanner

12f4573

Feat: Refactor handleInstallError to use handleWaitingForDbAfterFormC…

4c91e08

…ompletion

iamsamgibbs commented Oct 22, 2024

View reviewed changes

iamsamgibbs mentioned this pull request Oct 22, 2024

[Bug] Missing permissions banner incorrectly shows when reenabling an extension #3430

Closed

iamsamgibbs marked this pull request as ready for review October 22, 2024 15:58

iamsamgibbs requested review from a team as code owners October 22, 2024 15:58

rdig approved these changes Oct 23, 2024

View reviewed changes

iamsamgibbs requested review from bassgeta and mmioana October 23, 2024 16:41

bassgeta approved these changes Oct 25, 2024

View reviewed changes

iamsamgibbs mentioned this pull request Oct 29, 2024

Multi-sig permissions extension installation issues #3337

Closed

mmioana approved these changes Oct 30, 2024

View reviewed changes

iamsamgibbs merged commit 41132c8 into master Oct 30, 2024
4 of 6 checks passed

iamsamgibbs deleted the feat/2754-improve-failed-extension-transaction-handling branch October 30, 2024 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Improve failed extension transaction handling #3349

Feat: Improve failed extension transaction handling #3349

iamsamgibbs commented Oct 16, 2024 •

edited by rdig

Loading

mmioana Oct 16, 2024

iamsamgibbs Oct 16, 2024

mmioana Oct 16, 2024

bassgeta Oct 16, 2024

iamsamgibbs Oct 22, 2024

rdig left a comment

bassgeta left a comment

mmioana left a comment

Feat: Improve failed extension transaction handling #3349

Feat: Improve failed extension transaction handling #3349

Conversation

iamsamgibbs commented Oct 16, 2024 • edited by rdig Loading

Description

Testing

Reputation Weighted Extension

Multi-sig Extension

Staking Advanced Payments

Diffs

TODO

mmioana Oct 16, 2024

Choose a reason for hiding this comment

iamsamgibbs Oct 16, 2024

Choose a reason for hiding this comment

mmioana Oct 16, 2024

Choose a reason for hiding this comment

bassgeta Oct 16, 2024

Choose a reason for hiding this comment

iamsamgibbs Oct 22, 2024

Choose a reason for hiding this comment

rdig left a comment

Choose a reason for hiding this comment

bassgeta left a comment

Choose a reason for hiding this comment

Voting reputation

Multisig

Staking advanced payments

mmioana left a comment

Choose a reason for hiding this comment

iamsamgibbs commented Oct 16, 2024 •

edited by rdig

Loading