Skip to content

Add Multiple Recipients option to the Send form #450

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 May 8, 2025

This change introduces the multiple recipients controls to the Send form. It is enabled in the ellipses option menu by toggling on "Enable Multiple Recipients". This is stored as a QSetting for the user.

This PR depends on #448 which contains the

Screenshot from 2025-05-08 11-51-34
first implementation of the Send options menu.
Screenshot from 2025-05-08 11-51-26

@johnny9 johnny9 marked this pull request as draft May 8, 2025 15:39
@johnny9 johnny9 marked this pull request as ready for review May 29, 2025 04:51
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.

b4e421e

  • the amount is not tied to each recipient. If I enter x at 1, all have amount x
  • small nit, could this be centered

Screenshot from 2025-05-30 13-26-54

@johnny9
Copy link
Collaborator Author

johnny9 commented May 31, 2025

b4e421e

* the amount is not tied to each recipient. If I enter x at 1, all have amount x

* small nit, could this be centered

Screenshot from 2025-05-30 13-26-54

I believe I have both of these issues resolved now with the latest commits.

@johnny9
Copy link
Collaborator Author

johnny9 commented May 31, 2025

Discovered an issue with the calculation of the total amount in the review ui and coin selection ui. Will resolve that today.

@johnny9
Copy link
Collaborator Author

johnny9 commented May 31, 2025

Discovered an issue with the calculation of the total amount in the review ui and coin selection ui. Will resolve that today.

This should be resolved now with the last two commits.

@GBKS
Copy link
Contributor

GBKS commented Jun 2, 2025

Looks great.

  1. There seems to be a small issue with the removal logic.
  • For recipients 1, 2, and 3, I respectively enter a, b and c into the Send to field
  • With recipient 1 selected, I click - to remove the current recipient (as expected)
  • A recipient is removed, I can tell from the header changing from Recipient 1 of 3 to Recipient 1 of 2
  • However, I still see a in the Send to field. I expected the current recipient to be removed and that I now would see b.
  • I click > and now see c in the Send to field
  • I click < and now see b in the Send to field

Not sure if that outline is easy to follow, but by guess is that the current recipient is correctly being removed, but the UI is not being refreshed.

  1. There are some visual tweaks to be made, but those can be done separately.

  2. Is there a limit on how many recipients can be added? Should we restrict it to something like 25 for now? Michael and I were wondering about that.

  3. Also, should the Note to self be per recipient or for the whole transaction? In the prototype I have it per recipient (essentially an address label). But in this implementation it's the same for all recipients.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 3, 2025

Looks great.

1. There seems to be a small issue with the removal logic.


* For recipients 1, 2, and 3, I respectively enter `a`, `b` and `c` into the `Send to` field

* With recipient 1 selected, I click `-` to remove the current recipient (as expected)

* A recipient is removed, I can tell from the header changing from `Recipient 1 of 3` to `Recipient 1 of 2`

* However, I still see `a` in the `Send to` field. I expected the current recipient to be removed and that I now would see `b`.

* I click `>` and now see `c` in the `Send to` field

* I click `<` and now see `b` in the `Send to` field

Not sure if that outline is easy to follow, but by guess is that the current recipient is correctly being removed, but the UI is not being refreshed.

I think this is the issue. I think I know what signal is missing and I will try it out.

2. There are some visual tweaks to be made, but those can be done separately.
3. Is there a limit on how many recipients can be added? Should we restrict it to something like 25 for now? Michael and I were wondering about that.

There is currently no limit. I'll put it at 25 just so we have something

4. Also, should the `Note to self` be per recipient or for the whole transaction? In the prototype I have it per recipient (essentially an address label). But in this implementation it's the same for all recipients.

I think per recipient/output might make more sense but i can see the value of per transaction. Note to self isn't implemented yet so that part of the form just goes no where. I still need to figure out what is supported currently in core for this as that might dictate which way we go.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 3, 2025

