Feature/2397 display guard information in settings #2662

juampibermani · 2021-08-19T15:35:49Z

What it solves

Resolves #2397

How this PR fixes it

We display a table containing the transaction guard information with a button to delete the transaction guard

Creating the PR as draft since i'm not sure how ModulePair works and how to set a transaction guard

How to test it

⚠️ Adding a guard could lock the safe if this isn't done correctly. Usually you would deploy a guard for yourself and have some handling mechanism. For the Guard we were provided for testing purposes, is owner by shared QA testing account, so can be handled by this account. This guard will prevent any operation within a Safe unless explicitly enabled in the Guard.

⚠️ In order to be able to remove the Guard you need to enable in the guard the possibility for the safe to execute transactions to itself.

1.- Make sure you have access to TestOwner key or check with Liliy
2.- Use the transaction builder to create a transaction using setGuard method of your safe configuring this safe Guard 0x4f8a82d73729A33E0165aDeF3450A7F85f007528
3.- After correctly enabling the guard test that now its showed under Settings -> Advanced
4.- Check that currently you can't execute any transactions with your safe. Try a send funds or any settings change and you won't be able to do anything
5.- When you are ready you can go here and add your safeAddress to "allowTarget" function and submit the transaction so it's stored in the Guard
6.- BONUS try to enable and disable some functions at different points in the guard and check that the safe behaves as expected. To remove the guard make sure that "allowTarget" contains your safeAddress and you will be able to remove the Guard

…tings

github-actions · 2021-08-19T15:36:06Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-08-19T15:39:47Z

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	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

…tings

juampibermani · 2021-08-24T16:15:54Z

Blocked by safe-global/safe-gateway-typescript-sdk#13

dasanra · 2021-08-26T09:07:16Z

@juampibermani published the updated library version, should be unbloked now
c49f2c0

github-actions · 2021-08-26T09:14:49Z

Safe Multisig app polygon

github-actions · 2021-08-26T09:16:38Z

Safe Multisig app mainnet

github-actions · 2021-08-26T09:16:48Z

Safe Multisig app rinkeby

github-actions · 2021-08-26T09:20:40Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1169832649

src/routes/safe/components/Settings/Advanced/RemoveGuardModal.tsx

katspaugh · 2021-08-26T09:21:58Z

src/routes/safe/components/Settings/Advanced/RemoveGuardModal.tsx

+        }}
+      </EditableTxParameters>
+    </Modal>
+  )


That's a lot of code duplication from the Review modal. For this PR it's fine but we should definitely think of a way to reuse the same code for tx popups.

src/routes/safe/components/Settings/Advanced/TransactionGuard.tsx

src/routes/safe/components/Settings/Advanced/index.tsx

src/logic/safe/utils/__tests__/shouldSafeStoreBeUpdated.test.ts

mmv08 · 2021-08-26T09:54:00Z

src/routes/safe/components/Settings/Advanced/index.tsx

+          .
+        </InfoText>
+
+        {!guard ? <NoTransactionGuardLegend /> : <TransactionGuard address={guard} />}


nitpick: I'd put NoTransactionGuardLegend inside the TransactionGuard component. If we keep implementing the same logic inside this component for each field we have, it might get dirty

I'll change this when we improve the reuse of components for modules and guard

src/routes/safe/components/Settings/Advanced/index.tsx

src/routes/safe/components/Settings/Advanced/RemoveGuardModal.tsx

mmv08 · 2021-08-26T10:01:55Z

src/routes/safe/components/Settings/Advanced/TransactionGuard.tsx

