-
Notifications
You must be signed in to change notification settings - Fork 56
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
Client deployment to use with the sdk #698
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add README how to deploy client
- I’d move start-client inside client
- I think you need a
client.env
where one can configure parameters as Web3Url and proposerUrl. Also, i would add the location of s3 bucket in a variable in case it changes.
Added suggested changes |
nightfall-client/README.md
Outdated
@@ -8,7 +8,7 @@ | |||
|
|||
# nightfall-client | |||
|
|||
This code generates a containerised application that can be used to interact with Nightfall_2 via | |||
This code generates a containerised application that can be used to interact with Nightfall_3 via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this Polygon Nightfall from now on
nightfall-client/README.md
Outdated
@@ -19,79 +19,39 @@ nightfall-client requires a number of services to be present for it to work. The | |||
instructions explain how to run all of these up, in a similar 'developer' mode with local file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This README is intended for outside people trying to use the SDK. So, it needs to be very clear. Wha'ts developer mode?
nightfall-client/README.md
Outdated
[nightfall-scripts](https://github.com/EYBlockchain/nightfall-scripts), which does a similar job but | ||
uses docker images that do not rely on your local file system and therefore requires no other code | ||
repositories to be downloaded. | ||
If you just want to run Nightfall_3 then an easier way is to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polygon Nightfall
nightfall-client/client.env
Outdated
@@ -0,0 +1,4 @@ | |||
ETH_NETWORK=goerli | |||
BLOCKCHAIN_URL=your web3 url provider to access the blockchain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide an example to understand format
@@ -0,0 +1,72 @@ | |||
version: '3.5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some requirements on the docker-compose version required to be installed. Can you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I choose the same version and format of the docker-compose files of the root folder for other purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What i meant is that there should be a requirements section in the README where we explain what sw is needed to run this. For example, we need nodeJS v xxx, docker-compose v.xxx,... Again, this README needs to be enough for a user to spin a client without the need to go to the main README. All information required should be here.
nightfall-client/client.env
Outdated
@@ -0,0 +1,4 @@ | |||
ETH_NETWORK=goerli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call this file something else (client-example.env
for example) . Add client.env
to .gitignore. In the instructions, then specify that one should copy client-example.env
to client.env
and add their own information
|
||
```sh | ||
$ npm i | ||
ETH_NETWORK=goerli or mainnet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be clear that this client is to connect to existing testnet and mainnet deployments. Also, can you comment on how to deploy 2+ clients? And if one wants to launch a client vs a local environment. How would this be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments in README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left my comments in the review
nightfall-client/README.md
Outdated
```sh | ||
$ docker-compose build --build-arg GPR_TOKEN=<paste your token here> | ||
``` | ||
./start-client -c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you stop a client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With ctrl+c as start-nightfall. It shuts down the containers because the trap
in the start-client
script catch the event and runs a docker-compose down.
# bindings. See the readme for more information. | ||
services: | ||
client1: | ||
image: ghcr.io/eyblockchain/nightfall3-client:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should change the image name.... its strange that its called eyblockchain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this applies to all images that we are building now in the Dockerfiles in the root folder and not only this PR.
So we have to do it in all images I think.
- ghcr.io/eyblockchain/nightfall3-deployer:latest
- ghcr.io/eyblockchain/nightfall3-worker:latest
- ghcr.io/eyblockchain/nightfall3-client:latest
- ghcr.io/eyblockchain/nightfall3-optimist:latest
- ghcr.io/eyblockchain/nightfall3-proposer:latest
- ghcr.io/eyblockchain/nightfall3-user-local:latest
nightfall-client/start-client
Outdated
usage() | ||
{ | ||
echo "Usage:" | ||
echo " -c or --client; for client service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this flag (-c)? Does it have sense to call this script without this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Changed to always run this part by default.
nightfall-client/start-client
Outdated
} | ||
|
||
# select a Geth or Ganache client | ||
if [ -z "$1" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Expected at least 1 argument. With -c by default and after removal then it has no sense.
Added
|
10902c6
to
7a0fc37
Compare
c7da625
to
6dcf638
Compare
nightfall-client/start-client
Outdated
fi | ||
|
||
# delete env file | ||
rm -f ${ENV_FILE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats this ENV_FILE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from copy paste. Removed.
exit 1 | ||
fi | ||
|
||
VOLUME_LIST=$(docker volume ls -q) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does docker volume get initialized? Doesn't this require to have run setup-nightfall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docker volume gets initialized with start-client
in the docker-compose docker-compose.client.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @daveroga, I think this needs some changes. Let me know if you wanna discuss them.
@@ -44,14 +53,73 @@ const syncState = async (fromBlock = 'earliest', toBlock = 'latest', eventFilter | |||
} | |||
}; | |||
|
|||
const downloadFile = async (fileUrl, outputLocationPath) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have validations for these parameters (e.g. valid URL, path exists, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createWriteStream
creates the file passed as outputLocationPath
. It doesn't exists in principle. If the URL is not valid will thrown an error that should be captured in the parent. Is this what you mean?
test/client/erc20.test.mjs
Outdated
logger.debug(after, before, before + remainingBalance - transferValue); | ||
expect(after).to.be.lessThan(before); | ||
expect(after).to.satisfy(balance => { | ||
return balance < before || balance === before + remainingBalance - transferValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're relying on that the sequence of the tests will be preserved by the testing library when running them, I'm not sure if this is the best approach, what if someone changes the ordering of the test methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by changing the ordering of the test methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @daveroga, I think you've missed some comments, please take a look on them.
Reviewed and updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better now, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes and suggestions
@@ -44,14 +53,73 @@ const syncState = async (fromBlock = 'earliest', toBlock = 'latest', eventFilter | |||
} | |||
}; | |||
|
|||
const downloadFile = async (fileUrl, outputLocationPath) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createWriteStream
creates the file passed as outputLocationPath
. It doesn't exists in principle. If the URL is not valid will thrown an error that should be captured in the parent. Is this what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Fixes: #711, #681
Build client to be able to test with the SDK for testnet (goerli) and production (mainnet).
Based in docker-compose and script to start client.
nightfall-client
andworker
to download files from the public S3 bucket of the contracts abi in json and output of circuits based on some parameters.nightfall-client
that was outdated and added information about to configure and run the client.To test the client run:
client.env
in the root folder ofnightfall-client
./start-client -d
in the root folder ofnightfall-client
client
folder with correct values and connect:Now the client is ready to persist the data in the volume and when you start again with
./start-client -d
it syncs again but without removing the data from the volumes.If you want to remove the volumes first you can run
./start-client -d -r
with the-r
for removing the volumes first.Tasks