Update from e5cd1a7 to 8a6b593

  • rebased with main

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 3, 2025

Looks great.

1. There seems to be a small issue with the removal logic.
3. Is there a limit on how many recipients can be added? Should we restrict it to something like 25 for now? Michael and I were wondering about that.

These two issues are now resolved

@johnny9 johnny9 requested a review from MarnixCroes June 4, 2025 04:36
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.

8a6b593
I found multiple things, probably some are to be fixed and some can be follow-up.

  • Have 2 recipients, delete recipient 2. It stays at recipient 2 and displays Recipient 2 of 1:

Screenshot from 2025-06-04 09-30-31

  • I noticed a delay in the coin selection Remaining to select / Over required amount, this does not happen on master
Screencast.from.2025-06-04.09-12-47.webm
  • Have multiple recipients, one in sats and the other in BTC -> the Transaction Details screen is confusing as it doesn't have units:

Screenshot from 2025-06-04 09-14-57

Some issues when having 2 or more recipients and disabling Multiple Recipients as the other disabled recipients are still taken into account:

  • After disabling multiple recipients, the Send view will show the current selected single recipient, but the amount in Transaction Details is the sum of the multiple recipients (even though disabled)
Screencast.from.2025-06-04.09-07-22.webm
  • Recipient 2 is empty, disable Multiple Recipients. Send view is at recipient 1 (with address and amount) but Review doesn't work.

@rabbitholiness
Copy link

I also noticed a few additional things to the ones mentioned above:

  • If I switch to Sats as a unit for the amount and then add a new recipient, the unit for that newly added recipient is changed to ₿. I would have expected it to also use Sats.
  • Since the address is on one line, it gets cut off. I guess the multi-line view just isn't implemented yet?
Screenshot 2025-06-05 at 9 08 43
  • If I use the same address for different recipients, then we should probably highlight that somehow. Based on how difficult it is to implement, there is a more generic way to just say "You're reusing this address for multiple recipients." But ideally we would say "This address is being reused for Recipient x and Recipient y." The names of the recipients could be a link to the corresponding form. Here's how that could look like.
    address reuse multiple recipients
  • I can't get to the review screen, for some reason.
  • I think the note to self should be per recipient. Especially since it should be used to identify the payments on the review screen (and on the transaction details page), eventually.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 6, 2025

8a6b593 I found multiple things, probably some are to be fixed and some can be follow-up.

* Have 2 recipients, delete recipient 2. It stays at recipient 2 and displays `Recipient 2 of 1`:

Found the off by one error. It should work correctly now.

Screenshot from 2025-06-04 09-30-31

* I noticed a delay in the coin selection `Remaining to select` / `Over required amount`, this does not happen on master

This is a similar fix to the validation. I've push the fix to this PR as well.

Screencast.from.2025-06-04.09-12-47.webm

* Have multiple recipients, one in sats and the other in BTC -> the Transaction Details screen is confusing as it doesn't have units:

The transaction details I will create a new task/issue as a follow up as I would like to do a bunch of amount labels at once.

Screenshot from 2025-06-04 09-14-57

Some issues when having 2 or more recipients and disabling Multiple Recipients as the other disabled recipients are still taken into account:

* After disabling multiple recipients, the Send view will show the current selected single recipient, but the amount in `Transaction Details` is the sum of the multiple recipients (even though disabled)

Screencast.from.2025-06-04.09-07-22.webm

* Recipient 2 is empty, disable `Multiple Recipients`. Send view is at recipient 1 (with address and amount) but `Review` doesn't work.

I now have it clearing out the list up to the first recipient when the setting is disabled and the amounts should be correct now.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 6, 2025

I also noticed a few additional things to the ones mentioned above:

* If I switch to Sats as a unit for the amount and then add a new recipient, the unit for that newly added recipient is changed to ₿. I would have expected it to also use Sats.

ok, I set the new recipient to use the current's units. if we want it to be global, I will create an issue and do a separate follow up as it requires some refactoring

