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

Feat: Remove retry mechanism #3835

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

iamsamgibbs
Copy link
Contributor

@iamsamgibbs iamsamgibbs commented Dec 3, 2024

Description

This PR removes the "Retry" mechanism from the UserHub. The retry mechanism was inherently flawed as it would only retry individual transactions, not entire sagas, meaning any mutations or nuances of the saga would be missed on a reattempt. The intention going forward is that actions should be retry-able from where they are created, which will be properly implemented after #2528 is complete as it will make retrying actions much simpler.

This PR also adds a temporary retry system to the create colony flow (which is currently broken on master) but this should be replaced in the future.

Testing

  • Step 1 - Connect with the metamask wallet and clear your activity tab data under advanced settings.
Screenshot 2024-12-04 at 14 40 54
  • Step 2 - Create an action and reject the transaction in the metamask popup
Screenshot 2024-12-04 at 14 12 25
  • Step 3 - Ensure the UserHub transactions list pops opens automatically and shows the transaction as failed. There should be no "Retry" option.
Screenshot 2024-12-04 at 14 12 36
  • Step 4 - Run node scripts/create-colony-url.js and open the link
  • Step 5 - Follow through all the steps choosing to create a new token
  • Step 6 - When you submit the form, reject the first transaction. You should be returned to the confirmation screen. Note the error toast too.
Screen.Recording.2024-12-04.at.14.38.21.mov
  • Step 7 - Resubmit the form, accept the first transaction but reject the second one. On master, this would stay on the same screen and let you retry the SetOwner transaction. However, regardless of whether that transaction succeeded or failed, it would not progress to the following transactions. This PR adjusts this step to show a new error message and properly allow retrying the subsequent transactions. It isn't pretty as this is only a temporary implementation and should be replaced when Refactor: Transactions API #2528 is complete which will allow actions to be properly retried.
Screen.Recording.2024-12-04.at.16.52.17.mov
  • Step 8 - Click retry, accept the first transaction but reject the second one. (Note, it may take some time for the transaction to appear).
Screen.Recording.2024-12-04.at.17.02.24.mov
  • Step 9 - Expect to see a different error, this time the SetOwner transaction has succeeded, but the extension related transactions failed. At this point the extensions can be manually installed in the colony itself so we can click the link to navigate to the colony. (Rather than implementing another temporary saga just for this edge case).

  • Step 10 - Repeat the create colony process. This time accept the first two transactions and reject the third. You should skip the retry stage and just go straight to the extension error notice.

Screen.Recording.2024-12-04.at.17.05.45.mov
  • Step 11 - Repeat the create colony process one more time. This time accept all the transactions and check the flow works as expected.
Screen.Recording.2024-12-04.at.17.06.46.mov

Diffs

Changes 🏗

  • Added temporary "finishCreateColony" saga and adjusts the create colony flow to make retrying possible

Deletions ⚰️

  • Removed unused "TransactionsItem" component and styles
  • Removed any uses of the transaction retry system

Resolves #3707
Resolves #3549

@iamsamgibbs iamsamgibbs self-assigned this Dec 3, 2024
@iamsamgibbs iamsamgibbs changed the title Feat/3707 remove retry transaction mechanism Feat: Remove retry mechanism Dec 4, 2024
@iamsamgibbs iamsamgibbs marked this pull request as ready for review December 4, 2024 17:14
@iamsamgibbs iamsamgibbs requested a review from a team as a code owner December 4, 2024 17: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.

Really nice work on fixing these issues @iamsamgibbs 🥇

Cleared the metamask activity
Screenshot 2024-12-05 at 11 41 31

Created a Simple payment and rejected the transaction
Screenshot 2024-12-05 at 11 42 13
The transaction appeared in the UserHub without the retry button
Screenshot 2024-12-05 at 11 42 21

Created a new colony and this is before form submission
Screenshot 2024-12-05 at 11 43 24
Screenshot 2024-12-05 at 11 43 32
Rejected the first transaction
Screenshot 2024-12-05 at 11 43 36
Resubmitted the form and accepted the 1st transaction
Screenshot 2024-12-05 at 11 44 04
But rejected the 2nd transaction
Screenshot 2024-12-05 at 11 44 11
Got redirected to the retry screen
Screenshot 2024-12-05 at 11 44 14
Screenshot 2024-12-05 at 11 44 45
Accepted all transactions
Screenshot 2024-12-05 at 11 45 01
And got redirected to the new colony page
Screenshot 2024-12-05 at 11 45 19
Then created a new colony, confirmed the first transaction and rejected the 2nd to get to the retry screen. Then accepted the 1st transaction and rejected the 2nd
Screenshot 2024-12-05 at 11 47 11
Pressed the Go to your colony link
Screenshot 2024-12-05 at 12 05 22

Created yet another colony, confirmed the first 2 transactions and rejected the 3rd and saw directly the error notice
Screenshot 2024-12-05 at 11 48 54

All good from my side, really nice job! 💯

@arrenv
Copy link
Member

arrenv commented Dec 5, 2024

Nice PR, thanks for working on this, I will try and do a review (don't hold out for me though).

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.

Holy hell this is some nice work here!!!

Tested both rejecting single, random transactions, as well as rejecting parts of the Colony creation flow.

Fun fact, we had a similar "colony creation retry" functionality in the old Dapp, when it was on Mainnet

Screencast.from.2024-12-05.17-13-17.mp4
Screencast.from.2024-12-05.17-14-31.mp4

Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Really really nice solution @iamsamgibbs 👏

All the flows look to be working great too! 🎉 Amazing stuff.

Screenshot 2024-12-09 at 17 22 32 Screenshot 2024-12-09 at 17 23 24 Screenshot 2024-12-09 at 17 24 18 Screenshot 2024-12-09 at 17 25 15

@iamsamgibbs iamsamgibbs merged commit c105d59 into master Dec 9, 2024
2 checks passed
@iamsamgibbs iamsamgibbs deleted the feat/3707-remove-retry-transaction-mechanism branch December 9, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants