Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) Executed transactions status#1552

Merged
fernandomg merged 17 commits intodevelopmentfrom
fix/1490-tx-status
Nov 4, 2020
Merged

(Fix) Executed transactions status#1552
fernandomg merged 17 commits intodevelopmentfrom
fix/1490-tx-status

Conversation

@fernandomg
Copy link
Contributor

This PR closes #1490 also, closes #1501, and closes #1342 too, by:

  • extracting the logic to create the "pending tx" and it's corresponding successful status into a pendingTransactions.ts actions file.
  • creating two main actions:
    • storeSignedTx which is the tx to be executed but still not mined, aka "pending tx",
    • storeExecutedTx which will optimistically update the "pending tx" with the tx receipt
  • fixing types
  • fixing decoded values

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Oct 30, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Oct 30, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

1 similar comment
@ghost
Copy link

ghost commented Oct 30, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

awaiting_execution: 'Awaiting execution',
pending: 'Pending',
}
cacatua: 'Cacatúa',
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I ask? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can leave it and do this

if (Math.random() < 0.01) {
  status = status.cacatua
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahaha 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks! 🐦

@ghost
Copy link

ghost commented Nov 2, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 2, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

Copy link
Contributor

@Agupane Agupane left a comment

Choose a reason for hiding this comment

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

💯 🚀

import { makeConfirmation } from 'src/logic/safe/store/models/confirmation'
import { storeExecutedTx, storeSignedTx, storeTx } from 'src/logic/safe/store/actions/transactions/pendingTransactions'

const processTransaction = ({ approveAndExecute, notifiedTransaction, safeAddress, tx, userAddress }) => async (
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can type this too that would be nice, if results complicated just leave it for later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ghost
Copy link

ghost commented Nov 2, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

Copy link
Contributor

@matextrem matextrem left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pablofullana pablofullana requested a review from dasanra November 3, 2020 14:51
@ghost
Copy link

ghost commented Nov 3, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 3, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 3, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 3, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 4, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 4, 2020

Travis automatic deployment:
https://pr1552--safereact.review.gnosisdev.com/volta/app

@francovenica
Copy link
Contributor

Stats looks good now.
For 1 owner it will show the tx right away with the pending status

For 2 or more owners. The mock transaction has been gone for a while so after a few secs the tx just show up with the "Awaiting confirmations" status.
When a the final signature is being done, the pending status shows next to the owners address and in the tx status.
Same as before happens for a rejection: the first signature just won't show a change in status, the last signature will show the person who signs and the tx itself with the "Pending" status.

If there are 3 or more people. Any tx signed "off-chain" won't trigger a change in the tx status.

Looks good to me

@fernandomg fernandomg merged commit 7a88153 into development Nov 4, 2020
@fernandomg fernandomg deleted the fix/1490-tx-status branch November 4, 2020 22:41
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Pending" status missing when tx is being process Tx's show wrong status sometimes Refactor Pending transaction implementation

7 participants