* Since the address is on one line, it gets cut off. I guess the[ multi-line view](https://lively-kashata-cfde7e.netlify.app/screen/send) just isn't implemented yet?

yes I will fix the wrapping when i do the address formatting

Screenshot 2025-06-05 at 9 08 43
* If I use the same address for different recipients, then we should probably highlight that somehow. Based on how difficult it is to implement, there is a more generic way to just say "You're reusing this address for multiple recipients." But ideally we would say "This address is being reused for Recipient x and Recipient y." The names of the recipients could be a link to the corresponding form. Here's how that could look like.
  ![address reuse multiple recipients](https://private-user-images.githubusercontent.com/112604177/451751927-97e07352-1922-4c4e-82ab-18adde0ea9df.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDkxODM5NTcsIm5iZiI6MTc0OTE4MzY1NywicGF0aCI6Ii8xMTI2MDQxNzcvNDUxNzUxOTI3LTk3ZTA3MzUyLTE5MjItNGM0ZS04MmFiLTE4YWRkZTBlYTlkZi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwNjA2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDYwNlQwNDIwNTdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT02YTkyMWRiOGE3MjBiYWJjZTcxOGUxYTgwYTY4YTAwZWFjMTE5OTJlOTBlMjlkZDZiNDI2YzZmZmY4NTJkYTU2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.ZDrE85L9EC21gdVcs3xe1ziPF4-_7ih3HT6yDD1_Bvc)

i agree with this and will create a follow up task.

* I can't get to the review screen, for some reason.

that means an input was wrong. additional validation is needed to make that clear. hopefully the send validation PR catches what is missing in your case.

* I think the note to self should be per recipient. Especially since it should be used to identify the payments on the review screen (and on the transaction details page), eventually.

It should be. I think this is a bug.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 6, 2025

All comments have been addressed via a commit or new issue.

@GBKS
Copy link
Contributor

GBKS commented Jun 6, 2025

Awesome progress. Not sure if it's covered elsewhere, here are some things I am seeing based on the latest code:

  • I can enter infinite characters in the address, amount and note fields. Super long amounts end up overlapping the unit toggle and going outside of the form boundaries.
  • I can enter letter (abc...) in the amount input, should be just numbers and comma/period.
  • When enabling multiple recipients, it should automatically add a second one.
  • In the prototype, I set up an application-wide setting for the unit format. So if you toggle it in the form for one recipient, it also toggles it for all other recipients, the balance in the top left, any other screen you visit, etc. As a mental model, that seems easier track than having individual components have their own setting.
  • The disabled state for the pagination/add/remove buttons is super dark, it should be Neutral 4 instead of Neutral 2.
  • When a transaction was sent, the Multiple recipients setting should also reset to disabled. Weird to be on the screen and having it read Recipient 1 of 1.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jun 6, 2025

Awesome progress. Not sure if it's covered elsewhere, here are some things I am seeing based on the latest code:

* I can enter infinite characters in the address, amount and note fields. Super long amounts end up overlapping the unit toggle and going outside of the form boundaries.

* I can enter letter (abc...) in the amount input, should be just numbers and comma/period.

These two are addressed in #462

* When enabling multiple recipients, it should automatically add a second one.

This makes sense, will add

* In the prototype, I set up an application-wide setting for the unit format. So if you toggle it in the form for one recipient, it also toggles it for all other recipients, the balance in the top left, any other screen you visit, etc. As a mental model, that seems easier track than having individual components have their own setting.

I'll make an issue to track this for a follow up. I need to refactor some things to get the setting to be global.

* The disabled state for the pagination/add/remove buttons is super dark, it should be Neutral 4 instead of Neutral 2.

Will fix.

* When a transaction was sent, the `Multiple recipients` setting should also reset to disabled. Weird to be on the screen and having it read `Recipient 1 of 1`.

Ill try to fix this as well.

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