Hide dangerous operations behind configuration option #1025

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

Jump to bottom

Draft

csillag wants to merge 4 commits into master

from csillag/danger-mode

Draft

Hide dangerous operations behind configuration option #1025

csillag wants to merge 4 commits into master from csillag/danger-mode

Conversation

@csillag

Copy link

Contributor

@csillag csillag commented Sep 24, 2022

This PR adds the following things:

Implement a settings dialog, accessible from the sidebar, currently with a single setting: the "danger mode". (For background, see here)
dialog
Updates the behavior of the modal dialogs, when the isDangerous flag is specified. The new behavior is as follows:

If an action is indicated as dangerous, but "danger mode" is off (the default), then the operation will be refused, and the user is referred to the settings.
If "danger mode" is enabled, then it's possible to execute the action, but there is still a strong warning message, and a mandatory waiting period.

modal

@csillag csillag requested a review from buberdds

September 24, 2022 00:07

@github-actions

Copy link

github-actions bot commented Sep 24, 2022 •

edited

Loading

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ EDITORCONFIG	editorconfig-checker	20	0	0.04s
✅ JSON	eslint-plugin-jsonc	1	0	0	0.87s
✅ JSON	jsonlint	1	0	0.2s
✅ JSON	prettier	1	0	0	0.51s
✅ JSON	v8r	1	0	1.75s
✅ REPOSITORY	checkov	yes	no	11.46s
✅ REPOSITORY	git_diff	yes	no	0.0s
✅ TSX	eslint	8	0	0	5.8s
✅ TYPESCRIPT	eslint	9	0	0	4.51s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@kostko

Copy link

Member

kostko commented Sep 24, 2022

Task linked: CU-3ggrpj8 Implement configuration option for dangerous actions

@csillag csillag force-pushed the csillag/danger-mode branch from 821766f to 3838d62 Compare

September 24, 2022 00:16

@codecov

Copy link

codecov bot commented Sep 24, 2022 •

edited

Loading

Codecov Report

Merging #1025 (8f4da1e) into master (0e7cab6) will decrease coverage by 1.54%.
The diff coverage is 52.08%.

Impacted file tree graph

@@ Coverage Diff @@
## master #1025 +/- ##
==========================================
- Coverage 88.75% 87.20% -1.55% 
==========================================
 Files 102 108 +6 
 Lines 1778 1852 +74 
 Branches 411 436 +25 
==========================================
+ Hits 1578 1615 +37 
- Misses 200 237 +37

Flag	Coverage Δ
cypress	`59.55% <40.62%> (-1.03%)`	⬇️
jest	`77.81% <52.08%> (-1.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/app/components/Sidebar/index.tsx	`88.75% <ø> (ø)`
src/app/index.tsx	`100.00% <ø> (ø)`
src/store/reducers.ts	`100.00% <ø> (ø)`
src/app/components/SettingsDialog/index.tsx	`12.50% <12.50%> (ø)`
src/app/components/Modal/slice/index.ts	`30.00% <30.00%> (ø)`
src/app/components/SettingsDialog/slice/index.ts	`42.85% <42.85%> (ø)`
src/app/components/Modal/index.tsx	`58.13% <57.14%> (-21.87%)`	⬇️
src/app/components/SettingsButton/index.tsx	`72.72% <72.72%> (ø)`
src/app/components/AddEscrowForm/index.tsx	`92.30% <100.00%> (ø)`
src/app/components/Modal/slice/selectors.ts	`100.00% <100.00%> (ø)`
... and 5 more

@csillag

Copy link

Contributor Author

csillag commented Sep 24, 2022

Obviously, some tests need to be added, but first I wanted to get some feedback on the basics.

lukaw3d

lukaw3d reviewed

Sep 27, 2022

View reviewed changes

Copy link

Member

@lukaw3d lukaw3d left a comment

Choose a reason for hiding this comment

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

I love the UX flow

there's instructions on why confirm button is missing
opening settings as modal doesn't destroy the unfinished transaction
there's wait time on the button

src/app/components/SettingsDialog/index.tsx Outdated Show resolved Hide resolved

src/app/components/Modal/index.tsx Outdated Show resolved Hide resolved

@csillag csillag force-pushed the csillag/danger-mode branch from 3838d62 to cdf1ccb Compare

September 27, 2022 13:54

@csillag

Copy link

Contributor Author

csillag commented Sep 27, 2022

@lukaw3d I have addressed all the feedback. Thanks.

@tjanez tjanez marked this pull request as draft

September 27, 2022 14:06

@csillag csillag mentioned this pull request

Sep 27, 2022

Add support for burning tokens #1035

Draft

@csillag csillag added the blocked label

Sep 28, 2022

@csillag csillag force-pushed the csillag/danger-mode branch 2 times, most recently from 109cd65 to d1f488e Compare

October 19, 2022 22:21

@csillag

Copy link

Contributor Author

csillag commented Oct 19, 2022 •

edited

Loading

We have discussed the design with @donouwens. His main feedback was that it would be nice if the settings dialog could be opened directly from within the dangerous modal dialogs. However, @lukaw3d has explained that Grommet has an obscure issue with pop-ups being opened from pop-ups.

So I have created an implementation which supports opening the settings dialog, but in a way that first it closes the existing modal, then it brings up the settings dialog, and when we close that, it brings up the original modal dialog again.

I'm not fully satisfied with the implementation, because I feel that the logic and APIs for dealing with the modal are now so complex that they should be implemented in a redux store, via proper actions, instead of locally managed react hooks state, passed along in a react context.. I will refactor that after we have confirmed that this behavior is what we want from the UI/UX POV.

@csillag csillag removed the blocked label

Oct 19, 2022

@lukaw3d

Copy link

Member

lukaw3d commented Oct 19, 2022

popup in popup issue reference: #863

csillag and others added 4 commits

October 20, 2022 13:28

@csillag


 Add a settings dialog, with "dangerous mode" setting

04094eb

Also, in modals, improve handling of danger, and add delays.

@csillag


 Fix Grommet icon import

72cb2c4

@csillag


 Support opening the settings dialog directly from dangerous modal dia...

964bf8f

...logs

@csillag


 Refactor Modal so that

8f4da1e

 - The state is handled in Redux
 - No more ModalProvider
 - pop-up in pop-up is now supported (recursively)

@csillag csillag force-pushed the csillag/danger-mode branch from 39da44e to 8f4da1e Compare

October 20, 2022 11:31

@csillag

Copy link

Contributor Author

csillag commented Oct 20, 2022

I'm not fully satisfied with the implementation, because I feel that the logic and APIs for dealing with the modal are now so complex that they should be implemented in a redux store, via proper actions, instead of locally managed react hooks state, passed along in a react context..

I have refactored Modal so that

The state is stored in Redux
It now be opened recursively. (When opening a new one, we always close the previous one and stash it, to be re-opened later.)

I like it better now, however there is one drawback: the handleConfirm functions (which are part of the modal definition) are now also stored in the Redux store. Redux doesn't like non-serializable data in the store, so I had to add specific instructions to make it stop complaining.

@lukaw3d

Copy link

Member

lukaw3d commented Oct 24, 2022

one drawback: the handleConfirm functions (which are part of the modal definition) are now also stored in the Redux store. Redux doesn't like non-serializable data in the store, so I had to add specific instructions to make it stop complaining.

This will probably cause problems with redux-saga, webext-redux, redux-state-sync, or future libraries we may want. I don't think this simple settings shortcut is worth breaking redux's best practice

@lukaw3d

Copy link

Member

lukaw3d commented Oct 24, 2022 •

edited

Loading

To reduce complexity I'd rather have any of:

remove settings shortcut
or ignore accessibility bug for this rare feature
or fix accessibility bug upstream

@csillag csillag mentioned this pull request

Feb 2, 2023

Unable to withdraw to arbitrary consensus account oasisprotocol/oasis-wallet-ext#300

Open

Labels

None yet

4 participants

@csillag @kostko @lukaw3d

Hide dangerous operations behind configuration option #1025

Are you sure you want to change the base?

Hide dangerous operations behind configuration option #1025

Uh oh!

Conversation

@csillag csillag commented Sep 24, 2022

Uh oh!

github-actions bot commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦙 MegaLinter status: ✅ SUCCESS

Uh oh!

kostko commented Sep 24, 2022

Uh oh!

codecov bot commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

csillag commented Sep 24, 2022

Uh oh!

@lukaw3d lukaw3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csillag commented Sep 27, 2022

Uh oh!

csillag commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaw3d commented Oct 19, 2022

Uh oh!

csillag commented Oct 20, 2022

Uh oh!

lukaw3d commented Oct 24, 2022

Uh oh!

lukaw3d commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Sep 24, 2022 •

edited

Loading

codecov bot commented Sep 24, 2022 •

edited

Loading

csillag commented Oct 19, 2022 •

edited

Loading

lukaw3d commented Oct 24, 2022 •

edited

Loading