-
Notifications
You must be signed in to change notification settings - Fork 37
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
Smart Contract intents factories #327
Conversation
const hex = Buffer.from(value).toString("hex"); | ||
return codecUtils.zeroPadStringIfOddLength(hex); | ||
} | ||
export { utf8ToHex } from "../utils.codec"; |
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 is a non-breaking change.
In the future, this file will be removed (new intent factories are to be used).
export function utf8ToHex(value: string) { | ||
const hex = Buffer.from(value).toString("hex"); | ||
return zeroPadStringIfOddLength(hex); | ||
} | ||
|
||
export function byteArrayToHex(byteArray: Uint8Array): string { | ||
return Buffer.from(byteArray).toString("hex"); | ||
} |
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.
Functions can be tested in utils.codec.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.
added unit tests
src/transactionIntent.ts
Outdated
public value?: BigNumber.Value; | ||
public data?: Uint8Array; | ||
|
||
public constructor(sender: string, receiver: string, gasLimit: BigNumber.Value, value?: BigNumber.Value, data?: Uint8Array) { |
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.
Receive an initialization object in the constructor, similar to transaction.ts
: constructor({ ... })
;
That way, if we'd like to re-order the parameters (options), we will do non-breaking changes. Otherwise, in this format, a breaking change will occur if we'd like to, say, add a new parameter "between" gasLimit
and value
.
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.
Ok. Now using constructor(options: {...})
private executionGasLimit: BigNumber.Value; | ||
private value?: BigNumber.Value; | ||
|
||
constructor(config: Config, sender: IAddress, receiver: IAddress, dataParts: string[], executionGasLimit: BigNumber.Value, value?: BigNumber.Value) { |
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.
constructor({ ... })
(receive parameters as an object).
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.
Done
} | ||
|
||
private computeGasLimit(payload: ITransactionPayload, executionGasLimit: BigNumber.Value): BigNumber.Value { | ||
const dataMovementGas = new BigNumber(this.config.minGasLimit).plus(new BigNumber(this.config.gasLimitPerByte).multipliedBy(new BigNumber(payload.length()))); |
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.
multipliedBy(payload.length)
should work, without new BigNumber()
.
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.
removed.
assert.deepEqual(deployIntent, abiDeployIntent); | ||
}); | ||
|
||
it("should build execute intent", async function () { |
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.
"should build intent for execute" (similar to the function name).
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.
Renamed
|
||
assert.isDefined(deployIntent.data); | ||
let decoder = new TextDecoder(); | ||
assert.equal(decoder.decode(deployIntent.data), "add@07"); |
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.
assert.equal(deployIntent.data.toString(), "add@07")
or assert.equal(deployIntent.data, Buffer.from("add@07"))
should work, without using a TextDecoder
.
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.
Indeed. Works when using assert.deepEqual()
assert.deepEqual(deployIntent, abiDeployIntent); | ||
}); | ||
|
||
it("should build upgrade intent", async function () { |
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.
"... intent for upgrade" etc.
} | ||
}); | ||
|
||
it("should build deploy intent", async function () { |
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.
"... intent for deploy".
assert.equal(deployIntent.receiver, "erd1qqqqqqqqqqqqqpgqhy6nl6zq07rnzry8uyh6rtyq0uzgtk3e69fqgtz9l4"); | ||
assert.isDefined(deployIntent.data); | ||
|
||
if (deployIntent.data) { |
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 drop condition if not necessary. Below, refactor to not use TextDecoder
(as above).
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.
Not using TextDecoder
. Condition still necessary.
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.
Also here, I think you can use !
operator to avoid this check:
assert(checkIfByteArrayStartsWith(deployIntent.data!, "upgradeContract@"));
const expectedGasLimit = 6000000 + 50000 + 1500 * deployIntent.data!.length;
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.
Now using the !
operator.
|
||
assert.equal(deployIntent.sender, "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th"); | ||
assert.equal(deployIntent.receiver, "erd1qqqqqqqqqqqqqpgqhy6nl6zq07rnzry8uyh6rtyq0uzgtk3e69fqgtz9l4"); | ||
assert.isDefined(deployIntent.data); | ||
assert(checkIfByteArrayStartsWith(deployIntent.data!, "upgradeContract@")); |
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.
Alternatively, shorter, without checkIfByteArrayStartsWith
:
assert.isTrue(Buffer.from(deployIntent.data!).toString().startsWith("upgradeContract@"));
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.
Will do this in the next PR.
else { | ||
preparedArgs = this.argsToStrings(args) | ||
} | ||
const preparedArgs = this.argsToDataParts(args, this.abiRegistry?.constructorDefinition) |
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, parts
could have been const
.
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.
Will change in the next PR.
bytecode: adderByteCode.valueOf(), | ||
gasLimit: gasLimit, | ||
args: args | ||
}); |
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.
Formatting / newline.
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.
Will fix.
Implemented the smart contract transaction intent factories based on the sdk-specs