refactor: aggregator uses flags instead of config-file #60
Conversation
samlaf
left a comment
samlaf
left a comment
There was a problem hiding this comment.
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
| } | ||
| EnvironmentFlag = cli.StringFlag{ | ||
| Name: "environment", | ||
| Required: true, |
There was a problem hiding this comment.
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.
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.
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
| EthRpcUrlFlag, | ||
| EthWsUrlFlag, | ||
| AggregatorServerIpPortAddressFlag, | ||
| CredibleSquaringDeploymentFileFlag, |
There was a problem hiding this comment.
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.
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.
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.
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.
| 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.
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. |
2 participants
Working on this issue: #57
Refactored the aggregator such that
make start-aggregatoruses flags instead of config-files for the following variables:Question: Should I delete the
config-files/aggregator.yamlfile ?