Don't include stub contracts in NPM packages#3569
Closed
michalinacienciala wants to merge 1 commit into
Closed
Conversation
If `hardhat deploy` is run with `TEST_USE_STUBS_ECDSA` environment variable set to `true`, the deployment will include deployment of stub contracts, which are needed for testing purposes (execution of unit tests). So far we've been running `hardhat deploy TEST_USE_STUBS_ECDSA=true` together with `USE_EXTERNAL_DEPLOY=true` in `ecdsa` in the `deploy:test` script. The `USE_EXTERNAL_DEPLOY` variable specifies how the contracts of the dependencies should be deployed - whether to deploy them from scratch (`true`) or reuse the already deployed artifacts (`false`). We were using the `deploy:test` script in `ecdsa` in two places - when running `contracts-deployment-dry-run` job (test of deployment on `hardhat` local network) and in `npm-compile-publish-contracts` job (publishing of `development`-tagged NPM package). This second use proved to be problematic, as the `development`-tagged package is used to generate the client bindings when running `make all`, resulting in the presence of unneeded (and unwanted) functionalities (like `forceAddWallet` and `getDkgData` from `WalletRegistryStub`). The first use is not causing such problems, but it should still be modified, as it's better to run the dry-run of deploy on the real contracts and not on the stubs. What we decided to do is to replace ``` "deploy:test": "USE_EXTERNAL_DEPLOY=true TEST_USE_STUBS_ECDSA=true hardhat deploy", ``` with ``` "deploy:local": "USE_EXTERNAL_DEPLOY=true hardhat deploy", ``` and use the new `deploy:local` script in both jobs that previously used `deploy:test`. This way we'll no longer include stub functionalities in NPM packages (and hence in the clinet bindings) and we'll execute dry-run of the unmodified contracts. Scripts in `random-beacon` also got updated to follow the new naming.
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If
hardhat deployis run withTEST_USE_STUBS_ECDSAenvironment variable set totrue, the deployment will include deployment of stub contracts, which are needed for testing purposes (execution of unit tests). So far we've been runninghardhat deploy TEST_USE_STUBS_ECDSA=truetogether withUSE_EXTERNAL_DEPLOY=trueinecdsain thedeploy:testscript. TheUSE_EXTERNAL_DEPLOYvariable specifies how the contracts of the dependencies should be deployed - whether to deploy them from scratch (true) or reuse the already deployed artifacts (false).We were using the
deploy:testscript inecdsain two places - when runningcontracts-deployment-dry-runjob (test of deployment onhardhatlocal network) and innpm-compile-publish-contractsjob (publishing ofdevelopment-tagged NPM package). This second use proved to be problematic, as thedevelopment-tagged package is used to generate the client bindings when runningmake all, resulting in the presence of unneeded (and unwanted) functionalities (likeforceAddWalletandgetDkgDatafromWalletRegistryStub). The first use is not causing such problems, but it should still be modified, as it's better to run the dry-run of deploy on the real contracts and not on the stubs.What we decided to do is to replace
with
and use the new
deploy:localscript in both jobs that previously useddeploy:test.This way we'll no longer include stub functionalities in NPM packages (and hence in the clinet bindings) and we'll execute dry-run of the unmodified contracts. Scripts in
random-beaconalso got updated to follow the new naming.Refs:
#3531
threshold-network/tbtc-v2#623