Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Handle cancel state for ethereum txs #1532

Merged
merged 5 commits into from
Nov 23, 2021

Conversation

ramirotw
Copy link
Contributor

@ramirotw ramirotw commented Oct 7, 2021

Summary

Closes #1382 #1435

This adds a Cancelling and Cancelled states for ethereum transactions in the Recent Activity list. Before cancelled txs were removed from the list when they were confirmed.

The changes are built on top of @anxolin's PR #1429 as I'm making use of the new EnhancedTransactionDetails type and also to reduce conflicts due to changes in the enhancedTransactions state files.

image

To Test

  1. Run an onchain transaction (wrap eth or approve) with very low gas fees
  2. Cancel it through MM
  3. The transaction log in the Recent Activity list should change to Cancelling and then Cancelled when confirmed

@ramirotw ramirotw added the Protofire Handled by Protofire development team label Oct 7, 2021
@ramirotw ramirotw requested review from alfetopito and anxolin October 7, 2021 13:20
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2021

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Awesome, working perfectly!

@anxolin anxolin force-pushed the presign/enhanced-transactions branch from 030864b to 5b32325 Compare October 7, 2021 18:23
@ramirotw ramirotw changed the base branch from presign/enhanced-transactions to presign/base October 7, 2021 18:51
@ramirotw ramirotw force-pushed the ramirotw/issue-1382-cancelled-txs branch from a563197 to 86411b9 Compare October 7, 2021 18:53
@elena-zh
Copy link

elena-zh commented Oct 8, 2021

Hi there!
I do not what I'm doing wrong, but I can't verify this issue, due to each time when I cancel a transaction, I get it stuck in 'Open' status. Each of these transaction navigates to a not found transaction log
image
Approve:
https://rinkeby.etherscan.io/tx/0x86aa24b660c876ade79f1bdcca830b047ab323634747ebed3c958a8a15c348f9
Unwrap: https://rinkeby.etherscan.io/tx/0x1f05656a69f4ec33d0c2b6e4c51eac09398ae92a83316182b2bd1e00d3ba1cd9
Wrap: https://rinkeby.etherscan.io/tx/0x70875d4dd9f9a86d66d075a6705846b14202a5fc25f25455bb53c0fc561ab02a

Transaction logs in MM:
image

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Code-wise looks good!

I haven't tested the edge cases though. But I know there's enough eyes :)

@alfetopito
Copy link
Contributor

Hi there! I do not what I'm doing wrong, but I can't verify this issue, due to each time when I cancel a transaction, I get it stuck in 'Open' status. Each of these transaction navigates to a not found transaction log image Approve: https://rinkeby.etherscan.io/tx/0x86aa24b660c876ade79f1bdcca830b047ab323634747ebed3c958a8a15c348f9 Unwrap: https://rinkeby.etherscan.io/tx/0x1f05656a69f4ec33d0c2b6e4c51eac09398ae92a83316182b2bd1e00d3ba1cd9 Wrap: https://rinkeby.etherscan.io/tx/0x70875d4dd9f9a86d66d075a6705846b14202a5fc25f25455bb53c0fc561ab02a

Transaction logs in MM: image

My guess is that this is related to my changes to blocknative api key.
Needs to have this change merged in #1534 and a manual setting change on blocknative

I'll do that and let you know when you can test it

@alfetopito
Copy link
Contributor

@ramirotw or whoever takes over this PR

I've added the PR URL to the list of allowed origins from blocknative
All it needs to work now is to have these 3 PRs synced.

@elena-zh
Copy link

Hey @alfetopito , @ramirotw , can I retest it?

@alfetopito
Copy link
Contributor

Hey @alfetopito , @ramirotw , can I retest it?

Not yet, need to resolve the conflicts with the base branch

@ramirotw ramirotw force-pushed the ramirotw/issue-1382-cancelled-txs branch from 7d8f457 to e5ab265 Compare October 26, 2021 15:00
@ramirotw
Copy link
Contributor Author

Not yet, need to resolve the conflicts with the base branch

@alfetopito @elena-zh I rebased the branch and solved all the conflicts

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Cancellation is working, I can see the "cancelling" state, but the final state is "Confirmed"

