Skip to content

Support local artifacts in the Makefile#3129

Merged
dimpar merged 3 commits intomainfrom
make-local-artifacts
Aug 11, 2022
Merged

Support local artifacts in the Makefile#3129
dimpar merged 3 commits intomainfrom
make-local-artifacts

Conversation

@nkuba
Copy link
Copy Markdown
Member

@nkuba nkuba commented Aug 4, 2022

Depends on: #3126

On developer machine contracts are deployed to local geth instance. We need to use them in the client, instead of downloading a package from NPM registry.

Here we add support for local artifacts. The make command will fetch the artifacts from local directories and use them for client build.

This is useful if a developer wants to generate bindings for locally deployed contracts.

What is more, the client will embed the addresses of locally deployer contracts, so running the client locally won't require setting the addresses in the configuration file or --developer flags.

The directories can be provided by the caller, e.g.:
make local local_threshold_path=/other/path/threshold

If the directories are not provided default paths are assumed:

local_beacon_path := ./solidity/random-beacon
local_ecdsa_path := ./solidity/ecdsa
local_threshold_path := ../../threshold-network/solidity-contracts
local_tbtc_path := ../tbtc-v2/solidity

On developer machine contracts are deployed to local geth instance. We
need to use them in the client, instead of downloading a package from
NPM registry.

Here we add support for local artifacts. The make command will fetch the
artifacts from local directories. The directories can be provided by the
caller, e.g.:
make local local_threshold_path=/other/path/threshold

If the directories are not provided default paths are assumed.
@nkuba nkuba changed the base branch from main to make-download-artifacts August 4, 2022 22:51
@nkuba nkuba requested a review from a team August 4, 2022 22:51
@nkuba nkuba self-assigned this Aug 4, 2022
The target name has changed in the Makefile so we need to update it here
as well.
Base automatically changed from make-download-artifacts to main August 8, 2022 13:42
@nkuba nkuba marked this pull request as ready for review August 8, 2022 14:06
@nkuba nkuba mentioned this pull request Aug 8, 2022
1 task
Comment thread Makefile
# make all environment=mainnet

all: download_artifacts generate build cmd-help
local:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When we run Geth on our local we use the "development" name. But here we use "local" name to fetch the artifacts for the "development" env and in the logs I see:

Environment          local

even though the installation scripts run for "development". Do you think we can stick to "development" name in this case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Network and environment names got confusing. We've got following networks in hardhat:

  • hardhat (default) - local hardhat node
  • development - local geth node
  • goerli - public test network
  • mainnet - public main network

To use contracts packages as dependencies during the development, in the CI we build artifacts and publish them to NPM tagged development. These artifacts can be used as compiled artifacts but we cannot use the addresses to call them, as they were deployed to internal CI's hardhat node during the CI workflow execution.

In the client, we also pull those packages tagged development to build the client with the latest version of contract bindings.

An alternative path is when a developer wants to deploy contracts on their own machine and use the artifacts in the client. The network in hardhat they use is development, to not confuse it with the package the client already expects, I used the name local.

I'm open to straighten it up. But since renaming will have quite significant impact I'd like to have a broader discussion.

CC: @michalinacienciala @pdyraga

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to have a development name for local geth node. It also was fine for the packages to be named development pre local geth node time. But yea, now it's a bit confusing.

I would rename development npm packages to ex. integration since they run on CI and drop the development package name. However, I'm not sure how much time and effort it takes to do it.

Maybe we can start smaller, and change the logs from Environment: local -> development and add the explanation in the comments?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could rename the development network in the hardhat.config.ts in all projects to local. The package we publish to the NPM Registry will still be tagged development.
WDYT @michalinacienciala?

But let's not block this PR on that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@michalinacienciala michalinacienciala Aug 11, 2022

Choose a reason for hiding this comment

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

We could rename the development network in the hardhat.config.ts in all projects to local. The package we publish to the NPM Registry will still be tagged development.
WDYT @michalinacienciala?

Sounds good to me, I prefer this from changing the names of the packages.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dimpar
Copy link
Copy Markdown
Contributor

dimpar commented Aug 9, 2022

Tested by running installation scripts and building using make local local_threshold_path=../tmp/solidity-contracts local_tbtc_path=../tmp/tbtc-v2/solidity command. The keep-core client was started successfully by running the following command and no contract addresses were provided.

KEEP_ETHEREUM_PASSWORD=password go run . start --ethereum.url ws://localhost:8546 --ethereum.keyFile /Users/dp/ethereum/data/keystore/UTC--2019-08-27T11-40-44.016554000Z--407190f04d838ec47dad4c0e0d2a9c49d7737479 --storage.dataDir /Users/dp/.keep/keep-core/storage/client-v2

@nkuba nkuba requested a review from dimpar August 10, 2022 06:26
@dimpar dimpar mentioned this pull request Aug 11, 2022
@dimpar dimpar enabled auto-merge August 11, 2022 14:36
@dimpar dimpar merged commit a1dff16 into main Aug 11, 2022
@dimpar dimpar deleted the make-local-artifacts branch August 11, 2022 14:43
nkuba added a commit that referenced this pull request Aug 19, 2022
This PR uses changes made in #3129
to use the locally deployed artifacts. Now `install.sh` will build the client
with the local artifacts. Also, `start.sh` can be used now to start a client.

To start a client we need to:

Manually configure `config.toml` in the `/configs` dir. Providing contract addresses
is no longer required for "development" env. Next:
- run `scripts/install.sh`
- run `scripts/initialize.sh --stake-owner <address>`
- run `scripts/start.sh`

Available params for `start.sh`
- `KEEP_ETHEREUM_PASSWORD` env var. If not provided, "password" is used
- flag: `--config-dir` path to the config file. `/configs` is default
@pdyraga pdyraga added this to the v2.0.0-m1 milestone Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants