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

Implement edit multi signature profile #5358

Merged
merged 26 commits into from
Oct 10, 2023

Conversation

eniolam1000752
Copy link
Contributor

@eniolam1000752 eniolam1000752 commented Oct 2, 2023

What was the problem?

This PR resolves #5354

How was it solved?

  • Add ability to get multi signature profile on register multi signature flow
  • Add unit tests

How was it tested?

  • Register a multi-signature account
  • Update the multi-signature account by navigating to the account menu and updating existing members
  • The update transaction should be successfully signed and submitted to the network.

Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

Register multisignature transaction signing is not working as the signatures are placed in root instead of adding inside params during edit flow.

@eniolam1000752 eniolam1000752 marked this pull request as ready for review October 5, 2023 17:28
@ManuGowda ManuGowda removed the request for review from oskarleonard October 6, 2023 09:37
@ikem-legend
Copy link
Member

Unable to register multisig account after all members sign and primary account confirms transaction

Screenshot 2023-10-06 at 12 52 12 Screenshot 2023-10-06 at 12 57 12

@ikem-legend
Copy link
Member

Form values aren't retained between pages
Screenshot 2023-10-06 at 17 51 19
Screenshot 2023-10-06 at 17 54 15
Screenshot 2023-10-06 at 17 54 34

currentAccount,
moduleCommandSchemas,
transactions,
...rest
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just add transactionJSON here instead of ...rest so it is easy to see exactly what this function requires

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reason was because I wanted to preserve the term transactionJSON through in the code snippet, defining it there would mean that I had to define a new variable name which I didn't want to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dont you think it is easier to understand the function just renaming in those cases?
transactionJSON: initialTransactionJSON

This way you see exactly what the function needs (gets help in editors etc...) as well as you know that this var is both modified and used in its initialState.

Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

As discussed, please handle the member signature show and hide based on signature progress.

@ikem-legend
Copy link
Member

Unlike multisignature params signatures, signatures seems out of sync even after all transaction members have signed
Screenshot 2023-10-09 at 12 15 56

ManuGowda
ManuGowda previously approved these changes Oct 9, 2023
Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

LGTM.

Tested

  • Register multisig
  • Update multisig
  • Token transfer from multisig

@eniolam1000752 eniolam1000752 merged commit d76a5a4 into release/3.0.0 Oct 10, 2023
@eniolam1000752 eniolam1000752 deleted the 5354-implement-edit-multi-sig branch October 10, 2023 08:27
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.

4 participants