-
Notifications
You must be signed in to change notification settings - Fork 73
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
multi-level msig required cosignatures, txs issues fixed, signer selector&filter tree view, misc. bugfixes #1698
multi-level msig required cosignatures, txs issues fixed, signer selector&filter tree view, misc. bugfixes #1698
Conversation
635b073
to
bea6972
Compare
…fix: partial txs cannot be shown consistently issue fixed
bea6972
to
a427920
Compare
@yilmazbahadir all looks good for me 👍 |
@@ -736,7 +736,7 @@ export class FormTransferTransactionTs extends FormTransactionBase { | |||
this.networkType, | |||
this.networkConfiguration, | |||
this.transactionFees, | |||
this.currentSignerMultisigInfo ? this.currentSignerMultisigInfo.minApproval : this.selectedSigner.requiredCosignatures, | |||
this.currentSignerMultisigInfo ? this.currentSignerMultisigInfo.minApproval : this.selectedSigner.requiredCosigApproval, |
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 using the selectedSigner.requiredCosigApproval
directly for both cases same as FormMetadataCreationTs.ts?
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.
Thanks, just refactored this line.
After the signer
changes in place, I think most of the places using currentSignerMultisigInfo
can be refactored to use the signer
instead. I just didn't want to make this PR too big and introduce too much change. But I think this line could be change, tested locally seems fine.
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.
I agree, We should do the other places in a separate PR.
…ignerMultisigInfo
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
fixes #1687 , #1688, #1697
Changes:
Signer
class holds references to its parents as a tree (parentSigners
), allSigner
'srequiredCosigApproval
andrequiredCosigRemoval
s are pre-calculated while constructing the tree. This info is used to decide on creating AGGREGATE_COMPLETE when there is only 1 cosigner needed to approve(minApproval === 1
).SignerSelector and SignerFilter dropdowns show signers as grouped and in a tree structure (starting from the leave(current account) going down to the root signer (supporting multiple, multi-level multisig structures)
SignerSelector and SignerFilter components refactored to use the same base and became =>
SignerSelector
,SignerListFilter
=> extending/usingSignerBaseFilter
Several bugs noticed during the testing are fixed.