fix(extension): [lw-10539] shared wallet settings section #1231

vetalcore · 2024-06-19T17:23:36Z

Checklist

JIRA - LW-10539
Proper tests implemented
Screenshots added.

Proposed solution

Explain how does this PR solves the problem stated in JIRA ticket.
You can also enumerate different alternatives considered while approaching this task.

Testing

Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met

Screenshots

Attach screenshots here if implementation involves some UI changes

vetalcore · 2024-06-19T17:27:07Z

...extension-wallet/src/views/browser-view/features/settings/components/utils/isSharedWallet.ts

+const isValidSharedWalletScript = (script: Cardano.NativeScript): boolean => {
+  switch (script.kind) {
+    case Cardano.NativeScriptKind.RequireAllOf:
+    case Cardano.NativeScriptKind.RequireAnyOf:
+    case Cardano.NativeScriptKind.RequireNOf:
+      return script.scripts.every((nativeScript) => nativeScript.kind === Cardano.NativeScriptKind.RequireSignature);
+    default:
+      return false;
+  }
+};


this one exists but not exported from cardano-js-sdk*
could create a pr to export it if required?
@szymonmaslowski wdyt?

Yeah, why not! I guess exporting this utility from sdk will be pretty quick and then we could update the sdk package version in this PR. Thanks for spotting that!

pr is merged now
what is the release process in the sdk package?

cardano js sdk is bumped now

vetalcore · 2024-06-19T17:29:11Z

...extension-wallet/src/views/browser-view/features/settings/components/utils/isSharedWallet.ts

+export const useIsSharedWallet = (inMemoryWallet: Wallet.ObservableWallet): boolean => {
+  const walletAddresses = useObservable(inMemoryWallet.addresses$);
+
+  return walletAddresses?.some(
+    (addr) =>
+      isScriptAddress(addr) &&
+      Cardano.isNativeScript(addr.scripts.payment) &&
+      isValidSharedWalletScript(addr.scripts.payment) &&
+      Cardano.isNativeScript(addr.scripts.stake) &&
+      isValidSharedWalletScript(addr.scripts.stake)
+  );
+};


not sure this is a proper way to check if current wallet is shared, i've created shared wallet but there are no scripts found in the addresses observable
@szymonmaslowski could you please suggest?

