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

Introduce Create Single-Sig wallet flow #403

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

johnny9
Copy link
Contributor

@johnny9 johnny9 commented May 28, 2024

These changes add more pages after the initial onboarding.

To test, run the application with the -resetguisettings option.

Link to github actions build artifacts.

Build Artifacts

@johnny9 johnny9 force-pushed the create-wallet-pr branch 3 times, most recently from fb61899 to 4c047e0 Compare May 28, 2024 04:00
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

  • every entrybox should automatically be selected, so user doesn't have to move the cursor there and click it to enter something
  • at Create password, good to give feedback when user can't continue because passwords don't match
  • CreateConfirm dialog shouldn't have a back button imo, the wallet is already created so going back doesn't make sense, same for CreateBackup

some other things, but maybe for a follow-up:

  • check and don't allow to enter an already existing wallet name
  • at every entry it is intuitive to press enter to trigger the continue button, maybe good time to introduce the behavior that enter keystroke triggers it
  • the transition to the wallet home page after clicking done at CreateConfirm is in the opposite direction of the rest of the create wallet flow, why?

CoreText {
Layout.topMargin: 25
Layout.alignment: Qt.AlignCenter
text: qsTr("You are about to create \na single-key bitcoin wallet")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
text: qsTr("You are about to create \na single-key bitcoin wallet")
text: qsTr("You are about to create \na single-sig bitcoin wallet")

Copy link
Contributor

Choose a reason for hiding this comment

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

It says "single-key" also in figma.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, "single-key" is wrong and also confusing imo

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur "single-sig" is more intuitive than "single-key". It seems "single-key" is referenced in the docs although most people/users only refer to "single-sig"

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally use multi-key in the design community (example). Some reasons:

  • "sig" is an abbreviation that many people may not be familiar with
  • Avoiding "signature" altogether in the applications removes another technical concept that people don't really need to be aware of. For example, "sign transaction" can be just be the more common term "approve transaction". Just part of the overall idea to use fewer technical terms if possible
  • If we want to be super specific in technical terms, then a "single-sig" wallet could even be a "multi-key" wallet. For example, in a wallet with 3 keys that only requires 1 signature

I understand that the technical terms can be more intuitive for us who are super deep into the tech, and they allow us to discuss things more precisely. For broader audiences, especially global ones where english may be the second language, it's often best to simplify things a little (you even have tools for that like Hemingway). Does that sound reasonable?

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK 4c047e0

It creates the wallet successfully, with or without password (skip it when it's offered).

I agree with all observations from @MarnixCroes as, tested on desktop platform, I experienced the same issues.

some other things, but maybe for a follow-up:

* check and don't allow to enter an already existing wallet name

This is what I first tested, when user enters the same wallet name doesn't do anything/ warn about the situation (doesn't override existent ones at least 🙏🏼) and it goes to the CreateConfirm saying it was created.

* the transition to the wallet home page after clicking done at CreateConfirm is in the opposite direction of the rest of the create wallet flow, why?

I'd say opposite direction to the rest of the onboarding flow even, so create wallet flow follows the same pattern (screen passed to the left as we move forward to the next form/ screen to the right) until the user clicks on "Done" or even if the user "skip"s the wallet creation. Unless it's intentional, it seems there's an inconsistency here.

On CreateBackup.qml, there's a "View file" button (that's doesn't work yet?), is this the backup feature? If so I see in figma says "Save wallet file" on that button, otherwise other than "CreateBackup" I'd name the .qml file as "BackupIntro" o "BackupWarning"/ Info.

I see other discrepancies (missing options/ different wording) against figma, is this intentional?

CoreText {
Layout.topMargin: 25
Layout.alignment: Qt.AlignCenter
text: qsTr("You are about to create \na single-key bitcoin wallet")
Copy link
Contributor

Choose a reason for hiding this comment

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

It says "single-key" also in figma.

Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

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

tACK 4c047e0 on WSL Ubuntu 22.04.

Wallet creation worked. In general agree with Marnix's comments.

And to be clear is this the first iteration, then layering more functionality (i.e. error messages, wallet name collision and more specific passwords hints )?

@johnny9
Copy link
Contributor Author

johnny9 commented May 31, 2024

tACK 4c047e0 on WSL Ubuntu 22.04.

Wallet creation worked. In general agree with Marnix's comments.

And to be clear is this the first iteration, then layering more functionality (i.e. error messages, wallet name collision and more specific passwords hints )?

Yeah this is just following what was ready in the Figma. I think building it helps identify all of the gaps. I think all of these comments are great and I'll do my best to get these in on this first change-set.

qInstallMessageHandler(DebugMessageHandler);
//qInstallMessageHandler(DebugMessageHandler);
Copy link
Member

Choose a reason for hiding this comment

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

Why is that commented out?

@hebasto
Copy link
Member

hebasto commented Jun 8, 2024

cc @GBKS

@stackingsaunter
Copy link

stackingsaunter commented Jun 26, 2024

Testing the PR, so far worked well and created a wallet. "View file" button didn't work at all though.

I see other discrepancies (missing options/ different wording) against figma, is this intentional?

Yeah this is just following what was ready in the Figma. I think building it helps identify all of the gaps. I think all of these comments are great and I'll do my best to get these in on this first change-set.

@johnny9 mentioned in the issue in design repo, the lastest Figma design is here and was not yet merged to main Figma file.

It does not differ greatly from what's in this PR. Minor changes were introduced from user testing the flow. All differences should be highlighterd in the file and issue

@D33r-Gee
Copy link
Contributor

tACK a3de255 on WSL Ubuntu 22.04

Works as expected, was able to create a new wallet in the default datadir

@hebasto hebasto merged commit a73577b into bitcoin-core:main Aug 1, 2024
9 checks passed
hebasto added a commit that referenced this pull request Aug 12, 2024
…lly selected

0beac80 qml: create wallet: set focus on TextField (Marnix)

Pull request description:

  add `focus: true` to make it automatically selected so user doesn't have to first manually select it before it can type something

  addresses #403 (review)

ACKs for top commit:
  johnny9:
    tACK 0beac80

Tree-SHA512: 61430831010dbd582176e9fd57ec7dcafbc5c705ee7ede7d7d33bc5d49e76340d4d233de385f2b59fc8c27d4422d3d82c30238678a5ee1429f7659800e6820b0
hebasto added a commit that referenced this pull request Aug 12, 2024
456d28a qml: uncomment DebugMessageHandler install (johnny9)

Pull request description:

  DebugMessageHandler is needed to route output to debug.log and should not have been commented out.

  Mistakenly commented out in #403

ACKs for top commit:
  MarnixCroes:
    ACK 456d28a
  hebasto:
    ACK 456d28a

Tree-SHA512: c78e526deccba366854287c796e2ac4843f1aba7b936d6955b6ec2212e75fac6a4372e3994e4045a45e083c7422fb827fddb674c64a593affa1226589f14d8ab
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.

7 participants