screenshot_2021-10-26_10-52-22

This is the original wrap tx https://rinkeby.etherscan.io/tx/0xf5d4cf1c03436b6e2f76a3fb67f912b5d7750bd157e4d8f7e317f8015a337f39
And this is the cancellation tx which overwrote that one https://rinkeby.etherscan.io/tx/0x8de6d7df50b61024aa029e29e4a6cfd4f85559b5e7883e8fc04f028c63cf5f00

We might need to store something in the local state to indicate the successful transaction was actually a cancellation.
Or can we get that info from the executed tx?
Maybe we can use the tx input data (0x) to detect it is a cancel transaction?

@elena-zh
Copy link

As @alfetopito I see 'Confirmed' state for wrap/unwrap transactions when they are cancelled
cancel
View details link for these transactions navigates to the 404 error page. However, transaction link on the toast notification navigates to the successful 'Cancel' transaction.

Besides, changes are not applied to 'Approve' toke transaction: cancelled transaction stays in 'Open' status forever
https://watch.screencastify.com/v/Uj6EWIj5bBA0vNv8oxLD

@elena-zh
Copy link

I have retested changes.
Now I can run 'Approve' transactions.
But still I see 'Confirmed' status instead of 'Cancelled', and transaction details navigate to 404 page https://rinkeby.etherscan.io/tx/0x00e028bdb358f5f813e452ce10e04db610d57ec84fabe9a39ae1ec696423e6fd

Also, I got 'Cancelling' forever transaction when it is successfully cancelled:
pending

However, transaction details details navigate to success transaction https://rinkeby.etherscan.io/tx/0x8bd46631aca1617fcc764c1bdeb5634ac3276317322d16578d4884e8ad5c1c01

@ramirotw ramirotw changed the base branch from presign/base to develop November 17, 2021 18:12
@ramirotw
Copy link
Contributor Author

But still I see 'Confirmed' status instead of 'Cancelled', and transaction details navigate to 404 page

@elena-zh please re test

@elena-zh
Copy link

Hey @ramirotw , thanks!
Now I'm able to see "Cancelling' and 'Cancelled' statuses for transactions.

However, I have faced 2 issues during testing changes.

  1. I got a stuck 'Approve' transaction in 'Cancelling' state. View transaction details navigate to 404 error page:
    pending cancelling
    Navigation from Cowswap: https://rinkeby.etherscan.io/tx/0x9bf9ee1ecb2d2c54666abde3d1a1ddd0cb6babe2ea570ed67adc176c1811884a
    Navigation from the wallet: https://rinkeby.etherscan.io/tx/0x3fd3df9acdf0c1adcda09f66dc6146769c562ce74b2c8e359d49ae2966fb3ddb
    Transaction's history from the wallet
    image

  2. The case might not be related to the existing one, but I was able to reproduce it: I got failed transaction in stuck 'Pending' state. But before it failed, I tried to speed it up. The issue might be related to Cancelled and speeded up wrap/unwrap transactions hang in 'Open' status #1410 case, though...
    failed as pending
    Transaction details from the CowSwap: https://rinkeby.etherscan.io/tx/0x4f0f4497751252f4c03ae77fdeaf57126123e5a2ae0856ca25c61681906d042b
    Transaction details from the wallet: https://rinkeby.etherscan.io/tx/0x4f0f4497751252f4c03ae77fdeaf57126123e5a2ae0856ca25c61681906d042b

@elena-zh
Copy link

If I'm not mistaken, all described issues might be related to #1832 task.
If yes, changes under the current PR LGTM. 🙂

@ramirotw
Copy link
Contributor Author

If I'm not mistaken, all described issues might be related to #1832 task.
If yes, changes under the current PR LGTM. 🙂

You are right, this is probably related to the race condition issue already in #1832

@ramirotw ramirotw merged commit 02fd6cb into develop Nov 23, 2021
@ramirotw ramirotw deleted the ramirotw/issue-1382-cancelled-txs branch November 23, 2021 18:10
@ramirotw ramirotw mentioned this pull request Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Protofire Handled by Protofire development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not remove cancelled txs from recent activities
5 participants