-
Notifications
You must be signed in to change notification settings - Fork 225
feat(tools): new eigenda contract addresses reading cli #1804
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: master
Are you sure you want to change the base?
Conversation
This is used to fetch all addresses from a given EigenDADirectory
Given an eth-rpc it prints a table, json, or csv output of all contract names and their address.
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
if len(networks) == 0 { | ||
return "", fmt.Errorf("no EigenDA network found for chain ID: %s", chainID) | ||
} | ||
return networks[0], nil |
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 it make more sense to only do this default selection iff len(networks) == 1
.
In cases where there are multiple options, seems safer to make the user explicitly choose, instead of potentially choosing the wrong one
@@ -64,4 +63,23 @@ func (r *EigenDADirectoryReader) GetServiceManagerAddress() (gethcommon.Address, | |||
return r.getAddressWithValidation(ContractNames.ServiceManager) | |||
} | |||
|
|||
// GetAllAddresses returns all contract addresses from the directory in a map keyed by contract name | |||
func (r *EigenDADirectoryReader) GetAllAddresses() (map[string]gethcommon.Address, error) { | |||
names, err := r.contract.GetAllNames(&bind.CallOpts{}) |
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.
no timeout?
func (r *EigenDADirectoryReader) GetAllAddresses() (map[string]gethcommon.Address, error) { | ||
names, err := r.contract.GetAllNames(&bind.CallOpts{}) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to get all contract names: %w", err) |
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.
Your error messages in this method include "failed to"
|
||
addresses := make(map[string]gethcommon.Address) | ||
for _, name := range names { | ||
addr, err := r.contract.GetAddress0(&bind.CallOpts{}, name) |
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.
no timeout?
|
||
return addresses, nil | ||
} | ||
|
||
// TODO: add other getters for other contracts; they are not needed for the current usage |
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 notice you don't attribute TODOs to yourself, e.g. TODO(samlaf)
. Any reason? Curious your thoughts on the attribution- when I wrote the style guide I included it as a soft requirement, but we could roll that back if you disagree
} | ||
directoryAddr, err = network.GetEigenDADirectory() | ||
if err != nil { | ||
return fmt.Errorf("error getting EigenDADirectory address for network %s: %w", network, err) |
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.
Here too you're prefixing errors with words like "error getting". Intentional, or LLM not following your preferences?
if err != nil { | ||
return err | ||
} | ||
directoryAddr, err = network.GetEigenDADirectory() |
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 the logger output is slightly different, but theoretically you could pull this code out of the if else
, since both branches are doing essentially the same thing
directoryAddr, err = network.GetEigenDADirectory()
if err != nil {
return fmt.Errorf("error getting EigenDADirectory address for network %s: %w", network, err)
}
logger.Printf("Auto-detected EigenDADirectory address %s for network %s", directoryAddr, networkName)
return fmt.Errorf("failed to create EigenDADirectory reader: %w", err) | ||
} | ||
|
||
// Get all addresses |
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.
not exactly a helpful comment on a method called GetAllAddresses
func printTable(addressMap map[string]gethcommon.Address) { | ||
t := table.NewWriter() | ||
t.SetOutputMirror(os.Stdout) | ||
t.AppendHeader(table.Row{"Contract Name", "Address"}) |
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.
Contract Name
and Address
are duplicated strings in this file
if i == len(addressMap)-1 { | ||
comma = "" | ||
} | ||
fmt.Printf(" {\"name\": \"%s\", \"address\": \"%s\"}%s\n", name, addr.Hex(), comma) |
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.
any reason to just use name
here, instead of contract_name
, as with the other print methods?
Can you also post an example of the other output types? |
) | ||
|
||
const ( | ||
ethRpcUrlFlagName = "eth-rpc-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.
I'm not a fan of how we handle string literals when doing flag+env configurations.
All the flag names are specified as const strings at the top, but then the env var name string is defined far away, where the flag struct is initialized. Modifying a name is tedious at best, and error prone at worst (e.g., the flag name gets changed, but the env name is forgotten)
If we had a simple struct, named FlagAndEnvField
(or something better...) which contains fields for FlagName
, and EnvVarName
, then the relevant names for a given field would be right next to each other, and it would be easier to keep them properly in sync.
May or may not make sense to start such a system in this PR. It's just a painpoint I've noticed before, that probably would be good to address at some point.
func EigenDANetworkFromString(inputString string) (EigenDANetwork, error) { | ||
network := EigenDANetwork(inputString) | ||
|
||
switch network { | ||
case HoleskyTestnetEigenDANetwork, HoleskyPreprodEigenDANetwork, SepoliaTestnetEigenDANetwork, MainnetEigenDANetwork: | ||
return network, nil | ||
default: | ||
return "", fmt.Errorf("unknown network type: %s", inputString) | ||
allowedNetworks := []string{ | ||
MainnetEigenDANetwork.String(), |
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.
MainnetBeta missing
Why are these changes needed?
My endgoal target is to create a "discovery" CLI (hence the name) that given a network would discover all addresses (contracts, disperser/relays URLs, operator IPs, etc). This means it should probably incorporate the quorumscan tool, and others. I think ideally we would merge all of them into a single
eigenda
CLI with subcommands, which would be easier to use for everyone.Right now it only discovers contract address by reading them from the EigenDADirectory address provided (or using the default one for the chain-id or network provided).

Given an eth-rpc it prints a table, json, or csv output of all contract names and their address.
Checks