-
Notifications
You must be signed in to change notification settings - Fork 61
feat: Sedge<>Aztec integration #539
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #539 +/- ##
===========================================
+ Coverage 23.69% 24.28% +0.58%
===========================================
Files 121 121
Lines 19671 19922 +251
===========================================
+ Hits 4662 4838 +176
- Misses 14472 14522 +50
- Partials 537 562 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stdevMac
left a comment
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.
We need to add this as well
https://docs.aztec.network/network/setup/sequencer_management#step-4-configure-environment-variables
When using nethermind
cli/sub_gen.go
Outdated
| cmd.Flags().StringVarP(&flags.executionApiUrl, "execution-api-url", "", "", "Set execution api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") | ||
| cmd.Flags().StringVarP(&flags.consensusApiUrl, "consensus-url", "", "", "Set consensus api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") |
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.
Hardcoded values here for optimism, change to aztec
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 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 one is still missing.
, and only create optimism nodes.
We need to update this description to aztec
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.
| # --- Aztec Node configuration --- | ||
| AZTEC_IMAGE_VERSION={{.AztecImage}} | ||
| AZTEC_DATA_DIRECTORY={{.AztecDataDir}} | ||
| AZTEC_KEYSTORE_PATH={{.AztecSequencerKeystorePath}} |
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.
If set on full node, this shouldn't be added
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.
smth like this:
{{- if eq .AztecNodeType "sequencer" }}
- ${AZTEC_KEYSTORE_PATH}:/var/lib/keystore
{{- end }}
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 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 still not done, AZTEC_KEYSTORE_PATH is showing empty when we are running a full-node
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.
| ETHEREUM_HOSTS: ${AZTEC_ETHEREUM_HOSTS} | ||
| L1_CONSENSUS_HOST_URLS: ${AZTEC_L1_CONSENSUS_HOST_URLS} | ||
| {{- if eq .AztecNodeType "sequencer" }} | ||
| KEY_STORE_DIRECTORY: /var/lib/keystore |
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.
we need to copy the keystore file inside the folder of sedge, because we are using json file when setting the node, and set up this:
AZTEC_KEYSTORE_PATH=/root/.aztec/keystore/key1.json
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.
cli/sub_gen.go
Outdated
| cmd.Flags().StringArrayVar(&flags.aztecExtraFlags, "aztec-extra-flag", []string{}, "Additional flag to configure the Aztec node service in the generated docker-compose script. Example: 'sedge generate aztec --aztec-extra-flag \"p2p.maxPeers=200\" --aztec-extra-flag \"txPool.maxSize=10000\"'") | ||
| cmd.Flags().StringVarP(&flags.consensusName, "consensus", "c", "", "Consensus engine client, e.g. teku, lodestar, prysm, lighthouse, Nimbus. Additionally, you can use this syntax '<CLIENT>:<DOCKER_IMAGE>' to override the docker image used for the client. If you want to use the default docker image, just use the client name") | ||
| cmd.Flags().StringVarP(&flags.executionName, "execution", "e", "", "Execution engine client, e.g. geth, nethermind, besu, erigon. Additionally, you can use this syntax '<CLIENT>:<DOCKER_IMAGE>' to override the docker image used for the client. If you want to use the default docker image, just use the client name") | ||
| cmd.Flags().StringVarP(&flags.executionApiUrl, "execution-api-url", "", "", "Set execution api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") |
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.
Hardcoded values here for optimism, change to aztec
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.
cli/sub_gen.go
Outdated
| // Bind flags | ||
| cmd.Flags().StringVar(&flags.aztecType, "type", aztecNodeTypeFullNode, fmt.Sprintf("Aztec node type. One of: %s,%s", aztecNodeTypeFullNode, aztecNodeTypeSequencer)) | ||
| cmd.Flags().StringVar(&flags.aztecSequencerKeystorePath, "aztec-keystore-path", "", "Path to Aztec sequencer keystore.json file (required when --type sequencer). The keystore must be generated using 'aztec validator-keys new' command. See https://docs.aztec.network/network/setup/sequencer_management for details.") | ||
| cmd.Flags().StringVar(&flags.aztecP2pIp, "aztec-p2p-ip", "", "P2P IP address for Aztec sequencer. This is the IP address that other nodes will use to connect to this sequencer.") |
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.
Since this is only 1 node, see if possible to show the default image here.
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 already done here
cli/sub_gen.go
Outdated
| cmd.Flags().StringVarP(&flags.executionApiUrl, "execution-api-url", "", "", "Set execution api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") | ||
| cmd.Flags().StringVarP(&flags.consensusApiUrl, "consensus-url", "", "", "Set consensus api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") |
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 still need to be fixed, look at the descriptions, it say at the create optimism nodes.
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.
cli/sub_gen.go
Outdated
| cmd.Flags().StringVarP(&flags.executionApiUrl, "execution-api-url", "", "", "Set execution api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") | ||
| cmd.Flags().StringVarP(&flags.consensusApiUrl, "consensus-url", "", "", "Set consensus api url. If Set, will omit the creation of execution and beacon nodes, and only create optimism nodes.") |
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 one is still missing.
, and only create optimism nodes.
We need to update this description to aztec
| # --- Aztec Node configuration --- | ||
| AZTEC_IMAGE_VERSION={{.AztecImage}} | ||
| AZTEC_DATA_DIRECTORY={{.AztecDataDir}} | ||
| AZTEC_KEYSTORE_PATH={{.AztecSequencerKeystorePath}} |
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 still not done, AZTEC_KEYSTORE_PATH is showing empty when we are running a full-node
|
@stdevMac changes addressed for the following: |
#383
#469
Changes:
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests?