-
Notifications
You must be signed in to change notification settings - Fork 45
Add multisig controller #597
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
Conversation
| private readonly abi: Abi; | ||
|
|
||
| constructor(options: { abi: Abi }) { | ||
| this.abi = options?.abi; |
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 don't think this is necessary.
src/multisig/resources.ts
Outdated
| this.type = MultisigActionEnum.SendTransferExecuteEsdt; | ||
| this.receiver = data.to; | ||
| this.tokens = data.tokens.map( | ||
| (token: { token_identifier: any; nonce: any; amount: any }) => |
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.
Can we have the types more precise? e.g. token_identifier: string, nonce: number, amount: bigint.
| transaction.guardian = options.guardian ?? Address.empty(); | ||
| transaction.relayer = options.relayer ?? Address.empty(); | ||
| transaction.nonce = nonce; | ||
| this.setTransactionGasOptions(transaction, options); |
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.
here and for the other controllers, we should also handle transaction options for guarded transactions and for hash signing.
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'll do this in a different pr
| /** | ||
| * Creates a transaction for deploying a new multisig contract | ||
| */ | ||
| async createTransactionForMultisigDeploy( |
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.
we should also implement parseDeploy, and awaitCompletedDeploy.
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'll do that in the next pr
| /** | ||
| * Awaits the completion of a propose add board member action | ||
| */ | ||
| async awaitCompletedProposeAddBoardMember(txHash: string): Promise<number> { |
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.
on sdk-py, I simply added one parse_propose_action method that can be used to get the proposal id of any proposal, instead of having separate methods for each proposal.
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.
next pr
| private multisigFactory: MultisigTransactionsFactory; | ||
| private multisigParser: MultisigTransactionsOutcomeParser; | ||
|
|
||
| constructor(options: { chainID: string; networkProvider: INetworkProvider; abi: Abi }) { |
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.
we should also have an optional addressHrp parameter, that can be used instead of altering the LibraryConfig.
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.
To be discussed, if decided so will do in a different pr
|
|
||
| /** | ||
| * Awaits the completion of a propose SC upgrade from source action | ||
| * Awaits the completion of a propose Contract upgrade from source action |
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.
Titlecase not needed; here and above etc.
| private readonly argSerializer: ArgSerializer; | ||
|
|
||
| constructor(options: { config: TransactionsFactoryConfig; abi?: Abi }) { | ||
| constructor(options: { config: TransactionsFactoryConfig; abi: Abi }) { |
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.
👍
| this.receiver = data.to; | ||
| this.tokens = data.tokens.map( | ||
| (token: { token_identifier: any; nonce: any; amount: any }) => | ||
| (token: { token_identifier: string; nonce: bigint; amount: bigint }) => |
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.
Above, classes aren't separated by newlines (formatting).
No description provided.