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

Resolves AP-638, AP-784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" #375

Merged
merged 12 commits into from
Sep 13, 2023

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Sep 12, 2023

Explanation of the solution

Instructions on making this work

  • run yarn or yarn install to install npm dependencies

@0xNeshi 0xNeshi added the enhancement New feature or request label Sep 12, 2023
@0xNeshi 0xNeshi self-assigned this Sep 12, 2023
@linear
Copy link

linear bot commented Sep 12, 2023

AP-638 Implement shell script for updating cross-chain strat params

When a strat param is updated we need to:

  • start params in the registrar on the primary chain
  • and to update them on the cross chain where the strategy is implemented

}
);

hre.changeNetwork = lazyFunction(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing Hardhat network during runtime is currently not possible, but is in the works. In the meantime, added this extension to our hardhat runtime that enables us to achieve this.

@@ -0,0 +1,56 @@
/**
* @author misicnenad
* @description created based on unmaintained repo https://www.npmjs.com/package/hardhat-change-network
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linked repo is unmaintained and had some issues with our version of hardhat.
Just copy/pasted the code and fixed the issues

Copy link
Contributor

Choose a reason for hiding this comment

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

That's awesome man.

This "context switching" assumes that the hardhat accounts are the same for each network, yes? Might be good to make that explicit though I suppose our env config mostly enforces that anyway. This is irrelevant in cases where we're executing calls with a pkey

Copy link
Contributor

Choose a reason for hiding this comment

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

When we make this repo public, consider linking it in the issue! Some people might be interested in using this in the meantime :) Get those gh stars!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome man.

This "context switching" assumes that the hardhat accounts are the same for each network, yes? Might be good to make that explicit though I suppose our env config mostly enforces that anyway. This is irrelevant in cases where we're executing calls with a pkey

The function is reading Hardhat's config for the target network (the one being switched to), so any accounts configured on said network should be there.
What's really being changed here are just the provider (create one connecting to the target network) and the network metadata.

@@ -1,3 +1,12 @@
export enum ChainID {
Copy link
Contributor Author

@0xNeshi 0xNeshi Sep 12, 2023

Choose a reason for hiding this comment

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

Makes it easier to associate network name with chain ID + improves type-safety a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

Love this

@@ -56,9 +59,72 @@ const stringArray: CLIArgumentType<Array<string>> = {
},
};

function enums<T extends {[key in string]: string | number}>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use this to validate types of other task params that should be cast to enums (see https://linear.app/angel-protocol/issue/AP-779/create-custom-task-argument-types-where-applicable)

@0xNeshi 0xNeshi changed the title Ap 638: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Ap 638, Ap 784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Sep 12, 2023
@0xNeshi 0xNeshi changed the title Ap 638, Ap 784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Resolves Ap 638, Ap 784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Sep 12, 2023
@0xNeshi 0xNeshi changed the title Resolves Ap 638, Ap 784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Resolves: Ap 638, Ap 784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Sep 12, 2023
@0xNeshi 0xNeshi changed the title Resolves: Ap 638, Ap 784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Resolves AP-638, AP-784: Update setStratParams to run both on Polygon (main Registrar) and strategy's network (LocalRegistrar)" Sep 12, 2023
Copy link
Contributor

@stevieraykatz stevieraykatz left a comment

Choose a reason for hiding this comment

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

Left a couple comments but this looks so cool man. Great work extending hardhat!

@@ -0,0 +1,56 @@
/**
* @author misicnenad
* @description created based on unmaintained repo https://www.npmjs.com/package/hardhat-change-network
Copy link
Contributor

Choose a reason for hiding this comment

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

That's awesome man.

This "context switching" assumes that the hardhat accounts are the same for each network, yes? Might be good to make that explicit though I suppose our env config mostly enforces that anyway. This is irrelevant in cases where we're executing calls with a pkey

@@ -1,3 +1,12 @@
export enum ChainID {
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this

@SovereignAndrey SovereignAndrey merged commit 8936571 into master Sep 13, 2023
@SovereignAndrey SovereignAndrey deleted the ap-638 branch September 13, 2023 04:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
No open projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants