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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+26 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a124f2e
rusk-wallet: Add confirmation if sending to own address
Daksh14 Nov 24, 2024
b2aa55e
rusk-wallet: Fix clippy lint
Daksh14 Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion rusk-wallet/src/bin/interactive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,15 @@ pub(crate) async fn run_loop(
match op {
Ok(ProfileOp::Run(cmd)) => {
// request confirmation before running
if confirm(&cmd, wallet)? {
let confirm_prompt = match confirm(&cmd, wallet) {
Ok(x) => x,
Err(e) => match e.downcast_ref::<InquireError>() {
Some(InquireError::OperationCanceled) => continue,
_ => return Err(e),
},
};

if confirm_prompt {
// run command
prompt::hide_cursor()?;
let result = cmd.run(wallet, settings).await;
Expand Down Expand Up @@ -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?

&& !prompt::ask_self_send_confirm()?
{
// we throw operation cancelled error so it is handeled by
// the event loop and we are just sent
// back to the last screen
return Err(InquireError::OperationCanceled.into());
}

prompt::ask_confirm()
}
Command::Stake {
Expand Down
6 changes: 6 additions & 0 deletions rusk-wallet/src/bin/io/prompt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,12 @@ pub(crate) fn ask_confirm() -> anyhow::Result<bool> {
.prompt()?)
}

/// Asks the user for confirmation
pub(crate) fn ask_self_send_confirm() -> anyhow::Result<bool> {
Ok(Confirm::new("ALERT: You are transfering to an address which belongs to this wallet. Proceed?")
.prompt()?)
}

/// Asks the user for confirmation before deleting cache
pub(crate) fn ask_confirm_erase_cache(msg: &str) -> anyhow::Result<bool> {
Ok(Confirm::new(msg).prompt()?)
Expand Down
Loading