+  return (
+    <>
+      <TableContainer>
+        <Table columns={columns} data={[address]} defaultFixed disablePagination label="Modules" noBorder>


I know it was written on the ticket, but we don't really need a table there. It makes things complicated. There's only one transaction guard per safe, so it will always use 1 row.

This is not required, but you can think about how we can remove it if you want.

src/routes/safe/components/Settings/Advanced/TransactionGuard.tsx

github-actions · 2021-08-26T11:28:34Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1170213422

…ttps://github.com/gnosis/safe-react into feature/2397-display-guard-information-in-settings

github-actions · 2021-08-26T16:35:00Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1171228493

github-actions · 2021-08-27T14:25:51Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1174566162

github-actions · 2021-08-27T14:43:55Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1174623186

github-actions · 2021-08-27T14:53:57Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1174651866

francovenica · 2021-08-31T20:24:24Z

Safe and env :https://pr2662--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x6E94311ab6964E53aa319Dc106836fBE8aEB142D/transactions

According of what Dani told me, the scope of the ticket is make sure that the user is able to setUp the guard, that is registered as one (not as a random module) and that can be deleted.

We tested using the guard in the description 0x4f8a82d73729A33E0165aDeF3450A7F85f007528, which owner is this MM account 0xF2CeA96575d6b10f51d9aF3b10e3e4E5738aa6bd

Test steps done:
Created a safe
Enable the guard using the method "setGuard" using the ABI of the MasterCopy for 1.3.0. This guard in particular blocks any type of tx that is not directed to a whitelisted address.
As this point trying to delete the module will fail, since the address of the safe itself is not whitelisted
in Etherescan I used the owner of the guard and used the method "allowTarget" to set the safe itself.
With this the safe was able to delete the guard
I was able to enable the guard once again, now whitelisting a test contract 0x49d4450977E2c95362C13D3a31a09311E0Ea26A6
Tried to use that test contract to execute a dummy tx. It worked fine

Looks good to me

…tings

github-actions · 2021-09-02T15:01:22Z

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1194672825

juampibermani added 3 commits August 19, 2021 11:28

Added transaction guard to advanced settings

dda9501

Merge branch 'dev' into feature/2397-display-guard-information-in-set…

11e66f6

…tings

Get guard from the unified gateway

f45b4af

juampibermani added this to the 3.13.0 milestone Aug 19, 2021

juampibermani requested review from dasanra and katspaugh August 19, 2021 15:35

juampibermani added 6 commits August 19, 2021 13:10

Fixed tests

b0ab537

Added TransactionGuard component

2510e78

WIP: created component without contract instance

0a0966d

Merge branch 'dev' into feature/2397-display-guard-information-in-set…

b9d1184

…tings

Call safe method to set guard tx address

320187d

Deleted console log

5208e47

juampibermani marked this pull request as ready for review August 24, 2021 16:15

juampibermani added the Blocked 🤷 Blocked by external dependencies label Aug 24, 2021

juampibermani and others added 2 commits August 25, 2021 11:17

Added description to remove guard modal

c769dfd

Update gateway-sdk to v2.1.0

c49f2c0

katspaugh modified the milestones: 3.13.0, 3.14.0 Aug 26, 2021

katspaugh suggested changes Aug 26, 2021

View reviewed changes

katspaugh requested a review from mmv08 August 26, 2021 09:25

mmv08 reviewed Aug 26, 2021

View reviewed changes

Fix remove guard function

078d761

dasanra removed the Blocked 🤷 Blocked by external dependencies label Aug 26, 2021

katspaugh approved these changes Aug 26, 2021

View reviewed changes

juampibermani added 2 commits August 26, 2021 10:58

Minor fixes

e77e8dc

Merge branch 'feature/2397-display-guard-information-in-settings' of h…

cee6eb0

…ttps://github.com/gnosis/safe-react into feature/2397-display-guard-information-in-settings

Replace guard if fetchedSafe has a null guard

6819f3c

juampibermani added 2 commits August 27, 2021 11:28

Fixed some tests

fae9148

Fixed test

8377a06

Merge branch 'dev' into feature/2397-display-guard-information-in-set…

f2af702

…tings

katspaugh merged commit a2d229d into dev Sep 13, 2021

katspaugh deleted the feature/2397-display-guard-information-in-settings branch September 13, 2021 09:13

github-actions bot locked and limited conversation to collaborators Sep 13, 2021

Feature/2397 display guard information in settings #2662

Feature/2397 display guard information in settings #2662

Uh oh!

Conversation

juampibermani commented Aug 19, 2021 • edited by dasanra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it solves

How this PR fixes it

How to test it

Uh oh!

github-actions bot commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

juampibermani commented Aug 24, 2021

Uh oh!

dasanra commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 26, 2021

Uh oh!

Uh oh!

Uh oh!

katspaugh Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmv08 Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

juampibermani Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmv08 Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 26, 2021

Uh oh!

github-actions bot commented Aug 27, 2021

Uh oh!

github-actions bot commented Aug 27, 2021

Uh oh!

github-actions bot commented Aug 27, 2021

Uh oh!

francovenica commented Aug 31, 2021

Uh oh!

github-actions bot commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

juampibermani commented Aug 19, 2021 •

edited by dasanra

Loading

github-actions bot commented Aug 19, 2021 •

edited

Loading

github-actions bot commented Aug 19, 2021 •

edited

Loading