-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: allow setting min gas price in launchNode
args
#2814
Conversation
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.
@mvares Thanks for this contribution 👍
We are almost there, we only need to remove the --min-gas-price
from the informed args.
This is required based on your solution, otherwise the --min-gas-price
will still be used twice.
You simply need to edit the call to extractRemainingArgs
at the launchNode
function.
const remainingArgs = extractRemainingArgs(args, [
'--snapshot',
'--consensus-key',
'--db-type',
'--poa-instant',
'--min-gas-price', // <- needs to be added here
'--native-executor-version', // Please can you also add this missing flag?
]);
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.
@mvares It would be good if we could also add a test to validate the changes made here.
Let's unskip and change this test case here to:
it('should ensure estimated fee values on getTransactionCost are never 0', async () => {
using launched = await setupTestProviderAndWallets({
nodeOptions: {
args: ['--min-gas-price', '0'],
},
});
const { provider } = launched;
const request = new ScriptTransactionRequest();
const { minFee, maxFee, gasPrice } = await provider.getTransactionCost(request);
expect(gasPrice.eq(0)).toBeTruthy();
expect(maxFee.eq(0)).not.toBeTruthy();
expect(minFee.eq(0)).not.toBeTruthy();
});
launchNode
args
launchNode
argslaunchNode
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.
@mvares This is the last one
launchNode
Support Min Gas Price #2813Summary
In the previous solution, the
minGasPrice
was already defined by default. This PR fixes it by basing it on theparams
.Checklist
tests
to prove my changesdocs