-
Notifications
You must be signed in to change notification settings - Fork 28
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 validation for global params with no default value #836
Conversation
@@ -65,7 +65,8 @@ export async function validateNamedContractDeployment( | |||
|
|||
const missingParams = moduleParams.filter( | |||
(param) => | |||
deploymentParameters[param.moduleId]?.[param.name] === undefined && | |||
(deploymentParameters[param.moduleId]?.[param.name] === undefined && | |||
deploymentParameters.$global[param.name]) === undefined && |
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 think this is a bug, the additional brackets are not going to give the value expected.
@@ -80,6 +81,7 @@ export async function validateNamedContractDeployment( | |||
if (isModuleParameterRuntimeValue(future.value)) { | |||
const param = | |||
deploymentParameters[future.value.moduleId]?.[future.value.name] ?? | |||
deploymentParameters.$global[future.value.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.
deploymentParameters.$global[future.value.name] ?? | |
deploymentParameters.$global?.[future.value.name] ?? |
I think we need to guard against there being no global parameters passed.
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 need tests around the new validation paths.
We should also add tests explicitly for the no default value in a module parameter but a valid entry in the global module parameters.
I closing this in favour of: #832 |
resolves #834