-
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
Delegation transaction intents factory #328
Conversation
}).build(); | ||
} | ||
|
||
private computeExecutionGasLimitForNodesManagement(numNodes: number): 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.
can you please move this at the end of the file or maybe decide on a pattern, first public then private?
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.
Moved to the end of file.
@@ -140,23 +141,12 @@ describe("test smart contract intents factory", function () { | |||
assert.equal(deployIntent.sender, "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th"); | |||
assert.equal(deployIntent.receiver, "erd1qqqqqqqqqqqqqpgqhy6nl6zq07rnzry8uyh6rtyq0uzgtk3e69fqgtz9l4"); | |||
assert.isDefined(deployIntent.data); | |||
assert(checkIfByteArrayStartsWith(deployIntent.data!, "upgradeContract@")); | |||
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.
👍
import { SignableMessage } from "../signableMessage"; | ||
|
||
describe("test delegation intents factory", function () { | ||
const delegationFactory = new DelegationTransactionIntentsFactory({ |
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.
If we add a config like here:
Then we can also use it in the unit tests (easier).
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.
Created the config file.
additionalGasLimitForDelegationOperations: 10000000 | ||
}); | ||
|
||
it("should build intent for new delegation contract", 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.
Now I see, we can change "should build" to "should create" (optional, "philosophical").
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.
Changed to should create
it("should build intent for new delegation contract", async function () { | ||
const sender = Address.fromBech32("erd18s6a06ktr2v6fgxv4ffhauxvptssnaqlds45qgsrucemlwc8rawq553rt2"); | ||
const delagationCap = new BigNumber("5000000000000000000000"); | ||
const serviceFee = new BigNumber(10); |
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 can also use raw strings and numbers, and the factory methods should be able to handle them appropriately. Perhaps do this in some tests? E.g. directly pass "5000000000000000000000"
.
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.
Changed this test, works as expected.
sender: sender, | ||
delegationContract: delegationContract, | ||
publicKeys: [publicKey], | ||
signedMessages: [signedMessage.getSignature()] |
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.
Since it's a mock message
, we can do:
const mockMessage = {
getSignature: () => Buffer.from("...", hex)
}
Should work. Then, we'll remove new SignableMessage()
and applySignature()
from the test.
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
let dataParts = ["addNodes"]; | ||
for (let i = 0; i < numNodes; i++) { | ||
dataParts = dataParts.concat(options.publicKeys[i].hex()); | ||
dataParts = dataParts.concat(byteArrayToHex(options.signedMessages[i])); | ||
} | ||
|
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.
If possible, we can have const dataParts
, and use .push().
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 have const dataParts
and are using .push()
now
let dataParts = ["removeNodes"]; | ||
|
||
for (const key of options.publicKeys) { | ||
dataParts = dataParts.concat(key.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.
I am not sure why concat
works fine in this case. Does the compiler complain if you add the typing markers dataParts: string[] = ...
? Perhaps use append
, as mentioned in the comment 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.
Now using push()
. It does not complain if I add typing markers but it already shows the correct type.
} | ||
|
||
const numNodes = options.publicKeys.length; | ||
const executionGasLimit = new BigNumber(numNodes).multipliedBy( |
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 be split, a temporary variable can be used.
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
let dataParts = ["unJailNodes"]; | ||
|
||
for (const key of options.publicKeys) { | ||
dataParts = dataParts.concat(key.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.
append
vs. concat
.
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 push()
createTransactionIntentForNewDelegationContract(options: { | ||
sender: IAddress, | ||
totalDelegationCap: BigNumber.Value, | ||
serviceFee: 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.
Indeed, we receive as the BigNumber.Value
interface.
Implemented the delegation transaction intents factory based on the sdk-specs.