Skip to content
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

Remove prepare dependencies script #143

Merged
merged 8 commits into from
Jun 6, 2023

Conversation

dimpar
Copy link
Contributor

@dimpar dimpar commented May 23, 2023

Refs #142

This PR removes prepare-dependencies.sh script that randomly errors out and gives a lot of headaches to developers. The script was added to mitigate the issue with the same naming of TokenStaking.sol contracts that are in keep-core and solidity-contracts repos. In some projects, both repos were added as dependencies and hardhat had a hard time differentiating the contracts during deployment. Since v1 contracts are already deployed and won't be changed, we can now hardcode the artifacts and get rid of the troubled prepare-dependencies.sh script and related code.

The following repos were checked for any potential issues as a consequence of removing this script. I didn't find any.

  • threshold-network/token-dashboard
  • threshold-network/merkle-distribution
  • keep-network/coverage-pools
  • keep-network/keep-core (random-beacon and ecdsa)

Depends on #144

dimpar added 3 commits May 23, 2023 15:38
Build artifacts are stored inside the build/ dir, not the artifacts/
dir. Ignoring artifacts/ dir prevents from unignoring
@keep-network/keep-core/artifact.

Ignoring artifacts under external/npm with the exception of
@keep-network/ dir
This script a lot of times errored out with the $INIT_CWD issue. It
happens randomly, but brings a lot of headache for the developers.
Instead of fixing the issue with dependencies and diving into yarn
that might be a rabbit hole we can hardcode a couple of artifacts
that were already deployed.

Most of the projects that have the solidity-contracts as dependency do
not use any artifacts from external/npm/@keep-network/keep-core dir.
@dimpar dimpar self-assigned this May 23, 2023
Copy link
Contributor

@michalinacienciala michalinacienciala left a comment

Choose a reason for hiding this comment

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

Let's also modify this comment:

// Due to a limitation of `hardhat-deploy` plugin, we have
// to modify the artifacts imported from NPM. Please see
// `scripts/prepare-dependencies.sh` for details.
artifacts: "external/npm/@keep-network/keep-core/artifacts",

dimpar added 2 commits May 24, 2023 22:59
We hardcode external goerli addresses from version @keep-network/[email protected]
and get rid of other unused artifacts. This change is done because
prepare-dependencies.sh script was removed.
Comment on lines 102 to 103
// ropsten network in no longer supported and should not be used
ropsten: [""],
Copy link
Member

Choose a reason for hiding this comment

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

So maybe remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I considered this myself but opted to just add a comment since there will be way more work around cleaning unused networks, and ropsten is a part of it. Anyway, removed it here fc2f177

.gitignore Outdated Show resolved Hide resolved
@michalinacienciala
Copy link
Contributor

Even if we're not planning any redeploys of threshold-network/solidity-contracts now on goerli, we might want to deploy to sepolia in a near future - it would be good to be prepared for that...

Not sure if I follow the preparation part. My understanding is that once we need to add Sepolia to the game then all the contracts will need to be deployed to Sepolia and new dirs and configurations will be created. We have some code to clean here, e.g. ropsten and rinkeby are dead. Cleaning all the references is out of the scope of this PR and should be addressed in a separate PR (if needed).

What I meant by being prepared for supporting sepolia was creating a file structure in the external directory in a way that we have separate paths for different networks (which is exactly what you did :) ).

@michalinacienciala michalinacienciala self-requested a review June 6, 2023 09:09
@michalinacienciala
Copy link
Contributor

After the PR gets merged, it will be good to see if there's anything that can be changed in the workflows we use in projects using @threshold-network/solidity-contracts package as a dependency. There shouldn't be anything that starts to fail, but there are places were we had some workarounds for randomly failing install steps and those workarounds will be no longer needed.
I can do that clean-up.

@michalinacienciala michalinacienciala merged commit 3592b22 into main Jun 6, 2023
@michalinacienciala michalinacienciala deleted the remove-prepare-dependencies-script branch June 6, 2023 09:14
dimpar added a commit to keep-network/tbtc-v2 that referenced this pull request Jun 9, 2023
Background:
Previously we've been running postinstall script in a separate step from
the `yarn install` command. We've been doing that as a workaround for a
problem we had with the postinstall script in the
`@threshold-network/solidity-contracts` package (the script in that
package often randomly failed). Running `yarn upgrade` with
`--ignore-scripts` flag prevented the execution of postinstall script in
the main project and its depandencies. And running `yarn run
postinstall` after did execute the postinstall script in the main
project.

The change:
Recently we have refactored the `@threshold-network/solidity-contracts`
project and got rid of the problematic script. We can go back to
executing `yarn install` without the `--ignore-scripts` flag.

Ref:
threshold-network/solidity-contracts#142
threshold-network/solidity-contracts#143
threshold-network/token-dashboard#531
@michalinacienciala
Copy link
Contributor

@dimpar, changes to typescript dependencies will be delivered in keep-network/tbtc-v2#636.
I had to use a different method of updating the dependencies, to limit the diff. I did it according to @nkuba's suggestion

I was updating version in package.json to the exact one, then running yarn install and then updating the package.json to the tag or range and running yarn install again.

This indeed was better. We still got a significant diff, but this time it was justifiable. Most of the diff is a result of getting rid of the @tenderly/hardhat-tenderly dependency in the newer version of @keep-network/tbtc-v2.

dimpar added a commit to keep-network/tbtc-v2 that referenced this pull request Jun 15, 2023
We have published new `@keep-network/ecdsa` and `@keep-network/tbtc-v2`
packages. Those packages no longer include the `prepare-dependencies.sh`
script from `@threshold-network/solidity-contracts` which was causing
random failures during `yarn install`. As in some CI jobs we install the
dependencies based on the lockfile (with the `--frozen-lockfile` flag),
we need to update dependencies in the lockfile so that the new, improved
packages would be used.

Ref:
threshold-network/solidity-contracts#142
threshold-network/solidity-contracts#143
#631
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants