Skip to content

Add validation to the Send form (address and amount) #462

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

johnny9
Copy link
Collaborator

@johnny9 johnny9 commented Jun 1, 2025

This is built on top of #450 so when reviewing, its best to select just the new commits in the "Files changed" tab.

These changes add properties for the error strings used in the QML to SendRecipient and add validation methods that are checked when the relevant properties are changed by the user.

Screenshot from 2025-06-01 12-42-20

@johnny9 johnny9 force-pushed the send-validation branch 2 times, most recently from 666aff7 to 1e81bb6 Compare June 1, 2025 20:59
setAmountError(tr("Amount must be greater than zero"));
} else if (m_amount->satoshi() > MAX_MONEY) {
setAmountError(tr("Amount exceeds maximum limit"));
} else if (m_amount->satoshi() > m_wallet->balanceSatoshi()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could remove this case for now as it allows removing the circular dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this might be a good idea and I can just do this validation when creating the transaction in the wallet model

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think im going to leave the circular dependency in for now.and take another look when doing validation with multiple recipients

@GBKS
Copy link
Contributor

GBKS commented Jun 2, 2025

Nice addition. A few notes:

  1. Can we restrict the character input altogether, so I can only enter numbers (and periods/commas) in the amount field and only allowed characters in the address field?
  2. We should also add a max length. I think 62 is max for addresses, and 17 for bitcoin amounts (21000000.00000000). Not 100% on those numbers. Either way, right now I can type in infinite characters, so some restriction would be good.
  3. I was not able to get any address to validate (grabbed a few different address types from here).
  4. I see the error "Amount exceeds maximum limit". Would be more useful if it states the limit, like "The maximum is 21,000,000." or so - sneak in the supply cap.
  5. When I enter a huge number and deselect the field, the number changes. Not sure why, and I could not find a pattern how it changes.
  6. For an empty wallet, there is the question whether the Send form should be enabled at all.

Always a bit finicky these input fields.

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.

1e81bb6

  • the UX is better if the verification is done without having to change focus from the field:
Screencast.from.2025-06-02.13-12-13.webm
  • the amount converter doesn't seem to be working as it should:
Screencast.from.2025-06-02.13-14-41.webm

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 3, 2025

1e81bb6

* the UX is better if the verification is done without having to change focus from the field:

Good catch, I will split up the validation and formatting helpers so that validation can happen while editing and it will reformat once editing is finished.

Screencast.from.2025-06-02.13-12-13.webm

* the amount converter doesn't seem to be working as it should:

Screencast.from.2025-06-02.13-14-41.webm

bummer, must have introduced a bug with conversion. I will double check the sats to btc conversion.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 3, 2025

Nice addition. A few notes:

  1. Can we restrict the character input altogether, so I can only enter numbers (and periods/commas) in the amount field and only allowed characters in the address field?
    Yes, I can add validators to restrict inputs into this form
  2. We should also add a max length. I think 62 is max for addresses, and 17 for bitcoin amounts (21000000.00000000). Not 100% on those numbers. Either way, right now I can type in infinite
    characters, so some restriction would be good.

I will look at what options there are to restrict the character limit

  1. I was not able to get any address to validate (grabbed a few different address types from here).

Thats interesting. I will double check. Could it be regtest/testnet/mainnet differences in format.

  1. I see the error "Amount exceeds maximum limit". Would be more useful if it states the limit, like "The maximum is 21,000,000." or so - sneak in the supply cap.

I can update the error string to include this

  1. When I enter a huge number and deselect the field, the number changes. Not sure why, and I could not find a pattern how it changes.

This is a precursor to what will end up being the rich formatting of the amount. If you are inputing BTC amounts it will add the satoshi trailing 0's

  1. For an empty wallet, there is the question whether the Send form should be enabled at all.

Always a bit finicky these input fields.

I will consider this.

@johnny9 johnny9 force-pushed the send-validation branch from 1e81bb6 to a5aba54 Compare June 4, 2025 04:39
@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 4, 2025

Update from 1e81bb6 to a5aba54

  • rebased with main

@johnny9 johnny9 force-pushed the send-validation branch from 6991ba5 to f1ad73a Compare June 4, 2025 05:29
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.

tested LGTM 3630bc2

Note that when entering full balance, Review is highlighted but not clickable.

As the GUI won't allow creating tx's with 0 fees, maybe Review should only be highlighted when amount < balance

@GBKS
Copy link
Contributor

GBKS commented Jun 5, 2025

Looking good.

Address input

  1. I just fixed some of the input logic in the prototype, so it properly breaks into multiple lines and applies for formatting. Not sure if you're referencing that right now or not.

Amount input

  1. Do we need to force the 8 decimals for bitcoin amounts? Seems easier to just have 0.1 rather than 0.10000000.
  2. The unit toggle needs hover states
  3. As I mentioned before, when the amount exceeds the balance, we discussed showing Balance: 0,00348200 ₿. Send max., where Send max is underlined and a button that inserts the full balance into the input field. I have this partially working in the prototype, but need to fix some logic. Gets a little tricky when we also have the Include fee in amount toggle and multiple recipients. Maybe something for a separate PR?

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 5, 2025

tested LGTM 3630bc2

Note that when entering full balance, Review is highlighted but not clickable.

As the GUI won't allow creating tx's with 0 fees, maybe Review should only be highlighted when amount < balance

This is a good one. I will add an issue to include this in my validation when I add proper fee estimation.

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.

4 participants