Skip to content
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

[LW-9813] Create posthog alias after new wallet is restored #906

Conversation

lucas-barros
Copy link

@lucas-barros lucas-barros commented Feb 21, 2024

Checklist


Proposed solution

  • Merges posthog users with multiple wallets installed.
  • Fixes issues with hardware wallet not aliasing posthog user id due to window reloading

Testing

Posthog user aliasing

  • Create/restore wallet A in one lace installation
  • Create/restore wallet B in another lace installation
  • Restore wallet B in wallet A installation with multi wallet feature
  • Verify wallet B posthog user is merged with wallet A posthog user
  • Repeat step with wallet B being a hardware wallet

Hardware wallet operations immediately after onboarding

  • Import hardware wallet with user onboarding
  • Immediately test hardware wallet operations (staking, sending, etc...)

Screenshots

Attach screenshots here if implementation involves some UI changes

@github-actions github-actions bot added browser Changes to the browser application. ui-toolkit labels Feb 21, 2024
@lucas-barros lucas-barros changed the base branch from main to LW-9170-multi-wallet-integration February 21, 2024 18:39
@lucas-barros lucas-barros self-assigned this Feb 21, 2024
Copy link

github-actions bot commented Feb 21, 2024

Allure report

allure-report-publisher generated test report!

smokeTests: ✅ test report for b1c8acd4

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

@lucas-barros lucas-barros force-pushed the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch 2 times, most recently from dd8c73b to c8c7217 Compare February 21, 2024 22:14
@coveralls
Copy link

coveralls commented Feb 21, 2024

Coverage Status

Changes unknown
when pulling b1c8acd on feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored
into ** on main**.

@lucas-barros lucas-barros force-pushed the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch 2 times, most recently from c76c207 to d7da760 Compare February 22, 2024 18:25
@lucas-barros lucas-barros marked this pull request as ready for review February 22, 2024 18:26
@lucas-barros lucas-barros requested a review from a team as a February 22, 2024 18:26
connectedDevice: model,
deviceConnection: connection,
name,
accountIndex: account
});
await analytics.sendMergeEvent(source.account.extendedAccountPublicKey);
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to send the public key in plain text?

Copy link
Author

Choose a reason for hiding this comment

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

Its not sent as is. Its hashed first with generateWalletBasedUserId.

@@ -53,7 +56,8 @@ export const RestoreRecoveryPhrase = (): JSX.Element => {
}}
onSubmit={useCallback(async () => {
try {
await createWallet(data);
const { source } = await createWallet(data);
await analytics.sendMergeEvent(source.account.extendedAccountPublicKey);
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to send & save the public key in plain text?

Copy link
Author

Choose a reason for hiding this comment

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

It's not sent as is. It's hashed first with generateWalletBasedUserId.

@VanessaPC VanessaPC self-requested a review February 23, 2024 13:00
Copy link
Contributor

@VanessaPC VanessaPC left a comment

Choose a reason for hiding this comment

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

LGTM!

@tomislavhoracek tomislavhoracek force-pushed the LW-9170-multi-wallet-integration branch from daa2543 to e4ed066 Compare February 27, 2024 08:57
Base automatically changed from LW-9170-multi-wallet-integration to main February 28, 2024 11:22
@lucas-barros lucas-barros force-pushed the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch from d7da760 to 5a847e6 Compare February 28, 2024 12:10
@lucas-barros lucas-barros force-pushed the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch 2 times, most recently from 9f3fdb4 to caa9ebc Compare March 8, 2024 12:57
@lucas-barros lucas-barros force-pushed the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch from caa9ebc to d30d8fa Compare March 8, 2024 14:27
@lucas-barros lucas-barros force-pushed the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch from d30d8fa to b1c8acd Compare March 8, 2024 16:03
Copy link

sonarcloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@lucas-barros lucas-barros merged commit 4879c29 into main Mar 11, 2024
12 of 13 checks passed
@lucas-barros lucas-barros deleted the feat/lw-9813-create-posthog-alias-after-new-wallet-is-restored branch March 11, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser Changes to the browser application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants