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

[WIP] [wallet/mobile] feat: add new Aggregate Bonded cosignature UI flow #327

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

OlegMakarenko
Copy link
Contributor

@OlegMakarenko OlegMakarenko commented Oct 4, 2022

  • Disallow signing transactions from unknown senders (signers). Provide options to block or whitelist the sender.
  • Show warning (the yellow one) for txs from known senders (account is a part of the multisig tree).
  • Don't show a warning, only the "sign" button for txs from trusted senders (the ones which are in the address book's whitelist).
  • Disallow signing tx from blocked signers. Show button to view contact.
  • Refresh transaction list when an error occurs in the AggregateTransactionDetails component (failed to fetch tx, expired).
  • Refactor AggregateTransactionDetails component.
  • Update copy.
  • Fetch full multisig tree.

Temporary branch which contains both changes Address Book and Cosign Form: https://github.com/symbol/mobile-wallet/tree/address-book-bonded-merged

@cryptoBeliever
Copy link
Contributor

cryptoBeliever commented Oct 13, 2022

@OlegMakarenko when I'm signing on mobile transaction announced by the cosigner from the desktop wallet I got an error like this (on branch address-book-bonded-merged, on address-book-bonded-2 it works):

image

@OlegMakarenko
Copy link
Contributor Author

@cryptoBeliever can you please pull latest changes?

@cryptoBeliever
Copy link
Contributor

cryptoBeliever commented Oct 17, 2022

@OlegMakarenko thank you. It's fixed now. What I noticed regarding signing flow:

  1. Transaction sent by account that is in multisig relation with our account:
  • block sender should be not available (desktop version screenshot to compare)
  • Unknown transaction awaiting signature. should be replaced by Please carefully review all amounts and recipient addresses, as transactions are not reversible.

mobile:
image
desktop:
image

  1. Transaction sent by account that is not in multisig relation with our account:
  • I would replace "Unknown transaction awaiting signature." with something else.
  • In the desktop wallet, we don't have "Add to whitelist" shortcut (next view). Have you added it on purpose?
  • I would add a "Learn more about common hacks and scams" link

mobile:
image

desktop:
image

  1. When we have a "trusted" (whitelisted) transaction and when we click "Sign" we are signing without further notification if:
  • we don't have pin enabled
  • we have face id enabled (it's automatically accepted)

image

@OlegMakarenko OlegMakarenko linked an issue Oct 26, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History view - presentation for account selector
2 participants