Make manage:registrar:setStrarParam more intuitive#395
Make manage:registrar:setStrarParam more intuitive#395
manage:registrar:setStrarParam more intuitive#395Conversation
| }; | ||
|
|
||
| export type AllStratConfigs = Record<string, StratConfig>; | ||
| export type AllStratConfigs = Record<"dummy" | "flux", StratConfig>; |
There was a problem hiding this comment.
I'm not a huge fan of this change. It means that each new strategy has to modify a Type which doesn't seem like it's a meaningful Type def. I would prefer the only edits necessary to be in the StratConfig.ts. Are there other ways we could make this more type safe without being overly restrictive here?
There was a problem hiding this comment.
Are there other ways we could make this more type safe without being overly restrictive here?
I think type-safety and restrictiveness come in a package 😕
I understood this type to be to strategy-addresses.json what type AddressObj is to contract-address.json. In order to make it even more so, defining keys strictly (this change) makes it so that we know for sure what the expected strategy names are; then we can do things like:
evm-smart-contracts/tasks/deploy/integrations/helpers/fullStrategy.ts
Lines 18 to 19 in 48bcff3
If you think we better leave this change for some other time, I'll remove it for now
| }; | ||
|
|
||
| const stratConfig: CLIArgumentType<StratConfig> = { | ||
| const stratConfig: CLIArgumentType<string> = { |
There was a problem hiding this comment.
cliTypes.stratConfig now expects the passed argument to be a key defined in allStrategyConfigs (atm: dummy, flux).
It no longer converts this key into type StratConfig, but lets the task to this itself, thus aligning the expected parameter type and resulting parameter type (for more info see: #384 (review))
|
|
||
| type TaskArgs = { | ||
| stratConfig: StratConfig; | ||
| strategyName: keyof AllStratConfigs; |
There was a problem hiding this comment.
Confirmed the expected type in cliTypes.stratConfig.validate
|
|
||
| export async function deployStrategySet( | ||
| strategyName: string, | ||
| strategyName: keyof AllStratConfigs, |
| }; | ||
|
|
||
| export type AllStratConfigs = Record<string, StratConfig>; | ||
| export type AllStratConfigs = Record<"dummy" | "flux", StratConfig>; |
There was a problem hiding this comment.
I'm not a huge fan of this change. It means that each new strategy has to modify a Type which doesn't seem like it's a meaningful Type def. I would prefer the only edits necessary to be in the StratConfig.ts. Are there other ways we could make this more type safe without being overly restrictive here?
2 participants
Explanation of the solution
Fixes issues with and addresses #384 (review)
Instructions on making this work
yarnoryarn installto install npm dependencies