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

Conversation

@katspaugh
Copy link
Member

@katspaugh katspaugh commented Feb 21, 2022

What it solves

Dima noticed the following bug:

  • connect the mobile app via WalletConnect
  • initiate a transaction on web (Execute checkbox unchecked)
  • reject on mobile
  • before the fix: web app redirects to the tx with 0 signatures 🐞

Another bug this PR fixes:

  • make a Safe an owner of another Safe
  • connect the first Safe to the owned safe via WC and WC Safe App
  • create a tx (Execute unchecked)
  • before the fix: tx created with 0 signatures 🐞

After this fix, a tx will be signed correctly in both cases.

How this PR fixes it

When a created tx is rejected by the wallet, we throw an error and show an error snackbar. The tx is not created and the user is not redirected anywhere.

Also checks if the connected wallet is a smart contract and does an on-chain signature if yes.

@katspaugh katspaugh requested a review from iamacook February 21, 2022 11:26
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@katspaugh katspaugh changed the title Fix: throw when wallet rejects a created tx Fix: throw when WC rejects a created tx Feb 21, 2022
@github-actions
Copy link

github-actions bot commented Feb 21, 2022

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

@github-actions
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

@github-actions
Copy link

github-actions bot commented Feb 21, 2022

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1877188890

Failed tests:

  • ❌ Safe Apps List Safe Apps List

@coveralls
Copy link

coveralls commented Feb 21, 2022

Pull Request Test Coverage Report for Build 1877142004

  • 11 of 13 (84.62%) changed or added relevant lines in 1 file are covered.
  • 25 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 34.285%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/logic/safe/store/actions/createTransaction.ts 11 13 84.62%
Files with Coverage Reduction New Missed Lines %
src/logic/safe/transactions/offchainSigner/EIP712Signer.ts 12 34.69%
src/logic/safe/transactions/offchainSigner/index.ts 13 15.15%
Totals Coverage Status
Change from base Build 1877126792: -0.3%
Covered Lines: 3299
Relevant Lines: 8630

💛 - Coveralls

Copy link
Contributor

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Sleek approach. Definitely needs testing with our supported hardware wallets though

@katspaugh katspaugh changed the title Fix: throw when WC rejects a created tx Fix: throw when WC rejects a created tx + on-chain signing with a Safe signer via WC Feb 21, 2022
@katspaugh katspaugh added the Bug 🐛 Something isn't working label Feb 21, 2022
@iamacook
Copy link
Contributor

@usame-algan, I think your TxSender tests need to be adjusted because of these changes.

@liliya-soroka
Copy link
Member

liliya-soroka commented Feb 21, 2022

The issue is still exist. Steps:

  1. add safe 1 as an owner for safe2
  2. open safe 2 on web and safe 1 on mobile
  3. click connect via WC on web
  4. Scan web WC code on mobile in dapps section
  5. after connection create tx on web
  6. confirm it on mobile
    Current result: the tx is created but with 0 of 2 confirmations

image

  1. Deeplink for the tx exists - https://pr3546--safereact.review-safe.gnosisdev.com/app/rin:0xf2565317F3Ae8Ae9EA98E9Fe1e7FADC77F823cbD/transactions/0x690af3a733dd2aa57f4912e4cdd1779c887af779661856d7a8fcba27f1bff211
  2. But the queued txs list is empty - https://pr3546--safereact.review-safe.gnosisdev.com/app/rin:0xf2565317F3Ae8Ae9EA98E9Fe1e7FADC77F823cbD/transactions/queue

@katspaugh
Copy link
Member Author

katspaugh commented Feb 22, 2022

@liliya-soroka I've just tested this, and I think the confusing part is that you need to separately execute the confirmation (on-chain signature) transaction on mobile. Until you do that, the queued tx won't have signatures.

@liliya-soroka
Copy link
Member

Verified - the both cases are fixed.

@katspaugh katspaugh merged commit b7695d1 into dev Feb 22, 2022
@katspaugh katspaugh deleted the fix-rejection branch February 22, 2022 10:19
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2022
@francovenica
Copy link
Contributor

francovenica commented Feb 24, 2022

Just for clarification: You need this notification (see snapshot) to show up in the "safe owner" before the tx stops showing the "0 out of x" signatures in the tx in the "safe own"
image

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

Labels

Bug 🐛 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants