-
Notifications
You must be signed in to change notification settings - Fork 97
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
Conversation
There was a problem hiding this 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.
src/modules/wallet/components/RegisterMultisigSummary/Summary.js
Outdated
Show resolved
Hide resolved
src/modules/transaction/components/MultiSignatureReview/MultiSignatureReview.js
Outdated
Show resolved
Hide resolved
src/modules/transaction/components/TransactionInfo/TransactionInfo.js
Outdated
Show resolved
Hide resolved
src/modules/transaction/components/TransactionDetails/signedAndRemainingMembersList.js
Outdated
Show resolved
Hide resolved
src/modules/wallet/components/RegisterMultisigSummary/Summary.js
Outdated
Show resolved
Hide resolved
currentAccount, | ||
moduleCommandSchemas, | ||
transactions, | ||
...rest |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
src/modules/wallet/components/RegisterMultisigSummary/Summary.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
What was the problem?
This PR resolves #5354
How was it solved?
How was it tested?