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

TX execution doubles categories/txs on UI while execution is in progress #3215

Closed
liliya-soroka opened this issue Feb 7, 2024 · 4 comments · Fixed by #3710
Closed

TX execution doubles categories/txs on UI while execution is in progress #3215

liliya-soroka opened this issue Feb 7, 2024 · 4 comments · Fixed by #3710
Assignees
Labels
bug Something isn't working

Comments

@liliya-soroka
Copy link
Member

Bug description

Bulk execution doubles categories/txs on UI while execution is in progress

Environment

  • Browser: Chrome
  • Wallet: MetaMask
  • Chain: Sepolia

Steps to reproduce

  1. Add a few txs in queue and sign fully
  2. click bulk execution with relay -> start the process
  3. check the queue view
    Current result:
    The queue list contains:
    pending
    queued
    next
    queued
    categories at the same time and the txs are displayed twice on UI

Expected result

Make sure that the categories and txs are not doubled , as this is quite confusing

Obtained result

Screenshots

image
@liliya-soroka liliya-soroka added the bug Something isn't working label Feb 7, 2024
@github-project-automation github-project-automation bot moved this to New issues in Safe{Wallet} Feb 7, 2024
@liliya-soroka liliya-soroka changed the title Bulk execution doubles categories/txs on UI while execution is in progress TX execution doubles categories/txs on UI while execution is in progress Feb 7, 2024
@liliya-soroka
Copy link
Member Author

Screen.Recording.2024-02-07.at.12.52.34.mov

@TanyaEfremova
Copy link
Contributor

I think, we could bundle these transactions under Pending category. Otherwise, it looks like other transactions are not pending and require action.

@katspaugh
Copy link
Member

The pending queue is shown on top of the regular queue for pending txs in 1/X Safes.

If a transaction from that queue is signed, it will be present also in the normal queue. We should not show signed txs in the pending queue.

@katspaugh katspaugh moved this from New issues to Todo in Safe{Wallet} Mar 25, 2024
@katspaugh katspaugh moved this from Todo to New issues in Safe{Wallet} Apr 2, 2024
@katspaugh katspaugh moved this from New issues to Todo in Safe{Wallet} Apr 25, 2024
@usame-algan usame-algan self-assigned this May 13, 2024
@usame-algan
Copy link
Member

usame-algan commented May 13, 2024

I think ideally we would only have one list of transactions and just switch their status to pending and remove all labels like Next/Queued and only have one label at the top saying Pending. The reason we have a pending queue on top for 1/n safes is because it is possible to immediately execute transactions which don't show up in the normal queue so we have to fetch the untrusted queue to show those.

However, when bulk executing we can be certain that all transactions are fully signed so there is no need to display this pending queue on top. So for a quick fix we could disable the pending queue for bulk execution i.e. if more than one transaction is pending.

For a more general fix we could try only showing pending queue items if they are unsigned (0 of n confirmations) or hide signed transactions from the normal queue if they are pending.

@usame-algan usame-algan moved this from Todo to In Progress in Safe{Wallet} May 13, 2024
@usame-algan usame-algan moved this from In Progress to Ready for QA in Safe{Wallet} May 15, 2024
@francovenica francovenica moved this from Ready for QA to QA in progress in Safe{Wallet} May 21, 2024
@github-project-automation github-project-automation bot moved this from QA in progress to Done in Safe{Wallet} May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants