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

Add option to cancel pending transaction #3406

Closed
314159265359879 opened this issue Mar 10, 2023 · 22 comments
Closed

Add option to cancel pending transaction #3406

314159265359879 opened this issue Mar 10, 2023 · 22 comments

Comments

@314159265359879
Copy link
Contributor

314159265359879 commented Mar 10, 2023

Brice: The problem is that its not possible to cancel a transaction. Once it's reached the mempool, a miner could include it in a block. There's no way to prove whether or not the miner received your attempt to cancel it. The better option is to create a new transaction with the same nonce and a higher fee. Then it is expected that the miner will pick up the higher fee transaction instead.

image

We can make replacing a transaction super easy. For the user it could be much like "delete transaction" By creating a STX transfer of 0.000001 to the burn address with a fee that is default set to 0.000001 STX more than the transaction being replaced. We could call it a cancel request rather than a delete transaction, as we can never be 100% certain the old isn't picked up anyway, the new transaction would have to be propagated through the network before a miner picks it up.

I think it would make it a lot easier for users to deal with troublesome transactions. Because it wouldn't require a user to understand nonces or RBF... it is handled for them.

Lets combine this with always showing the button to increase the fee for a transaction. They should be always visible. Users do not realize the option is there if it is only shown on hover.

This has been theorized and requested many times. It is a great help when an account has questionable transactions if something went wrong. For example a transaction to send 500 STX was send twice instead of once.

Please note that if we make sending STX very strictly #3389 possible based on available balance. I would want a "cancel request" option in case a user still succeeds in creating transactions that will remain pending until dropped. Because the regular flow may then nolonger allow sending an RBF transaction like this to unclog the wallet:
This transfer of 1 uSTX and a fee of 0.003001 STX replaces the 0.027 transfer with a fee of 0.003 STX:

image

@314159265359879 314159265359879 added Enhancement 💡 good-first-issue enhancement-p1 Critical functionality needed by many users, with no clear alternatives labels Mar 10, 2023
@markmhendrickson
Copy link
Collaborator

Dupe of #1834?

@ghost
Copy link

ghost commented Apr 11, 2023

Hi @314159265359879,

I would like to work on adding a "Cancel request" button to delete pending transactions, as mentioned in issue #3406. I have over 2 years of experience in JavaScript, Python, and building scalable systems. I am eager to begin my open source journey by contributing to this project.

Please consider assigning this issue to me, and let me know if there are any additional details or specifications I should be aware of before I start working on it.

Thank you!

Best regards,
Muditya Raghav

@314159265359879
Copy link
Contributor Author

Hi @Mudityadev thanks for wanting to pick this up, that is awesome. @markmhx anything to keep in mind and do you know if Michelly had time to look at design for this already. or is the current mockup okay for now?

@markmhendrickson
Copy link
Collaborator

Thanks for offering to help here, @Mudityadev.

@314159265359879 I presume these are the specific design needs:

  1. Option to select transaction as listed under activity for cancellation (e.g. "Cancel transaction")
  2. Screen for confirming cancellation upon its selection (e.g. "Are you sure you want to cancel this transaction? It will cost N STX in fees")
  3. (Optional) Confirmation screen for cancellation request with link to more information about how cancellations work and the lack of guarantee that it works
  4. (Optional) Special treatment of the cancellation transaction in activity (e.g. "Cancellation request" with indication of just which other transaction is intended for cancellation by it)

Given these pieces, I think it'd be best if @mica000 design this properly before we head into development. I'll get it on the board.

@markmhendrickson markmhendrickson changed the title Add option to delete pending transaction: "Cancel request" (button always on top) Add option to cancel pending transaction Apr 17, 2023
@markmhendrickson markmhendrickson moved this from Assigned to Enhancements backlog in Hiro Wallet (DEPRECATED) Apr 17, 2023
@markmhendrickson markmhendrickson moved this from Enhancements backlog to Triage in Hiro Wallet (DEPRECATED) Jun 22, 2023
@markmhendrickson markmhendrickson moved this from Triage to Assigned in Hiro Wallet (DEPRECATED) Jul 3, 2023
@markmhendrickson markmhendrickson moved this from Assigned to Triage in Hiro Wallet (DEPRECATED) Jul 3, 2023
@markmhendrickson markmhendrickson moved this from Triage to Assigned in Hiro Wallet (DEPRECATED) Jul 3, 2023
@markmhendrickson markmhendrickson moved this from Assigned to Triage in Hiro Wallet (DEPRECATED) Jul 3, 2023
@markmhendrickson
Copy link
Collaborator

"I think it would make it a lot easier for users to deal with troublesome transactions.

@314159265359879 We discussed this briefly during the product kick-off meeting yesterday and @kyranjamie was hoping to get some more input here on the particular use cases that this feature would help solve.

Can you provide a short list of the highest frequency issues with transactions that cancelling would resolve?

