Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

Make manage:registrar:setStrarParam more intuitive #395

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make manage:registrar:setStrarParam more intuitive #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02bcebe
Set exact keys for AllStratConfigs
0xNeshi Oct 2, 2023
541313c
Update cliTypes.stratConfig to parse string name
0xNeshi Oct 2, 2023
48bcff3
Refactor manage:registrar:setStratParams to read StratConfig during r…
0xNeshi Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions tasks/deploy/integrations/helpers/fullStrategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {ContractFactory} from "ethers";
import {Deployment, StrategyApprovalState} from "types";
import {
AllStratConfigs,
deploy,
getAddresses,
getChainId,
Expand All @@ -15,7 +16,7 @@ import {deployVaultPair} from "contracts/core/vault/scripts/deployVaultPair";
import {HardhatRuntimeEnvironment} from "hardhat/types";

export async function deployStrategySet(
strategyName: string,
strategyName: keyof AllStratConfigs,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better type-safety

factory: ContractFactory,
signerPkey: string,
hre: HardhatRuntimeEnvironment
Expand Down Expand Up @@ -65,7 +66,7 @@ export async function deployStrategySet(

// establish registrar config on primary chain and this chain
await hre.run("manage:registrar:setStratParams", {
stratName: strategyName,
strategyName: strategyName,
modifyExisting: true,
apTeamSignerPkey: signerPkey,
});
Expand Down
57 changes: 26 additions & 31 deletions tasks/manage/registrar/setStratParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
import {allStrategyConfigs} from "contracts/integrations/stratConfig";
import {subtask, task, types} from "hardhat/config";
import {subtask, task} from "hardhat/config";
import {submitMultiSigTx} from "tasks/helpers";
import {cliTypes} from "tasks/types";
import {Registrar__factory} from "typechain-types";
import {ChainID} from "types";
import {StratConfig, getAPTeamOwner, getAddressesByNetworkId, isProdNetwork, logger} from "utils";
import {
AllStratConfigs,
StratConfig,
getAPTeamOwner,
getAddressesByNetworkId,
getPrimaryChainId,
logger,
} from "utils";

type TaskArgs = {
stratConfig: StratConfig;
strategyName: keyof AllStratConfigs;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the expected type in cliTypes.stratConfig.validate

modifyExisting: boolean;
apTeamSignerPkey?: string;
};

task("manage:registrar:setStratParams")
.addParam(
"stratConfig",
`The name of the strategy according to StratConfig, possible values: ${Object.keys(
allStrategyConfigs
).join(", ")}`,
"strategyName",
`The name of the strategy, possible values: ${Object.keys(allStrategyConfigs).join(", ")}`,
undefined,
cliTypes.stratConfig
)
Expand All @@ -27,36 +32,26 @@ task("manage:registrar:setStratParams")
"If running on prod, provide a pkey for a valid APTeam Multisig Owner."
)
.setAction(async function (taskArguments: TaskArgs, hre) {
if (await isProdNetwork(hre)) {
await hre.run("manage:registrar:setStratParams:on-network", {
...taskArguments,
chainId: ChainID.polygon,
});
await hre.run("manage:registrar:setStratParams:on-network", {
...taskArguments,
chainId: taskArguments.stratConfig.chainId,
});
} else {
await hre.run("manage:registrar:setStratParams:on-network", {
...taskArguments,
chainId: ChainID.mumbai,
});
await hre.run("manage:registrar:setStratParams:on-network", {
...taskArguments,
chainId: taskArguments.stratConfig.chainId,
});
}
const mainChainId = await getPrimaryChainId(hre);
const stratChainId = allStrategyConfigs[taskArguments.strategyName].chainId;

await hre.run("manage:registrar:setStratParams:on-network", {
...taskArguments,
chainId: mainChainId,
});
await hre.run("manage:registrar:setStratParams:on-network", {
...taskArguments,
chainId: stratChainId,
});
});

subtask(
"manage:registrar:setStratParams:on-network",
"Updates strat params on the network specified by the 'chainId' param"
)
.addParam(
"stratName",
`The name of the strategy according to StratConfig, possible values: ${Object.keys(
allStrategyConfigs
).join(", ")}`,
"strategyName",
`The name of the strategy, possible values: ${Object.keys(allStrategyConfigs).join(", ")}`,
undefined,
cliTypes.stratConfig
)
Expand Down Expand Up @@ -85,7 +80,7 @@ subtask(

logger.divider();
logger.out("Checking current strategy params at specified selector");
const config: StratConfig = taskArguments.stratConfig;
const config: StratConfig = allStrategyConfigs[taskArguments.strategyName];
let currentStratParams = await registrar.getStrategyParamsById(config.id);
if (
currentStratParams.liquidVaultAddr == hre.ethers.constants.AddressZero &&
Expand Down
13 changes: 8 additions & 5 deletions tasks/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ const booleanArray: CLIArgumentType<Array<boolean>> = {
},
};

const stratConfig: CLIArgumentType<StratConfig> = {
const stratConfig: CLIArgumentType<string> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

name: "StratConfig",
parse: (_, strValue) => allStrategyConfigs[strValue] || {error: strValue},
parse: (argName, strValue) => string.parse(argName, strValue),
/**
* Check if argument value is of type "StratConfig"
*
Expand All @@ -87,10 +87,13 @@ const stratConfig: CLIArgumentType<StratConfig> = {
* @throws HH301 if value is not of type "StratConfig"
*/
validate: (argName: string, argValue: any): void => {
if (!argValue || typeof argValue !== "object" || "error" in argValue) {
const invalidValue = "error" in argValue ? argValue.error : argValue;
string.validate(argName, argValue);

const possibleValues = Object.keys(allStrategyConfigs);

if (!possibleValues.includes(argValue)) {
throw new Error(
`Invalid value '${invalidValue}' for argument '${argName}' of type \`StratConfig\``
`Invalid value '${argValue}' for argument '${argName}', possible values: ${possibleValues}`
);
}
},
Expand Down
2 changes: 1 addition & 1 deletion utils/manageStratParams/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export type StratConfig = {
params: LocalRegistrarLib.StrategyParamsStruct;
};

export type AllStratConfigs = Record<string, StratConfig>;
export type AllStratConfigs = Record<"dummy" | "flux", StratConfig>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better type-safety

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@0xNeshi 0xNeshi Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevieraykatz

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

export async function deployStrategySet(
strategyName: keyof AllStratConfigs,

If you think we better leave this change for some other time, I'll remove it for now


export type StrategyObject = {
locked: string;
Expand Down