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

refactor: aggregator uses flags instead of config-file #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ help:
AGGREGATOR_ECDSA_PRIV_KEY=0x2a871d0798f97d79848a013d4936a73bf4cc922c825d33c1cf7073dff6d409c6
CHALLENGER_ECDSA_PRIV_KEY=0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a

ETH_RPC_URL=http://localhost:8545
ETH_WS_URL=ws://localhost:8545

AGGREGATOR_SERVER_IP_PORT_ADDRESS=localhost:8090
Comment on lines +10 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to get out of hands. Let's move the config variables to a .env file that users can source before running make commands. ALso this way we can have an anvil.env file and a holesky.env file, etc.


CHAINID=31337
# Make sure to update this if the strategy address changes
# check in contracts/script/output/${CHAINID}/credible_squaring_avs_deployment_output.json
Expand Down Expand Up @@ -68,7 +73,10 @@ send-fund: ## sends fund to the operator saved in tests/keys/test.ecdsa.key.json
# TODO: piping to zap-pretty only works when zapper environment is set to production, unsure why
____OFFCHAIN_SOFTWARE___: ##
start-aggregator: ##
go run aggregator/cmd/main.go --config config-files/aggregator.yaml \
go run aggregator/cmd/main.go --environment production \
--eth-rpc-url ${ETH_RPC_URL} \
--eth-ws-url ${ETH_WS_URL} \
--aggregator-server-ip-port-address ${AGGREGATOR_SERVER_IP_PORT_ADDRESS} \
--credible-squaring-deployment ${DEPLOYMENT_FILES_DIR}/credible_squaring_avs_deployment_output.json \
--ecdsa-private-key ${AGGREGATOR_ECDSA_PRIV_KEY} \
2>&1 | zap-pretty
Expand Down
49 changes: 44 additions & 5 deletions core/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,31 @@ type IncredibleSquaringContractsRaw struct {
OperatorStateRetrieverAddr string `json:"operatorStateRetriever"`
}

// NewConfig parses config file to read from from flags or environment variables
// NewConfig parses config file to read from flags or environment variables
// Note: This config is shared by challenger and aggregator and so we put in the core.
// Operator has a different config and is meant to be used by the operator CLI.
func NewConfig(ctx *cli.Context) (*Config, error) {

var configRaw ConfigRaw
configFilePath := ctx.GlobalString(ConfigFileFlag.Name)
if configFilePath != "" {
sdkutils.ReadYamlConfig(configFilePath, &configRaw)

environment := ctx.GlobalString(EnvironmentFlag.Name)
if environment != "" {
configRaw.Environment = sdklogging.LogLevel(environment)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need to use configRaw anymore. Remove it.

}

ethRpcUrl := ctx.GlobalString(EthRpcUrlFlag.Name)
if ethRpcUrl != "" {
configRaw.EthRpcUrl = ethRpcUrl
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

these feel like bugs. What if the ethRpcUrl is set to ""? You should probably return an error.


ethWsUrl := ctx.GlobalString(EthWsUrlFlag.Name)
if ethWsUrl != "" {
configRaw.EthWsUrl = ethWsUrl
}

aggregatorServerIpPortAddr := ctx.GlobalString(AggregatorServerIpPortAddressFlag.Name)
if aggregatorServerIpPortAddr != "" {
configRaw.AggregatorServerIpPortAddr = aggregatorServerIpPortAddr
}

var credibleSquaringDeploymentRaw IncredibleSquaringDeploymentRaw
Expand Down Expand Up @@ -164,6 +180,26 @@ var (
Required: true,
Usage: "Load configuration from `FILE`",
}
EnvironmentFlag = cli.StringFlag{
Name: "environment",
Required: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

set to not required and give default value of development

Usage: "Set the environment (production or development)",
}
EthRpcUrlFlag = cli.StringFlag{
Name: "eth-rpc-url",
Required: true,
Usage: "Ethereum RPC URL",
}
EthWsUrlFlag = cli.StringFlag{
Name: "eth-ws-url",
Required: true,
Usage: "Ethereum WS URL",
Copy link
Collaborator

Choose a reason for hiding this comment

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

}
AggregatorServerIpPortAddressFlag = cli.StringFlag{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this was the old variable name, but don't like it anymore. Can we separate into two separate flags, ListenHost and ListenPort? This makes it more obvious that ip can be a hostname, plus feel like separating them is more canonical from what I've seen.

Also you can make the default ListenHost 0.0.0.0

Copy link
Author

Choose a reason for hiding this comment

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

So which one of them should I pass in config.AggregatorServerIpPortAddr ? If you want both to be passed then the config struct needs to be refactored which I think I could do in a different PR to reduce complexity. (as I would have to make changes in aggregator.go as well)

Name: "aggregator-server-ip-port-address",
Required: true,
Usage: "Aggregator server IP PORT address",
}
CredibleSquaringDeploymentFileFlag = cli.StringFlag{
Name: "credible-squaring-deployment",
Required: true,
Expand All @@ -179,7 +215,10 @@ var (
)

var requiredFlags = []cli.Flag{
ConfigFileFlag,
EnvironmentFlag,
EthRpcUrlFlag,
EthWsUrlFlag,
AggregatorServerIpPortAddressFlag,
CredibleSquaringDeploymentFileFlag,
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this flag

Copy link
Author

Choose a reason for hiding this comment

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

You mean move them from requiredFlags to optionalFlags

EcdsaPrivateKeyFlag,
}
Expand Down
Loading