-
Notifications
You must be signed in to change notification settings - Fork 124
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
base: master
Are you sure you want to change the base?
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.
Added a bunch of comments. I'll also run the tests so you an fix those, assuming they will be broken with these changes.
core/config/config.go
Outdated
@@ -164,6 +180,26 @@ var ( | |||
Required: true, | |||
Usage: "Load configuration from `FILE`", | |||
} | |||
EnvironmentFlag = cli.StringFlag{ | |||
Name: "environment", | |||
Required: true, |
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.
set to not required and give default value of development
Required: true, | ||
Usage: "Ethereum WS URL", | ||
} | ||
AggregatorServerIpPortAddressFlag = cli.StringFlag{ |
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 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
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.
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)
core/config/config.go
Outdated
EnvironmentFlag, | ||
EthRpcUrlFlag, | ||
EthWsUrlFlag, | ||
AggregatorServerIpPortAddressFlag, | ||
CredibleSquaringDeploymentFileFlag, |
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.
remove 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.
You mean move them from requiredFlags
to optionalFlags
core/config/config.go
Outdated
|
||
environment := ctx.GlobalString(EnvironmentFlag.Name) | ||
if environment != "" { | ||
configRaw.Environment = sdklogging.LogLevel(environment) |
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 don't need to use configRaw anymore. Remove it.
core/config/config.go
Outdated
if ethRpcUrl != "" { | ||
configRaw.EthRpcUrl = ethRpcUrl | ||
} |
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.
these feel like bugs. What if the ethRpcUrl is set to ""? You should probably return an error.
ETH_RPC_URL=http://localhost:8545 | ||
ETH_WS_URL=ws://localhost:8545 | ||
|
||
AGGREGATOR_SERVER_IP_PORT_ADDRESS=localhost:8090 |
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 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.
core/config/config.go
Outdated
Name: "eth-rpc-url", | ||
Required: true, | ||
Usage: "Ethereum RPC URL", | ||
} | ||
EthWsUrlFlag = cli.StringFlag{ | ||
Name: "eth-ws-url", | ||
Required: true, | ||
Usage: "Ethereum WS URL", |
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 add EnvVar flags to these? See how eigenDA does it: https://github.com/Layr-Labs/eigenda/blob/e7ec7fa7f8dddda8fd64ec77435694d1c5dc9851/disperser/cmd/batcher/flags/flags.go#L25
@samlaf looking at how eigenda implements flags, I think we can create a separate file for flags inside core/config given they're getting slightly large. |
Working on this issue: #57
Refactored the aggregator such that
make start-aggregator
uses flags instead of config-files for the following variables:Question: Should I delete the
config-files/aggregator.yaml
file ?