ZDC Update for Multi Chain #120
Conversation
…se zDC functionality linked, change all calls in DMs, add new env var for confirmation number, add to validator
…of srcChainName var
… env vars on zns side instead
… from zns campaign config
ZDC Update for Multi Chain
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
…ework postDeploy on EthereumPortalDM to properly run both tasks
…itialization for campaign scripts
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## multi-zns-proto #120 +/- ##
===================================================
+ Coverage 94.93% 95.62% +0.68%
===================================================
Files 19 19
Lines 731 731
Branches 166 166
===================================================
+ Hits 694 699 +5
+ Misses 37 32 -5 |
| env ?: TEnvironment; | ||
| }) : Promise<IZNSCampaignConfig> => { | ||
| // Prioritize reading from the env variable first, and only then fallback to the param | ||
| let envLevel = process.env.ENV_LEVEL as TEnvironment; |
There was a problem hiding this comment.
Could simplify this and any other null checks with const envLevel = env ?? process.env.ENV_LEVEL; or the opposite order.
Also this seems a bit odd because it will read ENV_LEVEL but it will always fallback to env if specified, but from your comment it sounds like it should only fallback to env if the env var wasn't found in the system with process.env.ENV_LEVEL
| @@ -93,7 +103,9 @@ export const getConfig = async ({ | |||
| } | |||
|
|
|||
| // Domain Token Values | |||
| const royaltyReceiver = process.env.ENV_LEVEL !== "dev" ? process.env.ROYALTY_RECEIVER! : zeroVaultAddressConf; | |||
| const royaltyReceiver = process.env.ENV_LEVEL !== EnvironmentLevels.dev | |||
There was a problem hiding this comment.
we could do else if above and it would feel cleaner than checking twice and separately
| @@ -82,7 +92,7 @@ export const getConfig = async ({ | |||
|
|
|||
| let zeroVaultAddressConf : string; | |||
There was a problem hiding this comment.
small nitpick, but what is the Conf suffix adding here? Makes me think this is a config, but it's just a single value. I would just use zeroVaultAddress
| export const buildCrosschainConfig = (env : TEnvironment) : { | ||
| crosschain : TZNSCrossConfig; | ||
| srcChainName : TSupportedChain; | ||
| } => { | ||
| const srcChainName = process.env.SRC_CHAIN_NAME as TSupportedChain; |
There was a problem hiding this comment.
This won't actually cause an error if proess.env.SRC_CHAIN_NAME is not one of the one's included in TSupportedChain, the as in TypeScript is basically telling the transpiler "even if this isn't what I type it as, don't complain because I know what I'm doing"
It will be fine though because your switch below checks for exact values
| @@ -275,5 +305,5 @@ export const buildCrosschainConfig = () : TZNSCrossConfig => { | |||
| throw new Error(`Unsupported chain: ${srcChainName}!`); | |||
| } | |||
|
|
|||
| return crossConfig; | |||
| return { crosschain: crossConfig, srcChainName }; | |||
There was a problem hiding this comment.
why not just use the name of the variable you're returning when you declare it above for simplicity? or change the return variable name to match? seems like a needless thing to have one prop named the return value and one not
| @@ -39,13 +36,14 @@ export interface IZNSEthCrossConfig extends IZNSBaseCrossConfig { | |||
|
|
|||
| export interface IZNSZChainCrossConfig extends IZNSBaseCrossConfig { | |||
| srcZnsPortal : string; | |||
| ethAdmin ?: IZNSSigner; | |||
| } | |||
|
|
|||
| export type TZNSCrossConfig = IZNSEthCrossConfig | IZNSZChainCrossConfig; | |||
There was a problem hiding this comment.
small nitpick, but it feels redundant to start every custom type with T, we know they are a type by the way we reference it
rc/multi-zns-mainormulti-zns-proto(after merge with the former) needs to be merged into this branch to update the code with all the previous changes !!!