-
Notifications
You must be signed in to change notification settings - Fork 238
feat: eth client address directory #1691
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
Conversation
2e39705
to
8d37186
Compare
be772d1
to
d4eb749
Compare
- Add NewReaderWithAddressDirectory() for address directory-based initialization - ContractNames constants matching AddressDirectoryConstants.sol - optional AddressDirectoryFlag, BlsOperatorStateRetrieverFlag, and EigenDAServiceManagerFlag - validation to require either address directory OR both legacy flags - backward compatibility for existing deployments without address directory
d4eb749
to
cf716c1
Compare
72aa96a
to
af2f194
Compare
af2f194
to
2a9ba2d
Compare
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.
Have we thought through the consequences of using a dynamic lookup system like the AddressRegistry (as opposed to hardcoded addresses)? Does it have the exact same behavior as the current strategy of reading from serviceManager pointers?
I guess the thing I'm unsure is... would a contractAddress in the addressRegistry ever change in place? If so, what are the conditions for this/when would we want to do this? If this can happen, then can't you run into a situation where the address changes, the binary crashes, gets restarted and loads from a new contract address, but this new contract then has a breaking api change, and the (hardcoded) bindings then don't work. How do we prevent this from happening?
What behaviors are you looking for here? When will it not be enough to just replace a service manager pointer with this one?
Address changes should happen with communication, just like any contract upgrade. It will be owned by the DA ops multisig. In that sense, it should be handled the same way as any contract upgrade would be. This mechanism allows us to reduce the technical load of doing upgrades. |
95291db
to
23bbb80
Compare
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
@@ -29,6 +29,7 @@ var ( | |||
) | |||
ServiceManagerAddrFlagName = withFlagPrefix("service-manager-addr") | |||
BLSOperatorStateRetrieverFlagName = withFlagPrefix("bls-operator-state-retriever-addr") | |||
EigenDADirectoryFlagName = withFlagPrefix("eigen-da-directory") |
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 used eigen-da-directory
here, but the other flags are named eigenda-directory
,
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.
updated 1ea1ff4
disperserHostname = "disperser-testnet-holesky.eigenda.xyz" | ||
|
||
// Eth client | ||
addressDirectoryAddress = "0x90776Ea0E99E4c38aA1Efe575a61B3E40160A2FE" |
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 is another case where the term "address directory" is used, instead of "EigenDA Directory"
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.
core/eth/address_directory_reader.go
Outdated
|
||
// AddressDirectoryReader wraps the address directory contract and provides | ||
// safe getters for contract addresses with zero address validation | ||
type AddressDirectoryReader struct { |
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 struct and file name use "address directory" terminology, instead of "EigenDA Directory"
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.
core/eth/reader.go
Outdated
@@ -65,21 +68,37 @@ var _ core.Reader = (*Reader)(nil) | |||
func NewReader( | |||
logger logging.Logger, | |||
client common.EthClient, | |||
blsOperatorStateRetrieverHexAddr string, | |||
eigenDAServiceManagerHexAddr string) (*Reader, error) { | |||
addressDirectoryHexAddr string) (*Reader, error) { |
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 is another case where the term "address directory" is used, instead of "EigenDA Directory"
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.
core/eth/writer.go
Outdated
@@ -37,12 +37,11 @@ var _ core.Writer = (*Writer)(nil) | |||
func NewWriter( | |||
logger logging.Logger, | |||
client common.EthClient, | |||
blsOperatorStateRetrieverHexAddr string, | |||
eigenDAServiceManagerHexAddr string) (*Writer, error) { | |||
addressDirectoryHexAddr string) (*Writer, error) { |
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 is another case where the term "address directory" is used, instead of "EigenDA Directory"
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.
@@ -204,7 +204,7 @@ func TestChurner(t *testing.T) { | |||
assert.NoError(t, err) | |||
} | |||
|
|||
func createTransactorFromScratch(privateKey, operatorStateRetriever, serviceManager string, logger logging.Logger) (*eth.Writer, error) { | |||
func createTransactorFromScratch(privateKey, addressDirectory string, logger logging.Logger) (*eth.Writer, error) { |
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 is another case where the term "address directory" is used, instead of "EigenDA Directory"
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.
relay/cmd/flags/flags.go
Outdated
Usage: "Address of the Eigen DA service manager", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(envVarPrefix, "EIGEN_DA_SERVICE_MANAGER_ADDR"), | ||
AddressDirectoryFlag = 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.
here is another case where the term "address directory" is used, instead of "EigenDA Directory"
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.
ServiceManagerAddrFlagName = withFlagPrefix("service-manager-addr") | ||
BLSOperatorStateRetrieverFlagName = withFlagPrefix("bls-operator-state-retriever-addr") |
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.
think we should mark these as deprecated and hint people to start moving to the eigenda-directory? Prob want to do it across all of our services (apiserver, nodes, etc). Everything in this PR. And eventually we'll remove all other address flags and only keep the eigenda-directory.
not sure if urfave has a way to mark flags as deprecated. If not then we might need to manually log a warn. cc @dmanc who will be updating our helm charts to use these new flags. Can you also add this as a note to this PR description? This kind of external config changes are very important to highlight.
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.
Please make the binaries backwards compatible again by keeping the old ServiceManager flag around, and allowing binaries to be started with those flags. We don't want to break people.
retriever/flags/flags.go
Outdated
BlsOperatorStateRetrieverFlag = cli.StringFlag{ | ||
Name: common.PrefixFlag(FlagPrefix, "bls-operator-state-retriever"), | ||
Usage: "Address of the BLS Operator State Retriever", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(envPrefix, "BLS_OPERATOR_STATE_RETRIVER"), |
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.
don't remove this flag, since thats a breaking change. instead mark it as deprecated and log a warning when its used, prompting the user to use the new flags.
} | ||
// This flag is kept for retriever's fetchBatchHeader; can later be removed by utilizing AddressDirectoryFlag | ||
EigenDAServiceManagerFlag = 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.
you didn't add the flag back, just the config. let's not delete flags until we're ready for the breaking change (which should be announced in advance).
relay/cmd/flags/flags.go
Outdated
BlsOperatorStateRetrieverAddrFlag = cli.StringFlag{ | ||
Name: common.PrefixFlag(FlagPrefix, "bls-operator-state-retriever-addr"), | ||
Usage: "Address of the BLS operator state retriever", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(envVarPrefix, "BLS_OPERATOR_STATE_RETRIEVER_ADDR"), | ||
} | ||
EigenDAServiceManagerAddrFlag = cli.StringFlag{ | ||
Name: common.PrefixFlag(FlagPrefix, "eigen-da-service-manager-addr"), | ||
Usage: "Address of the Eigen DA service manager", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(envVarPrefix, "EIGEN_DA_SERVICE_MANAGER_ADDR"), |
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.
add these flags back, we don't want to create breaking changes. Every binary should be backwards compatible with these old flags. you can mark them as deprecated but not delete.
Name: common.PrefixFlag(FlagPrefix, "eigenda-directory"), | ||
Usage: "Address of the EigenDA Address Directory", | ||
Required: false, | ||
EnvVar: common.PrefixEnvVar(envPrefix, "EIGENDA_DIRECTORY"), | ||
} |
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.
dont delete these flags. see my other comments.
node/flags/flags.go
Outdated
BlsOperatorStateRetrieverFlag = cli.StringFlag{ | ||
Name: common.PrefixFlag(FlagPrefix, "bls-operator-state-retriever"), | ||
Usage: "Address of the BLS Operator State Retriever", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(EnvVarPrefix, "BLS_OPERATOR_STATE_RETRIVER"), | ||
} | ||
EigenDAServiceManagerFlag = cli.StringFlag{ | ||
Name: common.PrefixFlag(FlagPrefix, "eigenda-service-manager"), | ||
Usage: "Address of the EigenDA Service Manager", | ||
Required: true, | ||
EnvVar: common.PrefixEnvVar(EnvVarPrefix, "EIGENDA_SERVICE_MANAGER"), |
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.
please do not ever do this. this will break users and they will scream at us. flag breaking changes should be announced way ahead of time, and should have been deprecated for a while, and any PRs removing flags (breaking API change!) should have its title with an !
like feat!: eth client address directory
to mark the breaking change (to be clear we should not do this right now, but the ! is useful to flag to the release captain that a MAJOR version bump is needed)
@hopeyen and I chatted offline. I'm very very sorry, as I caused a lot of churn and back-and-forth on this one, initially requesting that the flags be deleted, and later on realizing this was not good. Mea culpa 😭 Let's discuss during next week's ReleaseMgmt meeting how we want to deal with config changes wrt breaking changes:
|
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.
LGTM except for the wording of the deprecated notice. Please change everywhere and then we can merge.
registryCoordinatorAddress = "0x53012C69A189cfA2D9d29eb6F19B32e0A2EA3490" | ||
blsOperatorStateRetrieverAddress = "0x003497Dd77E5B73C40e8aCbB562C8bb0410320E7" | ||
eigenDAServiceManagerAddress = "0xD4A7E1Bd8015057293f0D0A557088c286942e84b" |
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.
nit: examples should use SOTA stuff, so should only use the eigendaDirectoryAddress.
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.
blsOperatorStateRetrieverAddress
is still used for CertBuilder, that's not changed within this PR.
I will update eigenDAServiceManagerAddress
to an empty string because new EthClient take all three arguments
@@ -119,18 +119,25 @@ func CLIFlags(envPrefix, category string) []cli.Flag { | |||
}, | |||
&cli.StringFlag{ | |||
Name: ServiceManagerAddrFlagName, | |||
Usage: "Address of the EigenDA service manager contract.", | |||
Usage: "[Deprecating soon in favor of EigenDADirectory] Address of the EigenDA Service Manager contract.", |
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.
Usage: "[Deprecating soon in favor of EigenDADirectory] Address of the EigenDA Service Manager contract.", | |
Usage: "[Deprecated: use EigenDADirectory instead] Address of the EigenDA Service Manager contract.", |
EnvVars: []string{withEnvPrefix(envPrefix, "BLS_OPERATOR_STATE_RETRIEVER_ADDR")}, | ||
Category: category, | ||
Required: false, | ||
}, | ||
&cli.StringFlag{ | ||
Name: EigenDADirectoryFlagName, | ||
Usage: "Address of the EigenDA directory contract.", |
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.
Usage: "Address of the EigenDA directory contract.", | |
Usage: "Address of the EigenDA directory contract, which points to all other EigenDA contract addresses. This is the only contract entrypoint needed offchain.", |
@@ -48,14 +48,14 @@ var ( | |||
} | |||
BlsOperatorStateRetrieverFlag = cli.StringFlag{ | |||
Name: common.PrefixFlag(FlagPrefix, "bls-operator-state-retriever"), | |||
Usage: "Address of the BLS Operator State Retriever", | |||
Required: true, | |||
Usage: "[Deprecating soon in favor of EigenDADirectory] Address of the BLS operator state Retriever", |
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.
Usage: "[Deprecating soon in favor of EigenDADirectory] Address of the BLS operator state Retriever", | |
Usage: "[Deprecated: use EigenDADirectory instead] Address of the BLS operator state Retriever", |
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.
It is already deprecated. Deprecated means there's a better way to do things and we're giving you some time to switch over to the new method before we delete this old method.
It's already deprecated, and will eventually get deleted. But better not to use relative language like "soon" or "eventually" because thats fuzzy and unuseful.
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.
Plesae change everywhere
Why are these changes needed?
Adds address directory support while maintaining backwards compatibility with legacy individual address configuration.
Services that uses an Eth Client will now be taking eigendaDirectory address in configurations, instead of blsOperatorStateRetriever and eigenDAServiceManager.
Disperser API server, batcher, controller, dataapi, node cmd, node plugin, operator churner, relay, retriever, ejection tool, quorumscan, semvarscan, proxy client
eigenda-directory
with each service's flag prefixChecks