-
Notifications
You must be signed in to change notification settings - Fork 203
test(benches): Add Highload-Wallet v3 benchmarks #3435
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
base: main
Are you sure you want to change the base?
Conversation
Waiting for tests and then approve |
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.
Good job! Solid PR overall. Just a few small comments. One thing: there are quite a few magic numbers in the tests - please consider replacing them with named constants or removing unnecessary ones.
I’d also suggest adding more comments to the Tact code so that people looking at it for examples can better understand what’s going on
codeSizeResults: CodeSizeResult, | ||
fromInit: FromInitHighloadWalletV3, | ||
) { | ||
let blockchain: Blockchain; |
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.
setup() instead of global vars
|
||
// after commit, check the message to prevent an error in the action phase | ||
|
||
let messageSlice = msgInner.messageToSend.beginParse(); |
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 we use structs instead of manual parsing?
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.
Good job on Tact code! A couple of notes about ts
|
||
const justTestFlow = async (kind: "external" | "internal") => { | ||
const testReceiver = receiver.address; | ||
const forwardToSelfValue = toNano(0.17239); |
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.
Why this const?
|
||
let step: Step; | ||
|
||
const justTestFlow = async (kind: "external" | "internal") => { |
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.
Let's rename it to something meaningful
message: internalMessage, | ||
mode: SendMode.PAY_GAS_SEPARATELY, | ||
queryId, | ||
createdAt: ~~(Date.now() / 1000), |
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.
createdAt: ~~(Date.now() / 1000), | |
createdAt: (Date.now() / 1000), |
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.
No, just Math.floor() is needed here, not ~~
"cells": "17", | ||
"bits": "4373" | ||
}, | ||
"pr": "https://github.com/ton-blockchain/highload-wallet-contract-v3" |
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.
Permalink to commit
.endCell(); | ||
} | ||
|
||
*generateBadMsg() { |
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.
Why *?
"externalTransfer": "6200", | ||
"internalTransfer": "2065" | ||
}, | ||
"pr": "https://github.com/ton-blockchain/highload-wallet-contract-v3" |
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.
permlink too
message: internalMessage, | ||
mode: SendMode.PAY_GAS_SEPARATELY, | ||
queryId, | ||
createdAt: ~~(Date.now() / 1000), |
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.
No, just Math.floor() is needed here, not ~~
export const SUBWALLET_ID = 0; | ||
|
||
const getRandom = (min: number, max: number) => { | ||
return Math.random() * (max - min) + min; |
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.
maybe
import { randomInt } from 'crypto'
?
Closes #3395