feat: edit recovery flow #2824

iamacook · 2023-11-20T15:52:15Z

What it solves

Resolves #2761

How this PR fixes it

This modifies the EditRecoveryFlow to be UpsertRecoveryFlow which now handles editing a recovery Delay Modifier.

How to test it

Open a Safe with a deployed recovery Delay Modifier and navigate to the (note: temporary) settings, observing the edit button that successfully transactions/alters the settings.

Screenshots

Checklist

I've tested the branch on mobile 📱
I've documented how it affects the analytics (if at all) 📊
I've written a unit/e2e test for it (if applicable) 🧑‍💻

github-actions · 2023-11-20T15:52:29Z

Branch preview

⏳ Deploying a preview site...

github-actions · 2023-11-20T15:54:55Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2023-11-20T15:59:28Z

Coverage report

❌ An unexpected error occurred. For more details, check console

Error: The process '/usr/local/bin/yarn' failed with exit code 1

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	75.32% (+0.04% 🔼)	10797/14334
🔴	Branches	50.05% (+0.03% 🔼)	2187/4370
🔴	Functions	57.95% (+0.12% 🔼)	1618/2792
🟡	Lines	76.78% (+0.02% 🔼)	9787/12747

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / UpsertRecoveryFlowEmailHint.tsx	60%	100%	0%	60%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / setup.ts	91.38% (-8.62% 🔻)	80% (-20% 🔻)	91.67% (-8.33% 🔻)	90.57% (-9.43% 🔻)

Test suite run failed

