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

(Feature) - #1656 Tx is sent to backend even if not signed#1672

Merged
dasanra merged 9 commits intodevelopmentfrom
feature/1656-tx-not-signed-fix
Dec 9, 2020
Merged

(Feature) - #1656 Tx is sent to backend even if not signed#1672
dasanra merged 9 commits intodevelopmentfrom
feature/1656-tx-not-signed-fix

Conversation

@Agupane
Copy link
Contributor

@Agupane Agupane commented Nov 30, 2020

Closes #1656 by:

  • Checks if the error comes from metamask missing signature before trying to execute the tx
  • Also fixs the black notification when the user rejects the signature

@Agupane Agupane self-assigned this Nov 30, 2020
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 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 1 0
Ignored 0 N/A
  • Result: ✅ success

  • Annotations: 1 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Nov 30, 2020

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

@ghost
Copy link

ghost commented Nov 30, 2020

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

Copy link
Contributor

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

@Agupane, this seems to be the solution for a safe with a threshold of 1.

I think you should check processTransaction as well. If I reject the execution of a tx, it will error without even a notification

failed-to-reject

.encodeABI()
const errMsg = await getErrorMessage(safeInstance.options.address, 0, executeDataUsedSignatures, from)
console.error(`Error creating the TX - an attempt to get the error message: ${errMsg}`)
if (err.code !== REJECT_CONFIRM_TX_ERROR_CODE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I reject the attempt to sign an off-chain tx, this condition is true as the tryOffchainSigning throws an error without code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, was very useful, found also more bugs with notifications on proccessTransaction that I fixed as well

@Agupane Agupane requested a review from fernandomg December 2, 2020 16:26
@ghost
Copy link

ghost commented Dec 2, 2020

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

@ghost
Copy link

ghost commented Dec 2, 2020

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

Copy link
Contributor

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

Nice! Unpopular opinion: I love being able to see the error notification 😛

A thing I stumbled upon was one of those console errors that appear from time to time

Following the stack trace, it was triggered here
image

Can you give it a look and try to fix it in this PR? It will help with #1650

@Agupane
Copy link
Contributor Author

Agupane commented Dec 3, 2020

Nice! Unpopular opinion: I love being able to see the error notification 😛

A thing I stumbled upon was one of those console errors that appear from time to time

Following the stack trace, it was triggered here
image

Can you give it a look and try to fix it in this PR? It will help with #1650

Did you found a way to reproduce this?

@ghost
Copy link

ghost commented Dec 3, 2020

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

@ghost
Copy link

ghost commented Dec 3, 2020

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

@fernandomg fernandomg self-requested a review December 4, 2020 22:31
@fernandomg
Copy link
Contributor

Did you found a way to reproduce this?

@Agupane, I couldn't find a way to reproduce it. But I'd try to avoid assuming that prev will always exist.

I'd go with something around prev?.set('dismissed', true)

@ghost
Copy link

ghost commented Dec 4, 2020

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

@ghost
Copy link

ghost commented Dec 4, 2020

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

@ghost
Copy link

ghost commented Dec 9, 2020

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

@ghost
Copy link

ghost commented Dec 9, 2020

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

@francovenica
Copy link
Contributor

Created a Tx and cancelled the MM prompt when it showed up

Looks good to me. Snapshots for comparison:

In this PR:
image.png
image.png

How it looked before:
image.png
image.png

@dasanra dasanra merged commit 9175552 into development Dec 9, 2020
@dasanra dasanra deleted the feature/1656-tx-not-signed-fix branch December 9, 2020 17:02
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 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.

[Bug] Transaction is sent to backend even if not signed.

4 participants