SE 641 parallel executor gas config #25
Conversation
| process.env.PTE_INITIAL_COIN_BALANCE ?? 5_000_000_000, | ||
| Number(balance) * 1000 ?? process.env.PTE_INITIAL_COIN_BALANCE, | ||
| ), | ||
| minimumCoinBalance: BigInt( | ||
| process.env.PTE_MINIMUM_COIN_BALANCE ?? 500_000_000, | ||
| Number(balance) * 1000 ?? process.env.PTE_MINIMUM_COIN_BALANCE, |
There was a problem hiding this comment.
What's the logic behind this change?
There was a problem hiding this comment.
It is related to the configuration of the coin balances. The initialisation of the gas coins' balance may not be sufficient for the transactions to happen. We don't have a problem when the transaction gas fee is smaller than the balance of the gas coin used, but if the gas coin is smaller the executor keeps erroring unless you merge all the coins again and increase the balance at the configuration.
| "0xdd33675337d769bc9cc4120e204afd8e3f6aa047b2a9ee5d6c6c1dcbc87bd169", | ||
| "0xc44fb22cb1786b4eae31ada831bb0fa2549612ea1dabec5231a792c9fa921f46", |
There was a problem hiding this comment.
Let's parse this test address from the .env so we don't push this change every time we make a PR to the project.
| ), | ||
| // The maximum number of gas coins to keep in the gas pool, | ||
| // which also limits the maximum number of concurrent transactions | ||
| maxPoolSize: parseInt(process.env.PTE_MAX_POOL_SIZE ?? "10"), | ||
| }); | ||
|
|
||
| export async function dryRunTransaction(): Promise<string | undefined> { |
There was a problem hiding this comment.
Why is this function only exported? Where and how should it be used?
There was a problem hiding this comment.
It shouldn't be exported, it's wrongly left with the export, I just wanted to test at some point and added the export. Will remove it.
| smartContractFunctionName: "mint_nft", | ||
| smartContractFunctionArguments: [process.env.ADMIN_CAP!], | ||
| receiverAddress: | ||
| "0x021318ee34c902120d579d3ed1c0a8e4109e67d386a97b841800b0a9763553ef", |
There was a problem hiding this comment.
These fields should not be hardcoded. Depending on how this function will be used, they should be passed as function arguments.
There was a problem hiding this comment.
It should be called before the configuration of the ParallelExecutor. Maybe it can be called in the deploy.sh script as well and define the env variable in the .env file from there.
There was a problem hiding this comment.
I think adding this to the deploy.sh would complicate the procedure because the user would have to specify even more variables in the .env, but I leave this up to you.
Also, what would happen if you wanted estimate the gas cost for more than one move call? Remember that there will be scenarios that not only a mint function will be available.
Nevertheless, we still have to avoid hard coded values.
There was a problem hiding this comment.
Correct! I'm not sure we can make it very generic as we need the parameters filled. Should we just include a descriptions on how to find the best amount for the env variables and don't include the dryRun in the solution?
There was a problem hiding this comment.
Should we just include a descriptions on how to find the best amount for the env variables and don't include the dryRun in the solution?
This approach sounds good to me. @teohaik what do you think?
There was a problem hiding this comment.
I agree that this should be generic and using the defined names for move module and function. Also we can use 0x0 for a generic receiver for dry run
There was a problem hiding this comment.
But after all, they won't have access to code, so they should be use the CLI for dry run
There was a problem hiding this comment.
So we may not need to implement that after all. Should we add it to not do and close the PR?
|
Close as won't do |
This PR adds the logic that dryRuns the transaction before setting up the Parallel Executor and based on the result, we set the minimum and initial coin balances.