Failed tests: 5/1226. Failed suites: 2/174.

  ● RecoveryModal › component › should render the in-progress modal when there is a queue for guardians

    expect(received).toBeTruthy()

    Received: null

      76 |
      77 |       expect(queryByText('Test')).toBeTruthy()
    > 78 |       expect(queryByText('Account recovery in progress')).toBeTruthy()
         |                                                           ^
      79 |     })
      80 |
      81 |     it('should render the in-progress modal when there is a queue for owners', () => {

      at Object.toBeTruthy (src/components/recovery/RecoveryModal/index.test.tsx:78:59)

  ● RecoveryModal › component › should render the in-progress modal when there is a queue for owners

    expect(received).toBeTruthy()

    Received: null

      90 |
      91 |       expect(queryByText('Test')).toBeTruthy()
    > 92 |       expect(queryByText('Account recovery in progress')).toBeTruthy()
         |                                                           ^
      93 |     })
      94 |
      95 |     it('should render the proposal modal when there is no queue for guardians', () => {

      at Object.toBeTruthy (src/components/recovery/RecoveryModal/index.test.tsx:92:59)

  ● RecoveryModal › component › should close the modal when the user navigates away

    expect(received).toBeTruthy()

    Received: null

      143 |
      144 |       expect(queryByText('Test')).toBeTruthy()
    > 145 |       expect(queryByText('Account recovery in progress')).toBeTruthy()
          |                                                           ^
      146 |
      147 |       // Trigger the route change
      148 |       mockUseRouter.events.on.mock.calls[0][1]()

      at Object.toBeTruthy (src/components/recovery/RecoveryModal/index.test.tsx:145:59)

  ● RecoveryHeader › should render the in-progress widget if there is a queue for guardians

    expect(received).toBeTruthy()

    Received: null

      29 |     )
      30 |
    > 31 |     expect(queryByText('Account recovery in progress')).toBeTruthy()
         |                                                         ^
      32 |   })
      33 |
      34 |   it('should render the in-progress widget if there is a queue for owners', () => {

      at Object.toBeTruthy (src/components/dashboard/RecoveryHeader/index.test.tsx:31:57)

  ● RecoveryHeader › should render the in-progress widget if there is a queue for owners

    expect(received).toBeTruthy()

    Received: null

      42 |     )
      43 |
    > 44 |     expect(queryByText('Account recovery in progress')).toBeTruthy()
         |                                                         ^
      45 |   })
      46 |
      47 |   it('should render the proposal widget when there is no queue for guardians', () => {

      at Object.toBeTruthy (src/components/dashboard/RecoveryHeader/index.test.tsx:44:57)

Report generated by 🧪jest coverage report action from fdeb566

github-actions · 2023-11-22T11:40:41Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

katspaugh · 2023-11-23T10:28:14Z

src/components/tx-flow/flows/UpsertRecovery/index.tsx

+export enum UpsertRecoveryFlowFields {
+  guardian = 'guardian',
+  txCooldown = 'txCooldown',
+  txExpiration = 'txExpiration',
+  emailAddress = 'emailAddress',
+}


Are these field names arbitrary or they actually map to ABI params?

All but guardian map to the ABI.

It might make sense to move that to services/recovery then. Or somehow import the ABI here.

These are only relevant for the fields in this flow though.

schmanu · 2023-11-23T10:22:03Z

src/components/settings/Recovery/DelayModifierRow.tsx

+
+  const onEdit = () => {
+    // TODO: Display flow
+    setTxFlow(undefined)


I think you forgot to add the UpsertRecoveryFlow here?

Indeed, good catch. I've added it in 09cad6f.

schmanu · 2023-11-23T10:22:29Z

src/components/settings/Recovery/index.tsx

@@ -68,13 +67,16 @@ const headCells = [
  { id: HeadCells.Actions, label: '', sticky: true },
 ]

+// TODO: Migrate section


Is this still valid?

Yes, the settings need to be merged with spending limits. I've clarified the comment in 09cad6f.

schmanu · 2023-11-23T10:33:42Z

src/services/recovery/setup.ts

+    txData.push(enableModule)
+
+    // Add guardian to cache
+    _guardians.push(guardianToAdd)


Do we actually need to add these to the cache?
The guardiansToAdd addresses can not be part of the guardiansToRemove. So they will never be relevant for removing a guardian.

True, I've removed it in 09cad6f.

github-actions · 2023-11-23T11:00:55Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

iamacook added 2 commits November 20, 2023 16:43

feat: edit recovery flow

8baedf9

fix: edit initial recovery module

8a316e9

iamacook requested a review from katspaugh November 20, 2023 15:52

iamacook linked an issue Nov 20, 2023 that may be closed by this pull request

[Recovery] Edit module flow #2761

Closed

2 tasks

iamacook added the recovery label Nov 21, 2023

iamacook added 3 commits November 21, 2023 10:56

Merge branch 'recovery-epic' into edit-recovery-flow

c705999

Merge branch 'recovery-epic' into edit-recovery-flow

5370666

Merge branch 'recovery-epic' into edit-recovery-flow

e90ffa6

iamacook added 4 commits November 22, 2023 18:28

Merge branch 'recovery-epic' into edit-recovery-flow

c9d7f56

fix: add guardian validation + test periods

3bffc0b

feat: add recovery delay tooltip

6dcfc49

Merge branch 'recovery-epic' into edit-recovery-flow

338d61e

katspaugh reviewed Nov 23, 2023

View reviewed changes

schmanu reviewed Nov 23, 2023

View reviewed changes

fix: set flow, improve comment + remove caching

09cad6f

iamacook requested a review from schmanu November 23, 2023 10:43

Merge branch 'recovery-epic' into edit-recovery-flow

fdeb566

iamacook requested a review from katspaugh November 23, 2023 10:58

Merge branch 'recovery-epic' into edit-recovery-flow

cb4e3bb

katspaugh approved these changes Nov 23, 2023

View reviewed changes

fix: tests

0a253d5

iamacook merged commit 5cabe00 into recovery-epic Nov 23, 2023
4 of 12 checks passed

iamacook deleted the edit-recovery-flow branch November 23, 2023 11:28

github-actions bot locked and limited conversation to collaborators Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: edit recovery flow #2824

feat: edit recovery flow #2824

iamacook commented Nov 20, 2023

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

katspaugh Nov 23, 2023

iamacook Nov 23, 2023

katspaugh Nov 23, 2023 •

edited

Loading

iamacook Nov 23, 2023

schmanu Nov 23, 2023

iamacook Nov 23, 2023

schmanu Nov 23, 2023

iamacook Nov 23, 2023

schmanu Nov 23, 2023

iamacook Nov 23, 2023

github-actions bot commented Nov 23, 2023

feat: edit recovery flow #2824

feat: edit recovery flow #2824

Conversation

iamacook commented Nov 20, 2023

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

github-actions bot commented Nov 20, 2023 • edited Loading

Branch preview

github-actions bot commented Nov 20, 2023 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Nov 20, 2023 • edited Loading

Coverage report

Test suite run failed

github-actions bot commented Nov 22, 2023 • edited Loading

ESLint Summary View Full Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2023

ESLint Summary View Full Report

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

katspaugh Nov 23, 2023 •

edited

Loading