Try the following way: (I didn't try it, just scratched this code)

const { walletId } = useObservable(walletManager.activeWalletId$); const wallets = useObservable(walletRepository.wallets$); const { paymentScript, stakingScript } = wallets.find((w) => w.walletId === walletId)

done here ddcf042

vetalcore · 2024-06-19T17:35:08Z

...extension-wallet/src/views/browser-view/features/settings/components/utils/isSharedWallet.ts

@@ -0,0 +1,28 @@
+import { Cardano } from '@cardano-sdk/core';


should it live in the share-wallet package?
also there are isHardwareWallet and isInMemoryWallet exported from the walletInfoSlice, would it be a proper place for isSaredWallet as well?

I think it is a good idea to have it in the walletInfoSlice along with isHardwareWallet and isInMemoryWallet 👍

done here ddcf042

github-actions · 2024-06-19T17:55:11Z

Allure Report

allure-report-publisher generated test report!

smokeTests: ✅ test report for 767d8e2f

	passed	failed	skipped	flaky	total	result
Total	33	0	0	0	33	✅

szymonmaslowski

GJ!

szymonmaslowski · 2024-06-20T07:35:47Z

...extension-wallet/src/views/browser-view/features/settings/components/utils/isSharedWallet.ts

@@ -0,0 +1,28 @@
+import { Cardano } from '@cardano-sdk/core';


I think it is a good idea to have it in the walletInfoSlice along with isHardwareWallet and isInMemoryWallet 👍

szymonmaslowski · 2024-06-20T07:38:18Z

...extension-wallet/src/views/browser-view/features/settings/components/utils/isSharedWallet.ts

+const isValidSharedWalletScript = (script: Cardano.NativeScript): boolean => {
+  switch (script.kind) {
+    case Cardano.NativeScriptKind.RequireAllOf:
+    case Cardano.NativeScriptKind.RequireAnyOf:
+    case Cardano.NativeScriptKind.RequireNOf:
+      return script.scripts.every((nativeScript) => nativeScript.kind === Cardano.NativeScriptKind.RequireSignature);
+    default:
+      return false;
+  }
+};


Yeah, why not! I guess exporting this utility from sdk will be pretty quick and then we could update the sdk package version in this PR. Thanks for spotting that!

szymonmaslowski · 2024-06-20T07:57:35Z

...extension-wallet/src/views/browser-view/features/settings/components/utils/isSharedWallet.ts

+export const useIsSharedWallet = (inMemoryWallet: Wallet.ObservableWallet): boolean => {
+  const walletAddresses = useObservable(inMemoryWallet.addresses$);
+
+  return walletAddresses?.some(
+    (addr) =>
+      isScriptAddress(addr) &&
+      Cardano.isNativeScript(addr.scripts.payment) &&
+      isValidSharedWalletScript(addr.scripts.payment) &&
+      Cardano.isNativeScript(addr.scripts.stake) &&
+      isValidSharedWalletScript(addr.scripts.stake)
+  );
+};


Try the following way: (I didn't try it, just scratched this code)

const { walletId } = useObservable(walletManager.activeWalletId$); const wallets = useObservable(walletRepository.wallets$); const { paymentScript, stakingScript } = wallets.find((w) => w.walletId === walletId)

vetalcore · 2024-06-20T16:49:50Z

apps/browser-extension-wallet/src/utils/is-shared-wallet.ts

+import { Wallet } from '@lace/cardano';
+
+export const isSharedWallet = (wallet?: Wallet.CardanoWallet): boolean => {
+  if (wallet?.source.wallet.type !== WalletType.Script) return false;


wallet?.source.wallet.type gives inMemory for me
@szymonmaslowski are we actually creating a shared wallet?
if no - how are we going to pass it to the sdet?

szymonmaslowski

Great Job! This one anyway will need to wait for the changes regarding shared-wallet instance creation. Not approving yet just because of changes in the SharedWalletGetStarted.tsx, they should be ready to be removed soon.

szymonmaslowski · 2024-06-24T11:04:06Z

...llets/src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStarted.tsx

@@ -21,12 +21,12 @@ export type SharedWalletGetStartedSharedProps = {
 type SharedWalletGetStartedProps = SharedWalletGetStartedSharedProps &
  (
    | {
-        createAndImportOptionsDisabled: true;
+        createAndImportOptionsDisabled: boolean;


I guess you changed it because there was an build error of the shared-wallet package, right? That is my mistake, somehow it sneaked into the main. The error is fixed here, let's revert the changes in this file.

done here 3a39e8a

danielmain

Great job @vetalcore

szymonmaslowski

GJ! I left one suggestion for later. Please also take a look on sonar cube report.

szymonmaslowski · 2024-07-10T11:35:53Z

apps/browser-extension-wallet/src/utils/is-shared-wallet.ts

+import { WalletType } from '@cardano-sdk/web-extension';
+import { Wallet } from '@lace/cardano';
+
+export const isSharedWallet = (wallet?: Wallet.CardanoWallet): boolean => {


I would move it to the shared-wallets space. Let's do it after merging this PR 😉

sonarqubecloud · 2024-07-12T13:30:08Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

vetalcore self-assigned this Jun 19, 2024

vetalcore requested a review from a team as a code owner June 19, 2024 17:23

vetalcore force-pushed the feat/lw-10539-shared-wallet-settings-section branch from bdcb925 to f5f9810 Compare June 19, 2024 17:25

vetalcore commented Jun 19, 2024

View reviewed changes

szymonmaslowski reviewed Jun 20, 2024

View reviewed changes

vetalcore force-pushed the feat/lw-10539-shared-wallet-settings-section branch 2 times, most recently from 99fbcb4 to 241ffdf Compare June 20, 2024 16:48

vetalcore requested a review from a team as a code owner June 20, 2024 16:48

vetalcore commented Jun 20, 2024

View reviewed changes

szymonmaslowski reviewed Jun 24, 2024

View reviewed changes

vetalcore force-pushed the feat/lw-10539-shared-wallet-settings-section branch from 241ffdf to f0e811d Compare July 8, 2024 08:11

danielmain approved these changes Jul 10, 2024

View reviewed changes

szymonmaslowski approved these changes Jul 10, 2024

View reviewed changes

vetalcore requested a review from szymonmaslowski July 10, 2024 13:08

vetalcore force-pushed the feat/lw-10539-shared-wallet-settings-section branch 2 times, most recently from e9bb448 to c36b7ee Compare July 11, 2024 04:40

vetalcore added 4 commits July 12, 2024 16:07

fix(extension): [lw-10539] shared wallet settings section

197cb2a

fix(extension): [lw-10539] add isSharedWallet key on wallet slive level

2ba9f53

chore: bump cardano-js-sdk

7acd33b

fix(extension): resolve pr comments

767d8e2

vetalcore force-pushed the feat/lw-10539-shared-wallet-settings-section branch from c36b7ee to 767d8e2 Compare July 12, 2024 13:28

vetalcore enabled auto-merge (squash) July 12, 2024 13:31

vetalcore merged commit 5796e23 into main Jul 12, 2024
27 of 29 checks passed

vetalcore deleted the feat/lw-10539-shared-wallet-settings-section branch July 12, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(extension): [lw-10539] shared wallet settings section #1231

fix(extension): [lw-10539] shared wallet settings section #1231

vetalcore commented Jun 19, 2024 •

edited

Loading

vetalcore Jun 19, 2024

szymonmaslowski Jun 20, 2024

vetalcore Jun 20, 2024 •

edited

Loading

vetalcore Jun 20, 2024

vetalcore Jun 19, 2024

szymonmaslowski Jun 20, 2024

vetalcore Jun 20, 2024

vetalcore Jun 19, 2024

szymonmaslowski Jun 20, 2024

vetalcore Jun 20, 2024

github-actions bot commented Jun 19, 2024 •

edited by pczeglik-iohk

Loading

szymonmaslowski left a comment

szymonmaslowski Jun 20, 2024

szymonmaslowski Jun 20, 2024

szymonmaslowski Jun 20, 2024

vetalcore Jun 20, 2024

szymonmaslowski left a comment

szymonmaslowski Jun 24, 2024

vetalcore Jul 10, 2024

danielmain left a comment

szymonmaslowski left a comment

szymonmaslowski Jul 10, 2024

sonarqubecloud bot commented Jul 12, 2024

		@@ -0,0 +1,28 @@
		import { Cardano } from '@cardano-sdk/core';

fix(extension): [lw-10539] shared wallet settings section #1231

fix(extension): [lw-10539] shared wallet settings section #1231

Conversation

vetalcore commented Jun 19, 2024 • edited Loading

Checklist

Proposed solution

Testing

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vetalcore Jun 20, 2024 • 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 Jun 19, 2024 • edited by pczeglik-iohk Loading

Allure Report

szymonmaslowski 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

szymonmaslowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmain left a comment

Choose a reason for hiding this comment

szymonmaslowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 12, 2024

Quality Gate passed

vetalcore commented Jun 19, 2024 •

edited

Loading

vetalcore Jun 20, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024 •

edited by pczeglik-iohk

Loading