Skip to content

Add one-click Coinos wallet setup #2982

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

Merged
merged 2 commits into from
Apr 21, 2025
Merged

Conversation

danieldaquino
Copy link
Collaborator

Summary

This commit implements a one-click Coinos wallet setup.

This was implemented using the Coinos API, and using account details that are deterministically generated from the user's private key.

Checklist

  • I have read (or I am familiar with) the Contribution Guidelines
  • I have tested the changes in this PR
  • I have opened or referred to an existing github issue related to this change.
  • My PR is either small, or I have split it into smaller logical commits that are easier to review
  • I have added the signoff line to all my commits. See Signing off your work
  • I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
    • I do not need to add a changelog entry. Reason: [Please provide a reason]
  • I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches

Test report

Device: iPhone SE simulator

iOS: 18.2

Damus: ac1ae3a

Steps:

  1. Create new deterministic wallet using one-click setup, ensure it works and immediately takes user to the balance view.
  2. Receive a zap from another account, ensure the zap is received on the new wallet, and that the balance view reflects that.
  3. Send a zap to another account, ensure the zap is sent and the balance view reflects that after refreshing.
  4. Disconnect wallet and connect Coinos wallet manually via the website. Ensure wallets can still be connected manually.
  5. Disconnect that manual wallet and try the one-click setup again. Ensure it loads the user's existing deterministic wallet (previous balance and transactions are there).
  6. Login with the user's npub. Ensure the "one-click setup" option is not available, with a human-readable message.
  7. Login with the user's nsec again, and connect the deterministic wallet
  8. Restart app. Ensure the deterministic wallet is still there, connected.
  9. Disconnect wallet again.
  10. Disconnect Wi-Fi and try the one-click setup. Ensure that — after a few seconds — a human friendly error screen appears.

Results:

  • PASS
  • Partial PASS
    • Details: [Please provide details of the partial pass]

Screenshots

Simulator Screenshot - iPhone SE (3rd generation) - 2025-04-16 at 20 28 50
Simulator Screenshot - iPhone SE (3rd generation) - 2025-04-16 at 20 28 04
Simulator Screenshot - iPhone SE (3rd generation) - 2025-04-16 at 20 42 35

// In terms of the risk of an accidental collision due to the birthday problem, 16 characters should be enough to pragmatically avoid any collision.
// According to `https://en.wikipedia.org/wiki/Birthday_problem#Probability_table`,
// even if we have 610 million Damus users connected to Coinos, the probability of even a single collision is still as low as 1%.
return String(fullText.prefix(16))
Copy link
Collaborator

Choose a reason for hiding this comment

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

does coinos have a 16 character limit on username or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was for aesthetic reasons only. 32 hex-encoded bytes looks too long.

I was thinking about using deterministic mnemonic words (e.g. "[email protected]"), but it's not really visible and we want to discourage users from trying to manually login their account, so maybe it's better to not use mnemonics either. It would also add complexity to the deterministic algorithm, so maybe it's best not to.

.disabled(self.userKeypair.privkey == nil)

if self.userKeypair.privkey == nil {
Text("The one-click setup requires the user's private key. Please login with your nsec to use this option.", comment: "Warning text for users who cannot create a Coinos account via the one-click setup without being logged in with their nsec.")
Copy link
Collaborator

@jb55 jb55 Apr 17, 2025

Choose a reason for hiding this comment

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

this message sounds a bit scary, it almost sounds like we are sending the users private key to coinos or something. maybe we should make it clear that the users profile is not linked to the coinos account, and there is no identifiable information

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe something like: "you must be logged in with your nsec to use one-click setup. your profile will not be shared with coinos"

This commit implements a one-click Coinos wallet setup.

This was implemented using the Coinos API, and using account details
that are deterministically generated from the user's private key.

Closes: damus-io#2961
Changelog-Added: Added one-click Coinos wallet setup
Signed-off-by: Daniel D’Aquino <[email protected]>
@danieldaquino
Copy link
Collaborator Author

@jb55, @ericholguin, I updated the sheet, here is how it looks like now:

1 2
Simulator Screenshot - iPhone SE (3rd generation) - 2025-04-18 at 11 42 40 Simulator Screenshot - iPhone SE (3rd generation) - 2025-04-18 at 11 56 02

Please let me know if you have any other suggestions!

Changelog-Changed: Added disclaimer to clarify that Coinos is a third-party service
Signed-off-by: Daniel D’Aquino <[email protected]>
@danieldaquino
Copy link
Collaborator Author

Added a new (optional) commit which adds a small disclaimer to clarify that Coinos is a third-party service. This is not legal advice, and I am not a legal expert — but I suppose it makes sense to add this, for the sake of clarity? cc @jb55

@danieldaquino danieldaquino merged commit 67f0e3d into damus-io:master Apr 21, 2025
@jb55
Copy link
Collaborator

jb55 commented Apr 24, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants