Add multisig transaction factory #595
Conversation
src/multisig/resources.ts
Outdated
| egldAmount: bigint; | ||
| gasLimit?: bigint; | ||
| tokenTransfers: TokenTransfer[]; |
There was a problem hiding this comment.
Should we have egldAmount and tokenTransfers as optionals? Just so the user understands they are not both mandatory. In case neither are provided we throw an error.
src/multisig/resources.ts
Outdated
|
|
||
| export type ProposeTransferExecuteInput = MultisigContractInput & { | ||
| to: Address; | ||
| egldAmount: bigint; |
There was a problem hiding this comment.
we usually use nativeTokenAmount.
| NativeSerializer.nativeToTypedValues( | ||
| [options.to, argsTyped, options.gasLimit, VariadicValue.fromItems(...input.functionCall)], | ||
| this.abi?.getEndpoint("proposeTransferExecuteEsdt") ?? | ||
| new EndpointDefinition("proposeTransferExecuteEsdt", [], [], new EndpointModifiers("", [])), |
There was a problem hiding this comment.
Does this work if the Abi is not provided?
Perhaps you could convert all values to typed values so the abi would not be needed.
There was a problem hiding this comment.
not, but I'll do this in a different pr
| for (let index = 1; index < functionCallParts.length; index++) { | ||
| const element = functionCallParts[index]; | ||
| functionArguments.push(element.valueOf()); | ||
| } |
There was a problem hiding this comment.
As below, in case Array.slice can be used.
| * Use this class to create multisig related transactions like creating a new multisig contract, | ||
| * proposing actions, signing actions, and performing actions. | ||
| */ | ||
| export class MultisigTransactionsFactory extends SmartContractTransactionsFactory { |
There was a problem hiding this comment.
Inheriting vs. doing "composition" - debatable; but here, indeed, using SmartContractTransactionsFactory as a base class seems the better choice 👍
| }).build(); | ||
| } | ||
|
|
||
| private mapTokenPayment(options: resources.ProposeTransferExecuteEsdtInput) { |
There was a problem hiding this comment.
Can be plural, since there are more payments.
Also, we should specify the return type.
| const argsTyped = []; | ||
| for (const token of options.tokens) { | ||
| argsTyped.push({ | ||
| token_identifier: new TokenIdentifierValue( |
There was a problem hiding this comment.
When ABI is available, I think the following is sufficient:
{
token_identifier: tokenComputer.extractIdentifierFromExtendedIdentifier(token.token.identifier),
token_nonce: token.token.nonce,
amount: token.amount
}
When the ABI isn't available, this does not work at all (as far as I remember), since a StructValue is required - a mere {} object does not suffice.
Additional comments about having the ABI as required or not can be seen in the PR with the controller implementation, thus we can leave this as it is in the current PR.
These being said, argsTyped can also be renamed (here, the args aren't completely typed).
| }); | ||
| } | ||
|
|
||
| private static getFunctionCall(transaction: Transaction) { |
There was a problem hiding this comment.
We should add a comment to explain the workaround / hack.
In the multisig transactions factory, we leverage the generic smart contract transactions factory to properly encode the arguments (given the ABI) for the "other" contract execute proposal. However, since we have to handle them as
variadic<bytes>when preparing the "top-level" multisig transaction, here we split back the data.
To-do for a future PR: convert the arguments to a list of buffers using the ArgSerializer or a SmartContractTransactionsFactory.argsToDataParts(), instead of the <create transaction using factory, then parse back into list of buffers> hack.
There was a problem hiding this comment.
As discussed, this is ok for the moment as long as we add the comment. Will perhaps refactor later if needed.
| assert.instanceOf(transaction, Transaction); | ||
| assert.equal(transaction.sender.toBech32(), senderAddress.toBech32()); | ||
| assert.equal(transaction.receiver.toBech32(), multisigContractAddress.toBech32()); | ||
| assert.isAbove(transaction.data.length, 0); |
There was a problem hiding this comment.
not sure this check is necessary since we assert on the data field bellow. not a big deal anyways
danielailie
dismissed stale reviews from popenta and andreibancioiu
via
721eb9d
No description provided.