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

Feat: Multisig creation with remark #1800

Open
wants to merge 105 commits into
base: tbaut-remove-matrix-3rd
Choose a base branch
from

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented May 27, 2024

closes #1756
closes #1807

Copy link
Contributor

github-actions bot commented May 27, 2024

Jest Unit tests results

Generic badge

Duration: 68.689 seconds
Start: 2024-10-09 09:19:58.453 UTC
Finish: 2024-10-09 09:21:07.142 UTC
Duration: 68.689 seconds
Outcome: Passed | Total Tests: 740 | Passed: 739 | Failed: 0
Total Test Suites: 157
Total Tests: 740
Failed Test Suites: 0
Failed Tests: 0
Passed Test Suites: 156
Passed Tests: 739
Pending Test Suites: 1
Pending Tests: 1

@Tbaut Tbaut requested a review from tuul-wq May 29, 2024 12:49
@Tbaut Tbaut marked this pull request as ready for review May 29, 2024 12:49
@Tbaut
Copy link
Collaborator Author

Tbaut commented May 29, 2024

There are still quite some things to iron out, such as error management for the form submission, UI glitches, and I'm investigating an issue I found in the multisig indexer, but I'd like to have your opinion on things such as:

  • do we need to support proxy signers
  • should I organize models differently (right now there's 1 for the flow, 1 for most of the data, 1 for the confirmation). I basically took example on the "add proxy" flow

@Tbaut Tbaut requested review from Asmadek and sokolova-an May 30, 2024 06:57
@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 1, 2024

@pgolovkin I addressed your comments in the last commit.

  1. I now take the proxy url when the network has the multisig option
  2. The alerts are only displayed after the user made a mistake and clicked on next
  3. I now display all additional costs on all screens
  4. My screen is now very close to the mock up. The only difference is that it's larger. The reason I think it's better this way, is so that I can re-utilize the nice "Sign with XYZ" button. And this together with the "additional costs" really doesn't look good on small modal, e.g
    image

When the user successfully signs system_remark operation then the new wallet has to be selected and perations page has to be opened. As I remember it's a common flow for multisig operations, please check it.

I need help, maybe @Asmadek @sokolova-an or @johnthecat can tell me how to do that.

@pgolovkin
Copy link
Collaborator

@pgolovkin I addressed your comments in the last commit.

  1. I now take the proxy url when the network has the multisig option
  2. The alerts are only displayed after the user made a mistake and clicked on next
  3. I now display all additional costs on all screens
  4. My screen is now very close to the mock up. The only difference is that it's larger. The reason I think it's better this way, is so that I can re-utilize the nice "Sign with XYZ" button. And this together with the "additional costs" really doesn't look good on small modal, e.g
    image

When the user successfully signs system_remark operation then the new wallet has to be selected and perations page has to be opened. As I remember it's a common flow for multisig operations, please check it.

I need help, maybe @Asmadek @sokolova-an or @johnthecat can tell me how to do that.

  1. ok
  2. ok
  3. ok
  4. let's clarify with designer

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 3, 2024

I've adapted the confirmation step to the new design.
The only difference I kept on purpose is the conversion in $ value present on my implementation but not on the design, because I think it brings value to users
image

@Tbaut Tbaut requested a review from pgolovkin October 3, 2024 11:10
@pgolovkin
Copy link
Collaborator

@Tbaut looks great!
Could you please check how the following scenario works:

  1. The user selects more then 1 accounts that the user owns (1 is polkadot vault, 1 is nova wallet)
  2. The user proceed
  3. Which account will be selected for signing?

Also please check with Sergy which popup has to be used for showing signatories on the confirmation screen. For example for staking it looks like on the screenshot
image

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 4, 2024

I implemented the modal for signatories instead of a tooltip:
image

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 4, 2024

The user selects more then 1 accounts that the user owns (1 is polkadot vault, 1 is nova wallet)
The user proceed
Which account will be selected for signing?

To answer here, what I answered in tg:
For now, the first account selected has to be an account that is owned, and this is the default signer. Let's discuss the design if you want me to add/change something.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 7, 2024

I added the signer selection, with a "Back" button and without the token info for now. I need some more info to manage the multi-shard accounts. I pinged @johnthecat to have a chat.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 9, 2024

Now showing the Wallet rather than the accounts @pgolovkin in the signer selection.
image

@pgolovkin
Copy link
Collaborator

@Tbaut looks great! Thanks!

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.

Redo the multisig creation to create a system.remark call
5 participants