Update transaction factories to match the sdk-specs #339
Conversation
src/smartcontracts/smartContract.ts
Outdated
| config: config, | ||
| abi: this.abi | ||
| }); | ||
|
|
||
| const bytecode = Uint8Array.from(Buffer.from(code.toString(), 'hex')); | ||
| const metadataAsJson = this.getMetadataPropertiesAsObject(codeMetadata); | ||
|
|
||
| const intent = scIntentFactory.createTransactionIntentForUpgrade({ | ||
| const intent = scIntentFactory.createTransactionForUpgrade({ |
There was a problem hiding this comment.
Transaction.fromDraft function could be used here. Try to see where else can be used
| @@ -33,6 +33,7 @@ describe("test smart contract interactor", function () { | |||
| let interaction = new Interaction(contract, dummyFunction, []); | |||
|
|
|||
| let transaction = interaction | |||
| .withSender(alice.address) | |||
| sender: IAddress; | ||
| tokenIdentifier: string; | ||
| supplyToMint: BigNumber.Value; | ||
| }): TransactionIntent { | ||
| supplyToBurn: BigNumber.Value; |
| ...(options.canChangeOwner ? [utf8ToHex("canChangeOwner"), this.trueAsHex] : []), | ||
| ...(options.canUpgrade ? [utf8ToHex("canUpgrade"), this.trueAsHex] : []), | ||
| ...(options.canAddSpecialRoles ? [utf8ToHex("canAddSpecialRoles"), this.trueAsHex] : []), | ||
| ...[utf8ToHex("canUpgrade"), utf8ToHex(String(options.canUpgrade).toLowerCase())], |
There was a problem hiding this comment.
Can we use the following, instead?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean/toString
E.g. new Boolean(...), or directly options.canUpgrade.toString().
There was a problem hiding this comment.
Or we can even define a boolToHex function, near utf8ToHex and so on.
There was a problem hiding this comment.
Created a boolToHex() function.
| @@ -1,7 +1,7 @@ | |||
| import { BigNumber } from "bignumber.js"; | |||
| import { DEFAULT_HRP } from "../constants"; | |||
|
|
|||
| export class TransactionIntentsFactoryConfig { | |||
| export class TransactionsFactoryConfig { | |||
There was a problem hiding this comment.
Thinking out loud - do we need chainID?
There was a problem hiding this comment.
As discussed, we will keep the chainID.
| data: new TransactionPayload(Buffer.from(intent.data!)), | ||
| chainID: chainID | ||
| }); | ||
| let transaction = Transaction.fromDraft(draftTx); |
There was a problem hiding this comment.
Can also be const (even if we mutate its inner state).
| assert.equal(transaction.getGasLimit().valueOf(), 56000); | ||
| assert.equal(transaction.getValue().toString(), "1000000000000000000"); | ||
| assert.equal(transaction.getData().toString(), "test"); | ||
| assert.equal(transaction.getChainID().valueOf(), ""); |
There was a problem hiding this comment.
Perhaps also add asserts for the other empty fields (e.g. nonce, gas price, signature etc.)?
| /** | ||
| * Creates a new Transaction object from a DraftTransaction. | ||
| */ |
There was a problem hiding this comment.
Formatting / indentation.
| sender: Address.fromBech32(draft.sender), | ||
| receiver: Address.fromBech32(draft.receiver), | ||
| gasLimit: new BigNumber(draft.gasLimit).toNumber(), | ||
| chainID: "", |
There was a problem hiding this comment.
Yes, this is a bit unfortunate at this moment (since chainID is required on the Transaction).
| gasLimit: new BigNumber(draft.gasLimit).toNumber(), | ||
| chainID: "", | ||
| value: draft.value ?? 0, | ||
| data: draft.data ? new TransactionPayload(Buffer.from(draft.data)) : new TransactionPayload() |
There was a problem hiding this comment.
new TransactionPayload() accepts buffers, as well.
Updated the transaction factories according to the sdk-specs.