@314159265359879
Copy link
Contributor Author

I think this is the top 3

  1. User making a mistake and prefer not to have it become part of the immutable history of the Stacks blockchain.
  2. A user wants to send STX/Standard Token they already have a pending transaction for, the effective balance is too low to make the adjustment until they delete the prior transaction.
  3. To delete forever pending transactions (nonce issues/insufficient STX to afford fees, etc).

@markmhendrickson markmhendrickson moved this from Triage to Enhancements backlog in Hiro Wallet (DEPRECATED) Jul 10, 2023
@markmhendrickson markmhendrickson moved this from Enhancements backlog to Triage in Hiro Wallet (DEPRECATED) Jul 10, 2023
@markmhendrickson
Copy link
Collaborator

Upon discussion it appears the most pressing use case here is for canceling transactions with incorrect nonces so they don't linger for days in the mempool and cause downstream problems with subsequent transactions.

@314159265359879
Copy link
Contributor Author

Another pressing use case is removing transactions that influence available balance such as outgoing STX transactions. Sometimes users can't replace a transaction because the wallet doesn't allow it because the available balance is 0 STX.

Cancelling such transactions should take into account balance available after cancelling in order to judge if the fee increase can be covered and if cancelling is possible.

Cancel transactions should create 0.000001 STX transfers to the burn address (Stacks network) with a relatively high fee, to make sure there is a high chance the transaction gets picked up after cancelling. Using the high fee for STX transfers with a cap 1-2 STX seems like a reasonable solution.

For cancelling bitcoin transactions, we may need optionality a higher fee than any other transaction pending to spend the same inputs would have to be the minimum, but typically I think transactions like this should be sent with the high priority fee to give users the best chance of actually cancelling the transaction.

@kyranjamie
Copy link
Collaborator

Rather than some deeply integrated solution, this would be much easier to build if we trigger an extension pop up. This way, we craft the tx, fire, and forget, rather than building an in-app UX flow.

@Aman-zishan
Copy link
Contributor

Aman-zishan commented Jun 4, 2024

@314159265359879 can i work on this?

  • I plan to focus first on working out the solution for stacks transactions and then move on to bitcoin transactions
  • The UX flow will be similar to increase fee
  • To cancel a transaction i plan to create and replace a new transaction that sends 0.000001 STX to the burn address with a 10% increase in fee and a cap of 1-2 STX
  • In mainnet i am planing to use SP000000000000000000002Q6VF78 as burn address, and ST000000000000000000002AMW42H for testnet
  • To address the edge case of available balance i am planning to calculate the effective balance by considering the pending transaction outgoing STX and judge if a cancellation is possible, if not inform the user that they don't have sufficient balance to cover the new increased fee.
  • unit tests

Is there any other details or important edge cases that i need to keep in mind?

@314159265359879
Copy link
Contributor Author

314159265359879 commented Jun 4, 2024

Hi again @Aman-zishan, yes, absolutely, you are welcome to work on this, thanks!

I plan to focus first on working out the solution for stacks transactions and then move on to bitcoin transactions

Excellent

The UX flow will be similar to increase fee

Please consider this comment from Kyran: #3406 (comment)

To cancel a transaction i plan to create and replace a new transaction that sends 0.000001 STX to the burn address with a 10% increase in fee and a cap of 1-2 STX


Ideally the user pick their own fee, low, medium, high or custom as with other transactions.

In mainnet i am planing to use SP000000000000000000002Q6VF78 as burn address, and ST000000000000000000002AMW42H for testnet

Use SP00000000000003SCNSJTCSE62ZF4MSE for mainnet instead, we'll spare the genesis address a little bit from spam ;-)

To address the edge case of available balance i am planning to calculate the effective balance by considering the pending transaction outgoing STX and judge if a cancellation is possible, if not inform the user that they don't have sufficient balance to cover the new increased fee.

We already have some logic to determine available balance based on pending outgoing STX transfers and pending fees. I think you should also consider the transaction being replaced if a user.
Consider this case: if a user has 1000 STX in their wallet, and they have a pending transaction to send 999.9 STX at a 0.1 STX fee, their available balance is 0 STX. They wouldn't be able to cover the minimal fee increase 0.000001 STX but their transaction can still be replaced because when the transaction is cancelled the transfer becomes 0.000001 STX instead of 999.9 STX which leaves more then enough to increase the fee.

unit tests

@Aman-zishan
Copy link
Contributor

Aman-zishan commented Jun 4, 2024

@314159265359879 Thanks! a few clarifications:

✅ Ideally the user pick their own fee, low, medium, high or custom as with other transactions.

In that case i can replicate the current fee selection UI with default set to high?.

Please consider this comment from Kyran: #3406 (comment)

So when a user click on cancel transaction, the extension opens up a new dialog ( similar to increase fee dialog ) , user chooses a fee , on choosing i have to calculate if user can afford replacement and this cancel request wont be shown as a pending transaction is that what is implied?

