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

rusk-wallet: Add confirmation if sending to own address #3049

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Daksh14
Copy link
Contributor

@Daksh14 Daksh14 commented Nov 24, 2024

Closes #2959

@HDauven HDauven requested a review from welf November 25, 2024 11:36
@moCello
Copy link
Member

moCello commented Nov 25, 2024

I actually don't think we need this check.
In order to create a transaction the user needs to enter the receiver address and I think this process is manual enough to ensure that the intended address is used.
Additionally we already preview both the sender and receiver address when we prompt the user to confirm the transaction, so I think we do enough already to prevent an unintended transaction.

@@ -379,6 +387,17 @@ fn confirm(cmd: &Command, wallet: &Wallet<WalletFile>) -> anyhow::Result<bool> {
if let Address::Public(_) = sender {
println!(" > ALERT: THIS IS A PUBLIC TRANSACTION");
}

// check if we are sending to our own address
if wallet.claim(rcvr.clone()).is_ok()
Copy link
Contributor

Choose a reason for hiding this comment

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

The essence of the issue to prevent possibly erroneous transaction when the user pays a fee when tokens are not actually moving (try to transfer Dusk from any account to the SAME account). Currently, the PR warns also if you move tokens between profiles but should prompt the user only when the receiver address == sender address.

Copy link
Member

Choose a reason for hiding this comment

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

There are cases though when creating a transaction to the same address is what a user wants to do, i.e. when sending a memo, or when merging phoenix notes. An extra confirmation in those cases is not desirable.
And as I argue above, we already prompt the user to confirm the transaction and double check the sender and receiver address. Imo this is more than enough and adding another confirmation is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@moCello It was not obvious to me when I first tried it. My intention was to transfer Dusk from my public account to my shielded account. I thought I could do it using the transfer menu. I guess I am not the only one who misunderstands how to transfer tokens from the public account to the shielded one without reading the docs first. Since the fee is charged, I think it is better to ask one more time before executing the transaction.

Copy link
Member

Choose a reason for hiding this comment

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

To me, what you are describing hints more to a documentation problem though.
But I won't block adding this extra check.
However, if we add the extra confirmation (beyond the already existing confirmation of the transfer), it should only appear when an attempt is made to:

  • transfer public DUSK
  • without a memo
  • when the sender and receiver is the exact same moonlight account.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @moCello here on the conditions, and if I'm not mistaken those are also inline with the original issue. To not let users do a double confirmation, can we instead provide a warning message to users @Daksh14 that doesn't require a confirmation?

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.

rusk-wallet: Ask user to confirm if they send to their own address
4 participants