Implemented the Transaction class #366
Conversation
src/transaction.ts
Outdated
| tx.applySignature(transaction.signature); | ||
| } | ||
|
|
||
| if (transaction.guardianSignature) { |
There was a problem hiding this comment.
The checks for transaction.signature and transaction.guardianSignature are not symmetric.
There was a problem hiding this comment.
Indeed, they were not. Fixed.
| @@ -458,3 +492,215 @@ export class TransactionHash extends Hash { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * An abstraction for creating, signing and broadcasting transactions. | |||
| * Will replace the {@link Transaction} class in the future. | |||
There was a problem hiding this comment.
Indeed, this is the one according to sdk-specs 👍
src/transaction.ts
Outdated
| this.gasLimit = gasLimit; | ||
| this.data = data || new Uint8Array(); | ||
| this.chainID = chainID; | ||
| this.version = version || TRANSACTION_OPTIONS_DEFAULT; |
There was a problem hiding this comment.
Version vs. options.
| receiver: Address.fromBech32(transaction.receiver), | ||
| gasLimit: new BigNumber(transaction.gasLimit).toNumber(), | ||
| chainID: transaction.chainID, | ||
| value: new BigNumber(transaction.value).toFixed(0), |
src/transaction.ts
Outdated
| constructor() {} | ||
|
|
||
| computeTransactionFee(transaction: ITransactionNext, networkConfig: INetworkConfig): BigNumber{ | ||
| let moveBalanceGas = new BigNumber( |
There was a problem hiding this comment.
When possible, we should use const instead of let (optional, can also be done in other PR).
There was a problem hiding this comment.
Replaced with const where possible.
src/transaction.ts
Outdated
| .update(buffer) | ||
| .digest("hex"); | ||
|
|
||
| return new Uint8Array(Buffer.from(hash, "hex")); |
There was a problem hiding this comment.
Generally speaking, the following:
return new Uint8Array(Buffer.from(hash, "hex"))
Can be replaced by:
return Buffer.from(hash, "hex")
(optional, shorter)
There was a problem hiding this comment.
Indeed, replaced it.
src/transaction.ts
Outdated
| chainID: transaction.chainID, | ||
| version: transaction.version, | ||
| options: transaction.options == 0 ? undefined : transaction.options, | ||
| guardian: transaction.guardian == "" ? undefined : transaction.guardian, |
There was a problem hiding this comment.
Check can also be: transaction.guardian ? undefined : transaction.guardian, so that it handles null values, as well. CC: @CiprianDraghici, makes sense? I think for options, as well. Since setters are public.
There was a problem hiding this comment.
Check is now:
transaction.guardian ? transaction.guardian : undefinedSame for options.
src/transaction.ts
Outdated
| receiverUsername: transaction.receiverUsername ? Buffer.from(transaction.receiverUsername).toString("base64") : undefined, | ||
| gasPrice: Number(transaction.gasPrice), | ||
| gasLimit: Number(transaction.gasLimit), | ||
| data: transaction.data.length == 0 ? undefined : Buffer.from(transaction.data).toString("base64"), |
There was a problem hiding this comment.
Since the setter is public, here we can also protect against a null / undefined data field.
| MinGasLimit: number; | ||
| GasPerDataByte: number; | ||
| GasPriceModifier: number; | ||
| ChainID: string; | ||
|
|
There was a problem hiding this comment.
Now I see that at some point we have to have consistent naming for public fields. Here, uppercase. For TransactionNext, the first letter is lowercase.
All right for now, no need to rename the fields of NetworkConfig in v13.
There was a problem hiding this comment.
Not really sure what you mean. The TransactionComputer uses the same INetworkConfig interface as the Transaction class where the first letter is uppercase.
There was a problem hiding this comment.
But I do agree that we need to stay consistent with the naming.
Implemented the new
Transactionclass as described in the specs.