Use SP00000000000003SCNSJTCSE62ZF4MSE for mainnet instead, we'll spare the genesis address a little bit from spam ;-)

We already have some logic to determine available balance based on pending outgoing STX transfers and pending fees. I think you should also consider the transaction being replaced if a user.
Consider this case: if a user has 1000 STX in their wallet, and they have a pending transaction to send 999.9 STX at a 0.1 STX fee, their available balance is 0 STX. They wouldn't be able to cover the minimal fee increase 0.000001 STX but their transaction can still be replaced because when the transaction is cancelled the transfer becomes 0.000001 STX instead of 999.9 STX which leaves more then enough to increase the fee.

@314159265359879
Copy link
Contributor Author

✅ Ideally the user pick their own fee, low, medium, high or custom as with other transactions.

In that case i can replicate the current fee selection UI with default set to high?.

Setting the default to high is a good idea. That way we increase the likelihood that the transaction is actually replaced.

Please consider this comment from Kyran: #3406 (comment)

So when a user click on cancel transaction, the extension opens up a new dialog ( similar to increase fee dialog ) , user chooses a fee , on choosing i have to calculate if user can afford replacement and this cancel request wont be shown as a pending transaction is that what is implied?

That sounds good one caveat:

We will have to show the transaction in the pending tab; it would be confusing not to. But I understand that "cancel" implies it vanishes, so the user may be confused.

These two additions could help mitigate that confusion: explain and add memo/label.

  1. Show a notice to users that Canceling a transaction isn't guaranteed to work. A higher fee can help replace the old transaction, but sometimes miners pick up a transaction before the new one replaces the old one in all network nodes.
  2. Add a memo to the transfer we make for example "_cancel transaction" That way we can add appropriate labeling in the UI later and/or as a quick fix just add this feature and display the memo for now literally: Show memo for transactions in activity view (only outbound transactions) #2695.

@Aman-zishan
Copy link
Contributor

Aman-zishan commented Jun 6, 2024

@314159265359879 i have finished working on the stacks transactions part, right now working on the integrations tests for this feature, but i noticed that few integration tests are failing (found an issue related to tests unreliability -> #3919) should i add tests anyway even though they are being unreliable? (i verified the working via manual testing with the earlier discussed edge case and can produce a test report in the new PR). I could move on to bitcoin transaction solution if that is the case. Let me know your thoughts

Screenshot 2024-06-06 at 9 48 50 AM

@markmhendrickson
Copy link
Collaborator

should i add tests anyway even though they are being unreliable?

cc @kyranjamie since he created the related Playwright tests unreliability issue

@Aman-zishan
Copy link
Contributor

How can i create a test environment for BTC transactions? even though i switch to testnet the BTC address remains to be mainnet one. leather gitbook seems to be down too

@314159265359879
Copy link
Contributor Author

@Aman-zishan you should be able to access bitcoin testnet(3) when you select this option in the wallet:

image

You should see testnet addresses on the receive button.
image

When adding custom (stacks) testnets there is currently an issue where the mainnet details are used instead. #5228 perhaps that is what you are running into.

Bitcoin testnet 3 is unreliable, constant blockstorm, but you may still be able to utilise it.
image

Work is being done to replace it with a new one public bitcoin testnet (testnet4), I just added an issue here to accommodate that later when it is ready. Right now #5491

@Aman-zishan
Copy link
Contributor

@314159265359879

Work is being done to replace it with a new one public bitcoin testnet (testnet4), I just added an issue here to accommodate that later when it is ready. Right now #5491

In that case, once I get confirmation from @kyranjamie regarding the status of integration tests, I would like to include the Stacks transactions cancel feature in a single PR and focus on BTC transactions in a later PR once I am able to reproduce a good testing environment, if that is okay.

@kyranjamie
Copy link
Collaborator

Ideally we should not be adding more tests that are unreliable, as this might exacerbate the issues we're having.

Do we know why the test in being unreliable?

@Aman-zishan
Copy link
Contributor

@kyranjamie

Ideally we should not be adding more tests that are unreliable, as this might exacerbate the issues we're having.
Do we know why the test in being unreliable?

Not all but a few tests are failing when test selectors are not found
Screenshot 2024-06-07 at 10 46 49 AM

@alter-eggo
Copy link
Contributor

@Aman-zishan have you set WALLET_ENVIRONMENT=testing in .env?

@Aman-zishan
Copy link
Contributor

Aman-zishan commented Jun 7, 2024

@alter-eggo

@Aman-zishan have you set WALLET_ENVIRONMENT=testing in .env?

No, i re ran after setting WALLET_ENVIRONMENT=testing and a TEST_ACCOUNT_SECRET_KEY i still get 21 failed tests
Screenshot 2024-06-07 at 1 35 39 PM

Screenshot 2024-06-07 at 1 37 08 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment