feat: settings security #462

fbwoolf · 2024-09-19T16:57:39Z

This PR wires up the security settings flows, and:

Separates settings read/write api to be exposed thru useSettings in settings.ts
Creates a UI library component for Switch (note, I didn't replace the one in Cell bc it will be removed)

cc @mica000 We will need final text for Description here on both sheets. Also, is App authorization the right naming here? I'm not sure I even understood what it meant and the toast says metrics?

pete-watters

Nice work 🚀

kyranjamie

Great work @fbwoolf

Lots of comments, pretty much all naming related

kyranjamie · 2024-09-20T08:05:58Z

apps/mobile/src/store/settings/settings.ts

+
+export const defaultNetworks = ['mainnet', 'testnet', 'signet'] as const;
+
+export const defaultThemes = ['light', 'dark', 'system'] as const;


Been thinking on this naming, because system isn't really a theme, it's a setting that points to a theme.

Maybe we have ThemeSetting/ThemePreference and Theme?

Agree, been struggling a bit with some of the naming so thx for the feedback on all comments. I'll refactor them here.

kyranjamie · 2024-09-20T08:07:50Z

apps/mobile/src/store/settings/settings.ts

+  walletSecurityLevel: 'undefined',
+};
+
+export function useSettings() {


Thanks for the refactor. Think this is a really nice pattern where we expose all store read/writes via a uniform hook, abstracting any store-specific implementation details.

Will follow up with a depcruiser rule to prevent components reading .{read,write}.ts files

apps/mobile/src/store/settings/settings.ts

kyranjamie · 2024-09-20T08:10:32Z

packages/ui/src/components/switch/switch.native.tsx

+  onToggleValue?(): void;
+  value: boolean;
+}
+export function Switch({ onToggleValue, value, ...props }: SwitchProps) {


Nice, think Pete made an issue for this we can close

kyranjamie · 2024-09-20T08:11:13Z

apps/mobile/src/store/settings/settings.ts

+export const initialState: SettingsState = {
+  accountDisplayPreference: 'ns',
+  analyticsEnabled: false,
+  appAuthEnabled: false,


Can't this be inferred from walletSecurityLevel?

I need clarification on what this setting is for the from design team. I am unclear what it is, the toast says Metrics updated. 🤔 cc @mica000 Can someone clarify for me what walletSecurityLevel is for?

cc/ @edgarkhanzadian

It does appear this is used with biometrics true/false, so I'll remove appAuthEnabled bc it seems the same.

Just registering that the copy should be App authentication updated.

apps/mobile/src/store/settings/settings.ts

kyranjamie · 2024-09-20T08:15:36Z

apps/mobile/src/app/wallet/settings/security/security-call.tsx

+    <TouchableOpacity onPress={onCreateSheetRef}>
+      <Flag img={<Avatar>{icon}</Avatar>}>
+        <ItemLayout
+          actionIcon={<ChevronRightIcon variant="small" />}
+          captionLeft={caption}
+          titleLeft={title}
+        />
+      </Flag>
+    </TouchableOpacity>


I feel like many would look at this, see the repetition between this and the other Cells, and have to fight the urge not to try refactor into a more terse JSX component.

But this would be a mistake IMO. This is just the right composition where we make no assumptions about the content and can change any cell to any layout requirement 👍🏼

apps/mobile/src/store/settings/settings.ts

apps/mobile/src/app/wallet/settings/security/security-call.tsx

codecov · 2024-09-20T20:43:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.38%. Comparing base (e8691eb) to head (f90d96c).
Report is 1 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #462   +/-   ##
=======================================
  Coverage   22.38%   22.38%           
=======================================
  Files         132      132           
  Lines        5521     5521           
  Branches      241      241           
=======================================
  Hits         1236     1236           
  Misses       4285     4285

Components	Coverage Δ
bitcoin	`53.77% <ø> (ø)`
query	`12.05% <ø> (ø)`
utils	`52.45% <ø> (ø)`
crypto	`69.40% <ø> (ø)`
stacks	`53.27% <ø> (ø)`

fbwoolf · 2024-09-23T15:35:26Z

@kyranjamie thoughts on renaming here, I went in a direction where I coordinated all settings to be ...Preference so when encountered we know it is associated to a preferred setting chosen by the user.

leather-bot · 2024-09-23T15:43:51Z

Expo simulator build link: https://expo.dev/accounts/leather-wallet/projects/leather-wallet-mobile/builds/0fc8b092-40c6-4c2a-b8f4-51caa89742e5

fbwoolf · 2024-09-23T15:45:44Z

@markmhendrickson can you confirm, is the app authentication turning on/off using biometrics? I am still a little unclear if I am interpreting this correctly?

markmhendrickson · 2024-09-23T17:00:15Z

Indeed, app authentication == whether the user needs to use device-level authentication (e.g. biometrics like Face ID or PIN as backup) to perform certain actions. If they do, they'll see the unlock view each time: https://github.com/leather-io/issues/issues/160

These are the intended triggers / actions that require unlock if enabled.

fbwoolf · 2024-09-23T17:12:24Z

Indeed, app authentication == whether the user needs to use device-level authentication (e.g. biometrics like Face ID or PIN as backup) to perform certain actions. If they do, they'll see the unlock view each time: leather-io/issues#160

These are the intended triggers / actions that require unlock if enabled.

Got it, thanks for clarifying. 👍

fbwoolf · 2024-09-23T23:01:32Z

@edgarkhanzadian where is the code that triggers the face id to popup? I see biometrics being passed around, but I'm not understanding what I can use to trigger it if the user toggles app auth on/off?

leather-bot · 2024-09-23T23:18:50Z

Expo simulator build link: https://expo.dev/accounts/leather-wallet/projects/leather-wallet-mobile/builds/64550d6e-3be2-42e0-b958-443c5aa60fd9

leather-bot · 2024-09-24T01:09:52Z

Expo simulator build link: https://expo.dev/accounts/leather-wallet/projects/leather-wallet-mobile/builds/24a7fc50-3946-4005-8ad4-2ca9eb01dd79

kyranjamie · 2024-09-24T08:55:03Z

apps/mobile/src/features/settings/app-authentication-sheet.tsx

+  useEffect(() => {
+    if (settings.securityLevelPreference === 'undefined')
+      settings.changeSecurityLevelPreference('insecure');
+  });


Mmm, this seems weird? Why wouldn't we just set the preference as insecure in the store if it it changed automatically when landing on this page?

Yeah, I'll remove. Prob not needed but was asking for clarification on this setting from Edgar. I'm not sure why undefined is even needed in the type?

leather-bot · 2024-09-24T19:48:50Z

Expo simulator build link: https://expo.dev/accounts/leather-wallet/projects/leather-wallet-mobile/builds/b9385ab1-ebba-4f72-a8ce-30c6acab6f42

fbwoolf requested review from kyranjamie, edgarkhanzadian and pete-watters September 19, 2024 16:57

pete-watters approved these changes Sep 20, 2024

View reviewed changes

kyranjamie reviewed Sep 20, 2024

View reviewed changes

pete-watters reviewed Sep 20, 2024

View reviewed changes

apps/mobile/src/app/wallet/settings/security/security-call.tsx Outdated Show resolved Hide resolved

fbwoolf force-pushed the feat/settings-security branch 2 times, most recently from 0936192 to df67bca Compare September 20, 2024 20:41

fbwoolf force-pushed the feat/settings-security branch from df67bca to 836a9dc Compare September 20, 2024 20:45

edgarkhanzadian added the needs:demo-build Create EAS simulator build based on label label Sep 23, 2024

fbwoolf force-pushed the feat/settings-security branch 2 times, most recently from bbfbb97 to 1e04613 Compare September 23, 2024 15:30

fbwoolf requested a review from kyranjamie September 23, 2024 15:33

fbwoolf force-pushed the feat/settings-security branch from 1e04613 to 922851f Compare September 23, 2024 22:57

fbwoolf force-pushed the feat/settings-security branch from 922851f to 900a9e4 Compare September 24, 2024 00:49

kyranjamie reviewed Sep 24, 2024

View reviewed changes

kyranjamie approved these changes Sep 24, 2024

View reviewed changes

fbwoolf force-pushed the feat/settings-security branch from 900a9e4 to 6147253 Compare September 24, 2024 18:24

feat: settings security, closes leather-io/issues#304

f90d96c

fbwoolf force-pushed the feat/settings-security branch from 6147253 to f90d96c Compare September 24, 2024 19:27

fbwoolf added this pull request to the merge queue Sep 24, 2024

Merged via the queue into dev with commit 936fa26 Sep 24, 2024
12 checks passed

fbwoolf deleted the feat/settings-security branch September 24, 2024 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: settings security #462

feat: settings security #462

fbwoolf commented Sep 19, 2024

pete-watters left a comment

kyranjamie left a comment

kyranjamie Sep 20, 2024

fbwoolf Sep 20, 2024

kyranjamie Sep 20, 2024

kyranjamie Sep 20, 2024

kyranjamie Sep 20, 2024

fbwoolf Sep 20, 2024

kyranjamie Sep 20, 2024

fbwoolf Sep 20, 2024

mica000 Sep 20, 2024 •

edited

Loading

kyranjamie Sep 20, 2024

codecov bot commented Sep 20, 2024 •

edited

Loading

fbwoolf commented Sep 23, 2024 •

edited

Loading

leather-bot commented Sep 23, 2024

fbwoolf commented Sep 23, 2024

markmhendrickson commented Sep 23, 2024

fbwoolf commented Sep 23, 2024

fbwoolf commented Sep 23, 2024

leather-bot commented Sep 23, 2024

leather-bot commented Sep 24, 2024

kyranjamie Sep 24, 2024

fbwoolf Sep 24, 2024

leather-bot commented Sep 24, 2024


		export const defaultNetworks = ['mainnet', 'testnet', 'signet'] as const;

		export const defaultThemes = ['light', 'dark', 'system'] as const;

feat: settings security #462

feat: settings security #462

Conversation

fbwoolf commented Sep 19, 2024

pete-watters left a comment

Choose a reason for hiding this comment

kyranjamie left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mica000 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2024 • edited Loading

Codecov Report

fbwoolf commented Sep 23, 2024 • edited Loading

leather-bot commented Sep 23, 2024

fbwoolf commented Sep 23, 2024

markmhendrickson commented Sep 23, 2024

fbwoolf commented Sep 23, 2024

fbwoolf commented Sep 23, 2024

leather-bot commented Sep 23, 2024

leather-bot commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leather-bot commented Sep 24, 2024

mica000 Sep 20, 2024 •

edited

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading

fbwoolf commented Sep 23, 2024 •

edited

Loading